[validation] Removing unneeded block serialization and signal from ABC #2284
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
Essentially, we were re-serializing every new connected block tip just to calculate the size and broadcast a notification for external listeners if the block size is above 1mb. Which well.. (1) the block arrives serialized from the network and it's parsed into a block object by the core, there is no need to re-serialize the entire block.. (2) the complete serialization is just done to notify external listeners if a block exceeds the 1mb block size.. --> which can easily be done from outside of the core without having the most important node processing thread's work notifying nor calculating it, blocking cs_main for its whole time..
random-zebra
added
Needs Backport
Fuzzbawls
approved these changes
Apr 3, 2021
random-zebra
added a commit
that referenced
this pull request
Apr 4, 2021
…tChain + optimization 50dbec5 Add unit tests for signals generated by ProcessNewBlock() (furszy) f68251d Validation: rename one of the two instances using "bad-prevblk" to its correct description of "prevblk-not-found" (furszy) a51a755 Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen) 198f435 Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo) 8d15cf5 Optimize ActivateBestChain for long chains (Pieter Wuille) 8640be1 Fix fast-shutdown crash if genesis block was not loaded (Matt Corallo) ef24337 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) f9d2ab3 Update ValidationInterface() documentation to explicitly specify threading and memory model (Jesse Cohen) cb9bb25 Update documentation for SingleThreadedSchedulerClient() to specify the memory model (Jesse Cohen) 4ea2048 Add Unit Test for SingleThreadedSchedulerClient (Jesse Cohen) Pull request description: 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. ACKs for top commit: Fuzzbawls: ACK 50dbec5 random-zebra: ACK 50dbec5 and merging... Tree-SHA512: 573144cc96d2caa49ed2f0887dc8c53b5aca0efd54b6a25626063ccb780da426f56b3c6b7491648e7abefc1960c82b1354a4ff2c51425bfe99adaa4394562608
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
…ain()` Github-Pull: bitcoin#2284 Rebased-From: e560afd
furszy
referenced
this pull request
in furszy/bitcoin-core
Apr 8, 2021
Essentially, we were re-serializing every new connected block tip just to calculate the size and broadcast a notification for external listeners if the block size is above 1mb. Which well.. (1) the block arrives serialized from the network and it's parsed into a block object by the core, there is no need to re-serialize the entire block.. (2) the complete serialization is just done to notify external listeners if a block exceeds the 1mb block size.. --> which can easily be done from outside of the core without having the most important node processing thread's work notifying nor calculating it, blocking cs_main for its whole time.. Github-Pull: bitcoin#2284 Rebased-From: 479d065
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
random-zebra
removed
the
Needs Release Notes
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.
Essentially, we are re-serializing every new connected block tip, locking
cs_mainfor its whole time, just to calculate the block size and broadcast a notification for external listeners if the block size is above 1mb..Which well.. can easily be done from outside of the core, listening the new block arrival signal without having the most important node's processing thread calculating it, blocking
cs_mainfor its whole time.Plus, block arrive serialized from the network and from disk, there was no need to waste resources forcing a re-serialization for this neither.
What this fix means:
The synchronization process will be faster. So, this can easily get into v5.1.0 release.