[consensus] skip bip30 checks when assumevalid is set for the block

pstratem commented at 5:39 pm on July 29, 2019: contributor

This eliminates the BIP30 checks for blocks before the BIP34 activation.

Those checks consist of one LevelDB Get call for each transaction in the block (34,846,734 in total).

Replace fScriptChecks in ConnectBlock with fAssumeValid.

We need this flag to handle masking the BIP34/BIP30 logic.

1fdcbf40c2

Do not enforce BIP30 if the block is assumed to be valid.

This eliminates the related disk access.

071fd86f0a

MarcoFalke commented at 5:51 pm on July 29, 2019: member

Haven’t reviewed closely, but it would be nice how much of a speed up this gives before deciding whether to review.

Also, you’d want to update the comment it is applied to all blocks except the // two in the chain, as the check is applied to no block (in the assumevalid chain) after your changes.

in src/validation.cpp:1823 in 071fd86f0a

1818@@ -1819,6 +1819,8 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
1819     CBlockIndex *pindexBIP34height = pindex->pprev->GetAncestor(chainparams.GetConsensus().BIP34Height);
1820     //Only continue to enforce if we're below BIP34 activation height or the block hash at that height doesn't correspond.
1821     fEnforceBIP30 = fEnforceBIP30 && (!pindexBIP34height || !(pindexBIP34height->GetBlockHash() == chainparams.GetConsensus().BIP34Hash));
1822+    // And we're not assuming the block is valid.
1823+    fEnforceBIP30 = fEnforceBIP30 && !fAssumeValid;

sdaftuar commented at 5:51 pm on July 29, 2019:

If the assumevalid block (which is command-line specifiable) is not on the chain that builds off the BIP34Hash, then I think not enforcing BIP 30 would be a bug that could prevent us from reorging to the honest chain.

pstratem commented at 7:33 pm on July 30, 2019:

Yes, but in the exact same way that an assumevalid chain with invalid scripts would cause that today.

I don’t see a incentives issue, as violating bip30 really only allows you to destroy coins.

Am I missing something?

sdaftuar commented at 8:13 pm on July 30, 2019:

Currently, if a bad assumevalid hash was given, we could still reorg to the honest chain if it had more work and we found a peer advertising it. The way this code is written would preclude that if an assumevalid hash was given for a chain where bip 30 was violated, which can prevent us from correctly reorging.

MarcoFalke commented at 3:24 pm on July 31, 2019:

From IRC:

[08:29] Anyway this would be a simple fix to your PR — just ensure that you only skip the bip30 checks if assume valid is set and the assume valid block hash builds on the known bip34 activation block hash; that would ensure that we only skip the bip30 checks on blocks we know to be safe from this issue

sdaftuar changes_requested

sdaftuar commented at 6:01 pm on July 29, 2019: member

This area of the code is complicated enough (see the extremely long comments around the BIP34/BIP30 interaction) that I am not sure it’s worth the review time and added complexity unless the performance gain is substantial, so I’d also like to see those numbers.

DrahtBot added the label Validation on Jul 29, 2019

DrahtBot commented at 9:30 pm on July 29, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16658 ([WIP] validation: Rename CheckInputs to CheckInputScripts by jnewbery)
#13868 (Remove unused fScriptChecks parameter from CheckInputs by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake added the label Needs Conceptual Review on Jul 29, 2019

fanquake commented at 3:53 am on July 30, 2019: member

@jamesob Could you run bitcoin perf over this PR?

fanquake commented at 6:22 am on July 30, 2019: member

I’ve done a single, simple (maybe too simple) benchmark of running time src/bitcoind -stopatheight=227931 for master (33894612c0de953b75b41dbfcc643986e4ac177e) and this PR. Starting with an empty data dir on each run, except for a bitcoin.conf which contained par=8 and dbcache=2048. Master took 16m27s and this PR took 16m43s.

pstratem commented at 5:47 pm on July 30, 2019: contributor

time ./src/bitcoind -reindex-chainstate -stopatheight=227931 -dbcache=1024

this pr: 3m25.503s master: 3m47.508s

ie 10% faster

sdaftuar commented at 8:14 pm on July 30, 2019: member

Since that quantity (22 seconds) doesn’t scale as the chain grows, this performance benefit doesn’t seem worth it to me (even though I think this change could be made to be correct, this is complex enough that we should have a higher bar for making this kind of change).

fanquake commented at 6:56 am on August 2, 2019: member

Running time ./src/bitcoind -reindex-chainstate -stopatheight=227931 -dbcache=1024 I do see the speedup:

master e653eeff7651d823407e2e31a89176cc0b240c62: 6m8s this PR: 5m48s

jamesob commented at 1:35 pm on August 2, 2019: member

-0 on this since it doesn’t speed up the non-trivial parts of IBD and introduces more varied behavior to consensus-critical code. Benching the first 200k blocks is deceptive since that’s such a small part of total IBD time. Benching a more meaningful part of the chain (almost by definition) shows no improvement:

ibd local range 500000 501500

fanquake commented at 5:25 am on August 5, 2019: member

I think I’m in agreement with @sdaftuar and @jamesob, even though this may bring a slight speedup to a small portion of IBD. @laanwj Can you weigh in here? @sdaftuar

(even though I think this change could be made to be correct, this is complex enough that we should have a higher bar for making this kind of change).

I’d be interested in hearing more of your thoughts on this. i.e what do you think that bar looks like? If this change was a 30% speedup would that be acceptable? 50%? Or would you only consider optimizations that would affect all of IBD etc.

DrahtBot commented at 9:48 am on September 2, 2019: member

DrahtBot added the label Needs rebase on Sep 2, 2019

fanquake commented at 2:15 am on February 25, 2020: member

No further discussion, and any potential performance improvement doesn’t seem to be worth the increased complexity in validation code. Going to close this for now.

fanquake closed this on Feb 25, 2020

DrahtBot locked this on Feb 15, 2022

[consensus] skip bip30 checks when assumevalid is set for the block #16486

Conflicts