[Validation][Test] Fix concurrency issues in ActivateBestChain + optimization #2290
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
#2284 merged. This needs rebase |
Ensures ordering of callbacks within a SingleThreadedSchedulerClient with respect to each other
…ading and memory model
Ensures that callbacks are invoked in the order in which the chain is updated Resolves bitcoin#12978
If the ShutdownRequested() check at the top of ActivateBestChain() returns false during initial genesis block load we will fail an assertion in UTXO DB flush as the best block hash IsNull(). To work around this, we move the check until after one round of ActivateBestChainStep(), ensuring the genesis block gets connected.
Technically, some internal datastructures may be in an inconsistent state if we do this, though there are no known bugs there. Still, for future safety, its much better to only unlock cs_main if we've made progress (not just tried a reorg which may make progress).
If multiple threads are invoking ActivateBestChain, it was possible to have them working towards different tips, and we could arrive at a less work tip than we should. Fix this by introducing a ChainState lock which must be held for the entire duration of ActivateBestChain to enforce exclusion in ABC.
…s correct description of "prevblk-not-found"
After a recent bug discovered in callback ordering in MainSignals, this test checks invariants in ordering of BlockConnected / BlockDisconnected / UpdatedChainTip signals Adaptation of btc@dd435ad40267f5c50ff17533c696f9302829a6a6
furszy
force-pushed
the
2021_concurrency_fixes
branch
from
dba80ff
to
50dbec5
Compare
|
rebased. |
Fuzzbawls
approved these changes
Apr 4, 2021
random-zebra
added
the
Needs Backport
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
Ensures ordering of callbacks within a SingleThreadedSchedulerClient with respect to each other Github-Pull: bitcoin#2290 Rebased-From: 4ea2048
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
…he memory model Github-Pull: bitcoin#2290 Rebased-From: cb9bb25
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
…ading and memory model Github-Pull: bitcoin#2290 Rebased-From: f9d2ab3
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
Ensures that callbacks are invoked in the order in which the chain is updated Resolves bitcoin#12978 Github-Pull: bitcoin#2290 Rebased-From: ef24337
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
If the ShutdownRequested() check at the top of ActivateBestChain() returns false during initial genesis block load we will fail an assertion in UTXO DB flush as the best block hash IsNull(). To work around this, we move the check until after one round of ActivateBestChainStep(), ensuring the genesis block gets connected. Github-Pull: bitcoin#2290 Rebased-From: 8640be1
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
Github-Pull: bitcoin#2290 Rebased-From: 8d15cf5
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
Technically, some internal datastructures may be in an inconsistent state if we do this, though there are no known bugs there. Still, for future safety, its much better to only unlock cs_main if we've made progress (not just tried a reorg which may make progress). Github-Pull: bitcoin#2290 Rebased-From: 198f435
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
If multiple threads are invoking ActivateBestChain, it was possible to have them working towards different tips, and we could arrive at a less work tip than we should. Fix this by introducing a ChainState lock which must be held for the entire duration of ActivateBestChain to enforce exclusion in ABC. Github-Pull: bitcoin#2290 Rebased-From: a51a755
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
…s correct description of "prevblk-not-found" Github-Pull: bitcoin#2290 Rebased-From: f68251d
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
After a recent bug discovered in callback ordering in MainSignals, this test checks invariants in ordering of BlockConnected / BlockDisconnected / UpdatedChainTip signals Adaptation of btc@dd435ad40267f5c50ff17533c696f9302829a6a6 Github-Pull: bitcoin#2290 Rebased-From: 50dbec5
Fuzzbawls
added a commit
that referenced
this pull request
Apr 12, 2021
50a5e84 GUI: if the custom fee is disabled, use the minimum required fee and not the stored value. (furszy) 1fc145c Automatically set the lowest possible Custom Fee when user type in fee that is too low. (MishaPozdnikin) 0d2555e net_processing: missing cs_main lock for chainActive.GetLocator() call (furszy) 64bd400 validation: Remove redundant request sync from ProcessNewBlock(). (furszy) c44b431 validation: missing cs_main lock for `CheckBlock` in `ProcessNewBlock` (furszy) 1f38b90 Removal of the `fAlreadyChecked` flag from the entire `ActivateBestChain` flow. (furszy) f53c824 Refactor: remove redundant `fAlreadyCheckedBlock` argument from `AcceptBlock` (furszy) 374f6c9 Refactor: move `CheckBlockSignature` function call inside `CheckBlock`. (furszy) a78dfbe Validation: Remove `CheckBlockSignature` now unneeded enableP2PKH flag. (furszy) 7f244b1 [Tests] Check last_processed_block in getwalletinfo Github-Pull: #2283 Rebased-From: a700fdf (random-zebra) b3a12b5 [RPC] `getwalletinfo`: Add last_processed_block return value. (furszy) e51dcee Add unit tests for signals generated by ProcessNewBlock() (furszy) c4dd07f Validation: rename one of the two instances using "bad-prevblk" to its correct description of "prevblk-not-found" (furszy) 39f6eaf Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen) fb19c3a Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo) fd79bb7 Optimize ActivateBestChain for long chains (Pieter Wuille) c2ae5ff Fix fast-shutdown crash if genesis block was not loaded (Matt Corallo) 7ab7112 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) c202bc1 Update ValidationInterface() documentation to explicitly specify threading and memory model (Jesse Cohen) f499e6e Update documentation for SingleThreadedSchedulerClient() to specify the memory model (Jesse Cohen) 9a9425f Add Unit Test for SingleThreadedSchedulerClient (Jesse Cohen) 4903f31 Removing blocksizenotify abomination. (furszy) c865148 [validation] Do not actively wait for cs_main lock in `ActivateBestChain()` (furszy) a582560 docs: add reduce-memory.md (fanquake) f806584 test: move sync_blocks and sync_mempool functions to test_framework.py (random-zebra) d902128 [Test] Fix intermittent sync_blocks failures (random-zebra) a1d6c0c Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley) 584bbba rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley) e10c1b6 Set fee to highest possible if input is too big #fixes 2234 (dnchk) 775e532 Fixes double fade-in animation when clicking the question mark next to the 'Available' label in the top bar (Volodia) 0cf4064 Make box of PIVX address return to purple when it's empty (Volodia) Pull request description: Backport the following PRs to the 5.1 branch: * #2237 * #2247 * #2249 * #2254 * #2262 * #2284 * #2290 Missing open PRs that need to get merged to move forward with v5.1.0: * [x] #2215 * [x] #2283 * [x] #2295 * [x] #2306 When every PR get merged, we can move forward with a new release candidate for the v5.1.0 release (rc3). Then one or two weeks of testing (depending on how many testers join the efforts) and we are ready for the production release. The heart of v5.1.0 has been already battle tested this past month with the v5.1.0rc2 testing phase. The only remaining point that needs a more broadly usage/testing is #2290 that it's solving a reported issue in the recent release candidate. ACKs for top commit: random-zebra: utACK 50a5e84 Fuzzbawls: ACK 50a5e84 Tree-SHA512: 1f77e1601c51f280b3de0d1de9e0b084871701c1af35ad817760ad55ec28c02e2423494b3d189273aacd367b41ba826d63beb73b0259a1c17f019d05711d54ea
Fuzzbawls
removed
the
Needs Backport
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Made some back ports and adaptations to validate further the work introduced in #2203 and #2209.
Now that the scheduler thread is processing most of the chain events, the callbacks execution order must be verified to dispatch the events serially, ensuring a consistent memory state in each event processing invocation.
There are some concurrency issues solved, as well a an optimization for larger chains for the ABC workflow. Each commit details them well.
List:
bitcoin#7917 (only fb8fad1)
bitcoin#12988
bitcoin#13023
bitcoin#13247
bitcoin#13835
This is built on top of #2284 (and the reason that made me notice that problem). So, 2284 comes first, then this one.