[Net_Processing] Cleaner block processing workflow #2295

furszy · 2021-04-05T13:49:34Z

Grouped several improvements to the block processing workflow. Cleaning redundancies, extra data requests, unneeded pieces of code and adding missing locks to critical sections.

Briefly describing the commits:

Remove the now unneeded, due v5 enforcement, P2PKH flag from CheckBlockSignature.
Move CheckBlockSignature inside CheckBlock.
Remove redundant fAlreadyCheckedBlock and exclusively use the CBlock::fChecked correct flag.
Add missing cs_main lock for CheckBlock function call in ProcessNewBlock.
Remove redundant, and never used, block sync request in ProcessNewBlock (check commit message for the precise description).
Add missing cs_main lock for block sync request in ProcessMessage.

This is part of a chain of PRs over the area that will continue doing, next one will most likely be a bit more invasive but will try to keep it atomic enough due the importance of the area been modified.

It was only needed for the activation period, now that it's fully enforced, can be removed. The activation time isn't needed, v5 enforcement checkpoint is ensuring that the chain cannot reorg to a time in which P2PKH stakes weren't enabled.

…ptBlock` `CBlock` has a member `fChecked` that has the exact same purpose as the `fAlreadyCheckedBlock` flag.

…ain` flow.

ProcessNewBlock is called by ProcessMessage (NetMsgType::BLOCK command) which is doing the exact same check: if the node does not have the previous block, request sync up to it.

furszy · 2021-04-07T14:58:45Z

Conflicts solved, ready for review.
This can part of the new v5.1.0 rc3, it has few easily reviewable fixes.

random-zebra

ACK 21646d8

random-zebra · 2021-04-07T19:58:07Z

src/blockassembler.cpp


        CValidationState state;
