Add LookupBlockIndex by promag · Pull Request #11041 · bitcoin/bitcoin

promag · 2017-08-13T16:24:41Z

Replace all mapBlockIndex lookups with the new LookupBlockIndex(). In some cases it avoids a second lookup.

promag · 2017-08-13T17:39:50Z

src/rest.cpp

!pblockindex

promag · 2017-08-13T17:40:23Z

src/rpc/blockchain.cpp

Same as above.

promag · 2017-08-13T17:40:52Z

src/rpc/blockchain.cpp

Remove assert.

jonasschnelli · 2017-08-14T10:27:21Z

Concept ACK

TheBlueMatt · 2017-08-14T14:45:33Z

This may make #10692 a bit simpler, though I'm not sure it matters what order they come in.

promag · 2017-08-15T15:37:16Z

Fixed bug and rebased.

promag · 2017-08-22T14:56:47Z

@laanwj I believe it's ready.

laanwj · 2017-08-22T15:49:20Z

Concept ACK

meshcollider

utACK

meshcollider · 2017-08-23T01:20:42Z

src/net_processing.cpp

Maybe explicit cast to bool here for clarity

Prefer this?

return LookupBlockIndex(inv.hash) != nullptr;

Yeah or a ternary or even an if statement would be a lot clearer

promag · 2017-08-25T14:38:45Z

Rebased and fixed @meshcollider comment.

promag · 2017-10-30T14:45:15Z

src/validation.h

Missing assert lock is held.

Missing EXCLUSIVE_LOCKS_REQUIRED(cs_main) too :-)

ryanofsky

utACK 1d1768f2978bafaf166bfa218c899ddaeeccad2d

ryanofsky · 2017-11-02T16:25:59Z

src/wallet/wallet.cpp

In some of these places you could do

if (const CBlockIndex* pindex = LookupBlockIndex(wtx.hashBlock))

to reduce the scope of the pindex variables.

I didn't know you could declare inside an if statement. Cool.

promag · 2017-11-02T16:39:26Z

Thanks @ryanofsky, I have to rebase and then I'll take into account your comment above.

jimpo

Concept ACK. This is much cleaner.

jimpo · 2017-12-06T23:13:36Z

src/init.cpp

Need to keep the !mapBlockIndex.empty().

jimpo · 2017-12-06T23:17:43Z

src/net_processing.cpp

Needs a !

promag · 2017-12-06T23:39:19Z

This needs rebase. I can do it and adapt the new code if there is interest in merging it quick.

Maybe it should also make BlockMap mapBlockIndex static?

cc @MarcoFalke @laanwj

TheBlueMatt · 2017-12-12T15:42:26Z

Concept ACK. Would like to see this rebased sooner rather than later.

promag · 2017-12-13T15:42:44Z

Forgot to tackle @ryanofsky comment above..

maflcko · 2017-12-13T19:00:00Z

travis failure:

Assertion failed: lock cs_main not held in ./validation.h:433; locks held:

zcash: cherry picked from commit 02de6a6 zcash: bitcoin/bitcoin#11041

zcash: cherry picked from commit c651df8 zcash: bitcoin/bitcoin#11041

When accessing mapBlockIndex cs_main must be held. zcash: cherry picked from commit f814a3e zcash: bitcoin/bitcoin#11041

zcash: cherry picked from commit 43a32b7 zcash: bitcoin/bitcoin#11041

rebroad · 2021-05-11T10:23:48Z

@promag What's the reason for avoiding mapBlockIndex lookups?

promag · 2021-05-11T12:05:05Z

@rebroad it avoids duplicate lookups - its unnecessary work.

zcash: cherry picked from commit 02de6a6 zcash: bitcoin/bitcoin#11041

zcash: cherry picked from commit c651df8 zcash: bitcoin/bitcoin#11041

When accessing mapBlockIndex cs_main must be held. zcash: cherry picked from commit f814a3e zcash: bitcoin/bitcoin#11041

zcash: cherry picked from commit 43a32b7 zcash: bitcoin/bitcoin#11041

Contains both the changes done upstream and changes done in Dash codebase Signed-off-by: pasta <[email protected]>

…OCKORDER 33eb907 Fix ComputeTimeSmart test failure with -DDEBUG_LOCKORDER (Russell Yanofsky) Pull request description: Failure looks like: ``` Entering test case "ComputeTimeSmart" test_bitcoin: sync.cpp:100: void potential_deadlock_detected(const std::pair<void*, void*>&, const LockStack&, const LockStack&): Assertion `false' failed. unknown location(0): fatal error in "ComputeTimeSmart": signal: SIGABRT (application abort requested) wallet/test/wallet_tests.cpp(566): last checkpoint ``` Reproducible with: ``` ./configure --enable-debug make -C src test/test_bitcoin && src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ComputeTimeSmart ``` Seems to be caused by acquiring `cs_main` inside `CWallet::ComputeTimeSmart` in bitcoin#11041. I think this may be causing timeouts on travis like: https://travis-ci.org/bitcoin/bitcoin/jobs/353005676#L2692 Tree-SHA512: b263cd122ea9c88204d1d8e7e35291c71ea6319f05114c5009235a75dbd0f669bc0394f44afeed0d9eb08c2a956cd7c08f1ac4ef28616932fef9b43eaac5521b

promag force-pushed the 2017-08-lookup-block-index branch 2 times, most recently from 63e918e to 171213c Compare August 13, 2017 16:34

promag commented Aug 13, 2017

View reviewed changes

promag force-pushed the 2017-08-lookup-block-index branch 3 times, most recently from fb505b2 to 51d4607 Compare August 13, 2017 23:22

fanquake added the Refactoring label Aug 14, 2017

promag force-pushed the 2017-08-lookup-block-index branch 2 times, most recently from 930b525 to c515391 Compare August 15, 2017 15:07

meshcollider reviewed Aug 23, 2017

View reviewed changes

promag force-pushed the 2017-08-lookup-block-index branch from c515391 to 1d1768f Compare August 25, 2017 14:22

promag commented Oct 30, 2017

View reviewed changes

promag mentioned this pull request Oct 30, 2017

net: Add missing lock in ProcessHeadersMessage(...) #11578

Merged

ryanofsky reviewed Nov 2, 2017

View reviewed changes

promag force-pushed the 2017-08-lookup-block-index branch 2 times, most recently from 8337539 to a029cd4 Compare November 3, 2017 15:14

jimpo reviewed Dec 6, 2017

View reviewed changes

promag force-pushed the 2017-08-lookup-block-index branch from a029cd4 to 6d9f555 Compare December 13, 2017 15:36

promag force-pushed the 2017-08-lookup-block-index branch from 6d9f555 to e741b98 Compare December 13, 2017 16:07

LarryRuane mentioned this pull request Mar 23, 2021

Bitcoin 0.17 locking PRs zcash/zcash#5034

Open

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 26, 2021

Assert cs_main is held when accessing mapBlockIndex

aaa5482

zcash: cherry picked from commit 02de6a6 zcash: bitcoin/bitcoin#11041

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 26, 2021

Lock cs_main while loading block index in AppInitMain

da9b866

zcash: cherry picked from commit c651df8 zcash: bitcoin/bitcoin#11041

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 26, 2021

Fix cs_main lock in LoadExternalBlockFile

5829056

When accessing mapBlockIndex cs_main must be held. zcash: cherry picked from commit f814a3e zcash: bitcoin/bitcoin#11041

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 26, 2021

Add missing cs_lock in CreateWalletFromFile

a3e7c0d

zcash: cherry picked from commit 43a32b7 zcash: bitcoin/bitcoin#11041

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

Assert cs_main is held when accessing mapBlockIndex

95fb09c

zcash: cherry picked from commit 02de6a6 zcash: bitcoin/bitcoin#11041

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

Lock cs_main while loading block index in AppInitMain

e9f7ea0

zcash: cherry picked from commit c651df8 zcash: bitcoin/bitcoin#11041

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

Fix cs_main lock in LoadExternalBlockFile

643504c

When accessing mapBlockIndex cs_main must be held. zcash: cherry picked from commit f814a3e zcash: bitcoin/bitcoin#11041

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

Add missing cs_lock in CreateWalletFromFile

297ab4c

zcash: cherry picked from commit 43a32b7 zcash: bitcoin/bitcoin#11041

gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 13, 2022

Merge bitcoin#11041 Add LookupBlockIndex

d5ccf58

Contains both the changes done upstream and changes done in Dash codebase Signed-off-by: pasta <[email protected]>

gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 22, 2022

rpc|wallet: More of bitcoin#11041

e7d403f

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

promag commented Aug 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 14, 2017

Uh oh!

TheBlueMatt commented Aug 14, 2017

Uh oh!

promag commented Aug 15, 2017

Uh oh!

promag commented Aug 22, 2017

Uh oh!

laanwj commented Aug 22, 2017

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 2, 2017

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Dec 6, 2017

Uh oh!

TheBlueMatt commented Dec 12, 2017

Uh oh!

promag commented Dec 13, 2017

Uh oh!

maflcko commented Dec 13, 2017

Uh oh!

rebroad commented May 11, 2021

Uh oh!

promag commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

promag commented Aug 13, 2017 •

edited

Loading