-        if (!TestBlockValidity(state, *pblock, pindexPrev, false, false)) {
+        if (!TestBlockValidity(state, *pblock, pindexPrev, false, false, false)) {


The block is already signed at this point. We can check the signature as well (removing the extra argument in TestBlockValidity and always calling CheckBlock with fCheckBlockSig = true).

The reason behind every check flag false is to not do double validations and slowdown the block generation process when the same wallet crafted the block and knows what is inside it + followed every consensus rule creating it.

If the signature fails for any reason, it will fail in the SignBlock function call that is few lines above of this line. The only reason to haveTestBlockValidity::CheckBlock() call failing is because (1) there is a problem in the block generation code that is not following the consensus rules or (2) the user modified the miner sources in some way (which we don't care).

If the signature fails for any reason, it will fail in the SignBlock function call that is few lines above of this line.

Assuming no bugs (or hw failures) in the signature process.
SignBlock only calls CKey::Sign, there is no verification.
CKey::Sign calls secp256k1_ecdsa_sign and secp256k1_ecdsa_signature_serialize_der. It can return false only if the key is not found, or not valid.
In CPubKey::Verify, instead, the signature is verified with secp256k1_ec_pubkey_parse and secp256k1_ecdsa_verify.

Of course, adding the check in TestBlockValidity would make it a bit slower... but maybe it's worth.
I'm fine anyway (that's why have already acked).

random-zebra · 2021-04-07T20:02:47Z

src/validation.cpp

+        // CheckBlock requires cs_main lock
        LOCK(cs_main);
-
-        if (!checked) {
+        if (!CheckBlock(*pblock, state)) {


could add an AssertLockHeld in Checkblock too, since we are at it.

Fuzzbawls

ACK 21646d8

It was only needed for the activation period, now that it's fully enforced, can be removed. The activation time isn't needed, v5 enforcement checkpoint is ensuring that the chain cannot reorg to a time in which P2PKH stakes weren't enabled. Github-Pull: bitcoin#2295 Rebased-From: 5aa3600

Github-Pull: bitcoin#2295 Rebased-From: 47cae28

…ptBlock` `CBlock` has a member `fChecked` that has the exact same purpose as the `fAlreadyCheckedBlock` flag. Github-Pull: bitcoin#2295 Rebased-From: 362a598

…ain` flow. Github-Pull: bitcoin#2295 Rebased-From: 6355774

Github-Pull: bitcoin#2295 Rebased-From: ce1781e

ProcessNewBlock is called by ProcessMessage (NetMsgType::BLOCK command) which is doing the exact same check: if the node does not have the previous block, request sync up to it. Github-Pull: bitcoin#2295 Rebased-From: 959936f

Github-Pull: bitcoin#2295 Rebased-From: 21646d8

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

furszy self-assigned this Apr 5, 2021

furszy added 7 commits April 7, 2021 11:49

Refactor: move CheckBlockSignature function call inside CheckBlock.

47cae28

Refactor: remove redundant fAlreadyCheckedBlock argument from `Acce…

362a598

…ptBlock` `CBlock` has a member `fChecked` that has the exact same purpose as the `fAlreadyCheckedBlock` flag.

Removal of the fAlreadyChecked flag from the entire `ActivateBestCh…

6355774

…ain` flow.

validation: missing cs_main lock for CheckBlock in ProcessNewBlock

ce1781e

validation: Remove redundant request sync from ProcessNewBlock().

959936f

ProcessNewBlock is called by ProcessMessage (NetMsgType::BLOCK command) which is doing the exact same check: if the node does not have the previous block, request sync up to it.

net_processing: missing cs_main lock for chainActive.GetLocator() call

21646d8

furszy force-pushed the 2021_ProcessNewBlock_postV5_updates branch from 49ec3f9 to 21646d8 Compare April 7, 2021 14:55

random-zebra approved these changes Apr 8, 2021

View reviewed changes

random-zebra added Needs Backport Placeholder tag for anything needing a backport to prior version branches Refactoring Validation labels Apr 8, 2021

random-zebra added this to the 5.1.0 milestone Apr 8, 2021

This was referenced Apr 8, 2021

[wallet] Remove now unneeded stake P2PKH->P2PK output forced conversion #2299

Merged

[Backport] 5.1.0 backports #2303

Merged

Fuzzbawls approved these changes Apr 9, 2021

View reviewed changes

furszy merged commit a0f5167 into PIVX-Project:master Apr 9, 2021

furszy referenced this pull request in furszy/bitcoin-core Apr 9, 2021

Refactor: move CheckBlockSignature function call inside CheckBlock.

374f6c9

Github-Pull: bitcoin#2295 Rebased-From: 47cae28

furszy referenced this pull request in furszy/bitcoin-core Apr 9, 2021

Removal of the fAlreadyChecked flag from the entire `ActivateBestCh…

1f38b90

…ain` flow. Github-Pull: bitcoin#2295 Rebased-From: 6355774

furszy referenced this pull request in furszy/bitcoin-core Apr 9, 2021

validation: missing cs_main lock for CheckBlock in ProcessNewBlock

c44b431

Github-Pull: bitcoin#2295 Rebased-From: ce1781e

furszy referenced this pull request in furszy/bitcoin-core Apr 9, 2021

net_processing: missing cs_main lock for chainActive.GetLocator() call

0d2555e

Github-Pull: bitcoin#2295 Rebased-From: 21646d8

Fuzzbawls removed the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Apr 12, 2021

furszy deleted the 2021_ProcessNewBlock_postV5_updates branch November 29, 2022 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Net_Processing] Cleaner block processing workflow #2295

[Net_Processing] Cleaner block processing workflow #2295

Uh oh!

furszy commented Apr 5, 2021

Uh oh!

furszy commented Apr 7, 2021

Uh oh!

random-zebra left a comment

Uh oh!

random-zebra Apr 7, 2021

Uh oh!

furszy Apr 8, 2021

Uh oh!

random-zebra Apr 8, 2021

Uh oh!

random-zebra Apr 7, 2021

Uh oh!

Fuzzbawls left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Net_Processing] Cleaner block processing workflow #2295

[Net_Processing] Cleaner block processing workflow #2295

Uh oh!

Conversation

furszy commented Apr 5, 2021

Uh oh!

furszy commented Apr 7, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

random-zebra Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

furszy Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

random-zebra Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

random-zebra Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants