Double check all block rules in `ConnectBlock`, not only `CheckBlock`

darosior commented at 8:12 pm on February 3, 2025: member

We currently only sanity check CheckBlock() in ConnectBlock(). Not running the rest of the block checks makes it hard to reason about upgrade edge cases if we were to introduce new consensus rules (which would necessarily be context-dependent checks). For instance a timewarp fix or a BIP34 supplementation.

This is fine to do as the expensive check in ContextualCheckBlock() is cached since 1ec6bbeb8d27d31647d1433ccb87b362f6d81f90 and the future-timestamp check is split out of ContextualCheckBlockHeader() beforehand.

update comment on MinimumChainWork check c59cbba518

Remove checkpoints

The headers presync logic should be enough to prevent memory DoS using
low-work headers. Therefore, we no longer have any use for checkpoints.

301017c621

DrahtBot commented at 8:12 pm on February 3, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31792.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	TheCharlatan
Approach NACK	l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31714 (validation: Do less work in NeedsRedownload by mzumsande)
#31564 (Add checkblock RPC and checkBlock() to Mining interface by Sjors)
#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#30595 (kernel: Introduce initial C header API by TheCharlatan)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

DrahtBot added the label CI failed on Feb 3, 2025

Armss9936 approved

in src/validation.cpp:4201 in cddc0e2a0f outdated

4193@@ -4194,6 +4194,15 @@ arith_uint256 CalculateClaimedHeadersWork(std::span<const CBlockHeader> headers)
4194     return total_work;
4195 }
4196 
4197+/** Check the timestamp of the block header is not too far in the future. **/
4198+static bool CheckHeaderNotFuture(const CBlockHeader& block, BlockValidationState& state)

TheCharlatan commented at 11:15 am on February 4, 2025:

In commit cddc0e2a0f2b8f19ef243edc8a85e206c31f9e62: Can you add some context to the commit message for why this move is being done? Maybe: “A call to ContextualCheckBlockHeader is added to ConnectBlock in the following commits, but it should not re-check the timestamp of the header there. Move the timestamp check into a separate function to avoid this.”

darosior commented at 3:05 pm on February 4, 2025:

Done.

in src/validation.cpp:4333 in cddc0e2a0f outdated

4348@@ -4345,6 +4349,10 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
4349             LogDebug(BCLog::VALIDATION, "%s: Consensus::CheckBlockHeader: %s, %s\n", __func__, hash.ToString(), state.ToString());
4350             return false;
4351         }
4352+        if (!CheckHeaderNotFuture(block, state)) {
4353+            LogDebug(BCLog::VALIDATION, "%s: Consensus::CheckHeaderNotFuture: %s, %s\n", __func__, hash.ToString(), state.ToString());

TheCharlatan commented at 11:15 am on February 4, 2025:

Why is this not LogError?

darosior commented at 3:21 pm on February 4, 2025:

To keep the existing level for this error. I suppose the existing level is debug due to log-filling concerns, maybe with very-low-difficulty headers?

Thanks for pointing this out though, i double checked the other log line in TestBlockValidity and there it should have been LogError. Fixed.

Sjors commented at 9:46 am on February 6, 2025:

Someone else making an invalid block is not a reason for our node to shut down, so LogError would be wrong.

We also run this before the actual proof-of-work check, and headers can be sent repeatedly, so need to be careful about log spam.

You can however drop __func__, because we have -logsourcelocations.

TheCharlatan commented at 11:17 am on February 4, 2025: contributor

Concept ACK

in test/functional/rpc_blockchain.py:272 in 1cf2721d2f outdated

276@@ -277,6 +277,9 @@ def _test_getdeploymentinfo(self):
277         # calling with an explicit hash works
278         self.check_signalling_deploymentinfo_result(self.nodes[0].getdeploymentinfo(gbci207["bestblockhash"]), gbci207["blocks"], gbci207["bestblockhash"], "started")
279 
280+        # restart clean with default activation heights
281+        self.restart_node(0)

ajtowns commented at 2:40 pm on February 4, 2025:

~~Should this line be in the following commit?~~

No, it shouldn’t.

darosior commented at 3:27 pm on February 4, 2025:

No, it’s to make this commit pass the test on its own. This is because this sub-test restart the node with a different deployment height for Segwit, and its caller then calls verifychain. This would fail if we don’t restart the node with the default deployment height because ConnectBlock would detect unexpected witnesses before activation height. This is essentially what the next commit checks explicitly.

ajtowns commented at 3:27 pm on February 4, 2025: contributor

I think what you’re doing here is:

redoing the contextual checks in ConnectBlock
currently, this could result in errors on startup if we accepted a block with timestamp T, but then our clock went backwards and is now set to a time prior to T-2 hours
so to avoid that, we move the T+2h check out of the contextual checks into its own special function, and call that from the appropriate places
once we redo contextual checks in ConnectBlock, verifychain rpc can detect contexual errors, so check that it does (should also check -reindex-chainstate sees them too?)

I think it would be better to just add declarations for the functions rather than shuffling the code around, and I would have separated the commits a bit differently, more like https://github.com/ajtowns/bitcoin/commits/202502-pr31792/ (except with explanations in the commit messages)

I’m not sure what the behaviour is now vs with this patch in the situation where you’re node has followed a high work chain that is no longer valid under a newer version’s rules. I think so long as the block that violates the rule is more than 6 blocks behind the tip, that both will just continue following the invalid chain until you manually -reindex. Will running verifychain with this PR just cause your node to crash rather than to reorg? It might be good to replace this comment:

0    // NOTE: We don't currently (re-)invoke ContextualCheckBlock() or
1    // ContextualCheckBlockHeader() here. This means that if we add a new
2    // consensus rule that is enforced in one of those two functions, then we
3    // may have let in a block that violates the rule prior to updating the
4    // software, and we would NOT be enforcing the rule here. Fully solving
5    // upgrade from one software version to the next after a consensus rule
6    // change is potentially tricky and issue-specific (see NeedsRedownload()
7    // for one approach that was used for BIP 141 deployment).

with the actual post-this-PR upgrade-behaviour, rather than just deleting it?

darosior force-pushed on Feb 4, 2025

mzumsande commented at 4:45 pm on February 4, 2025: contributor

This is fine to do as the expensive check in ContextualCheckBlock() is cached since https://github.com/bitcoin/bitcoin/commit/1ec6bbeb8d27d31647d1433ccb87b362f6d81f90

As discussed out of band, only the most recent block will be cached in memory and passed to ActivateBestChain / ConnectTip. During IBD when blocks arrive out of order ABC will frequently connect multiple blocks, which then need to be reloaded from disk during ConnectTip() . For these, the CheckWitnessMalleation check will be done twice after this PR, so there is a possible performance impact on IBD.

willcl-ark commented at 7:42 pm on February 4, 2025: member

I seem to have a problem running IBD with this change @ dd2714ed13eea3591162dbade8132ff517ca5509, which I have not investigated yet:

  0<snip>
  12025-02-04T19:38:24Z init message: Verifying blocks…
  22025-02-04T19:38:24Z Block index and chainstate loaded
  32025-02-04T19:38:24Z Setting NODE_NETWORK on non-prune mode
  42025-02-04T19:38:24Z initload thread start
  52025-02-04T19:38:24Z [bench]   - Load block from disk: 0.05ms
  62025-02-04T19:38:24Z [validation] BlockChecked: block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f state=Valid
  72025-02-04T19:38:24Z [bench]   - Connect total: 0.05ms [0.00s (0.05ms/blk)]
  82025-02-04T19:38:24Z [bench]   - Flush: 0.02ms [0.00s (0.02ms/blk)]
  92025-02-04T19:38:24Z [bench]   - Writing chainstate: 0.01ms [0.00s (0.01ms/blk)]
 102025-02-04T19:38:24Z [validation] Enqueuing MempoolTransactionsRemovedForBlock: block height=0 txs removed=0
 112025-02-04T19:38:24Z UpdateTip: new best=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f height=0 version=0x00000001 log2_work=32.000022 tx=1 date='2009-01-03T18:15:05Z' progress=0.000000 cache=0.3MiB(0txo)
 122025-02-04T19:38:24Z [bench]   - Connect postprocess: 0.06ms [0.00s (0.06ms/blk)]
 132025-02-04T19:38:24Z [bench] - Connect block: 0.19ms [0.00s (0.19ms/blk)]
 142025-02-04T19:38:24Z [validation] Enqueuing BlockConnected: block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f block height=0
 152025-02-04T19:38:24Z [validation] Enqueuing UpdatedBlockTip: new block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f fork block hash=null (in IBD=true)
 162025-02-04T19:38:24Z [validation] ActiveTipChange: new block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f block height=0
 172025-02-04T19:38:24Z [validation] MempoolTransactionsRemovedForBlock: block height=0 txs removed=0
 182025-02-04T19:38:24Z block tree size = 1
 192025-02-04T19:38:24Z nBestHeight = 0
 202025-02-04T19:38:24Z [validation] BlockConnected: block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f block height=0
 212025-02-04T19:38:24Z init message: Starting network threads…
 222025-02-04T19:38:24Z Failed to open mempool file. Continuing anyway.
 232025-02-04T19:38:24Z initload thread exit
 242025-02-04T19:38:24Z DNS seeding disabled
 252025-02-04T19:38:24Z net thread start
 262025-02-04T19:38:24Z init message: Done loading
 272025-02-04T19:38:24Z addcon thread start
 282025-02-04T19:38:24Z msghand thread start
 292025-02-04T19:38:24Z opencon thread start
 302025-02-04T19:38:24Z [validation] UpdatedBlockTip: new block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f fork block hash=null (in IBD=true)
 312025-02-04T19:38:24Z New manual v2 peer connected: version: 70016, blocks=882316, peer=0
 322025-02-04T19:38:24Z Pre-synchronizing blockheaders, height: 2000 (~0.24%)
 332025-02-04T19:38:25Z Pre-synchronizing blockheaders, height: 18000 (~2.14%)
 34
 35<snip>
 36
 372025-02-04T19:38:43Z [validation] Saw new header hash=00000000000000000002f0aba7c60667df80ee657873bfa22e5dba80f13fafcb height=859992
 382025-02-04T19:38:43Z [validation] Saw new header hash=00000000000000000002e66a5fe60350d7a5900c797dde1ac989e36de05e2cf3 height=859993
 392025-02-04T19:38:43Z [validation] Saw new header hash=0000000000000000000220ed045522462a2a96bc3e3898364f19dad67fe842a8 height=859994
 402025-02-04T19:38:43Z [validation] Saw new header hash=0000000000000000000124d178d6056cf493873ba3a865eb2a4fc64e2e9b2ccb height=859995
 412025-02-04T19:38:43Z [validation] Saw new header hash=00000000000000000000bc91fb6007041636f724770df4d16184af479102c69e height=859996
 422025-02-04T19:38:43Z [validation] Saw new header hash=000000000000000000028c2e4e0e6e83da708aabf189b252e5f8f62cbd090053 height=859997
 432025-02-04T19:38:43Z [validation] Saw new header hash=00000000000000000000de531aab72aa763d8f1dea92efdb47c96ea5d209b8ef height=859998
 442025-02-04T19:38:43Z [validation] Saw new header hash=0000000000000000000196a7111c7aa3368af5a803a81c7bfb32860100dfd9c7 height=859999
 452025-02-04T19:38:43Z [validation] Saw new header hash=0000000000000000000095dd5c0c8e176a6498eb335c491b96df1a1ae178bfbd height=860000
 462025-02-04T19:38:43Z Synchronizing blockheaders, height: 860000 (~97.51%)
 472025-02-04T19:31:58Z [bench]   - Using cached block
 482025-02-04T19:31:58Z [bench]   - Load block from disk: 0.01ms
 492025-02-04T19:31:58Z ERROR: ContextualCheckBlockHeader: forked chain older than last checkpoint (height 1)
 502025-02-04T19:31:58Z [error] ConnectBlock: Consensus::ContextualCheckBlockHeader: bad-fork-prior-to-checkpoint
 512025-02-04T19:31:58Z [validation] BlockChecked: block hash=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048 state=bad-fork-prior-to-checkpoint
 522025-02-04T19:31:58Z InvalidChainFound: invalid block=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048  height=1  log2_work=33.000022  date=2009-01-09T02:54:25Z
 532025-02-04T19:31:58Z InvalidChainFound:  current best=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f  height=0  log2_work=32.000022  date=2009-01-03T18:15:05Z
 542025-02-04T19:31:58Z [error] ConnectTip: ConnectBlock 00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048 failed, bad-fork-prior-to-checkpoint
 552025-02-04T19:31:58Z InvalidChainFound: invalid block=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048  height=1  log2_work=33.000022  date=2009-01-09T02:54:25Z
 562025-02-04T19:31:58Z InvalidChainFound:  current best=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f  height=0  log2_work=32.000022  date=2009-01-03T18:15:05Z
 572025-02-04T19:31:58Z [validation] ActiveTipChange: new block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f block height=0
 582025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 592025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 000000006a625f06636b8bb6ac7b960a8d03705d1ace08b1a19da3fdcc99ddbd is marked invalid
 602025-02-04T19:31:58Z [validation] BlockChecked: block hash=000000006a625f06636b8bb6ac7b960a8d03705d1ace08b1a19da3fdcc99ddbd state=duplicate-invalid
 612025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 622025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 632025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 0000000082b5015589a3fdf2d4baff403e6f0be035a5d9742c1cae6295464449 is marked invalid
 642025-02-04T19:31:58Z [validation] BlockChecked: block hash=0000000082b5015589a3fdf2d4baff403e6f0be035a5d9742c1cae6295464449 state=duplicate-invalid
 652025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 662025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 672025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 000000004ebadb55ee9096c9a2f8880e09da59c0d68b1c228da88e48844a1485 is marked invalid
 682025-02-04T19:31:58Z [validation] BlockChecked: block hash=000000004ebadb55ee9096c9a2f8880e09da59c0d68b1c228da88e48844a1485 state=duplicate-invalid
 692025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 702025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 712025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 000000009b7262315dbf071787ad3656097b892abffd1f95a1a022f896f533fc is marked invalid
 722025-02-04T19:31:58Z [validation] BlockChecked: block hash=000000009b7262315dbf071787ad3656097b892abffd1f95a1a022f896f533fc state=duplicate-invalid
 732025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 742025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 752025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 000000003031a0e73735690c5a1ff2a4be82553b2a12b776fbd3a215dc8f778d is marked invalid
 762025-02-04T19:31:58Z [validation] BlockChecked: block hash=000000003031a0e73735690c5a1ff2a4be82553b2a12b776fbd3a215dc8f778d state=duplicate-invalid
 772025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 782025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 792025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 0000000071966c2b1d065fd446b1e485b2c9d9594acd2007ccbd5441cfc89444 is marked invalid
 802025-02-04T19:31:58Z [validation] BlockChecked: block hash=0000000071966c2b1d065fd446b1e485b2c9d9594acd2007ccbd5441cfc89444 state=duplicate-invalid
 812025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 822025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 832025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 00000000408c48f847aa786c2268fc3e6ec2af68e8468a34a28c61b7f1de0dc6 is marked invalid
 842025-02-04T19:31:58Z [validation] BlockChecked: block hash=00000000408c48f847aa786c2268fc3e6ec2af68e8468a34a28c61b7f1de0dc6 state=duplicate-invalid
 852025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 862025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 872025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 000000008d9dc510f23c2657fc4f67bea30078cc05a90eb89e84cc475c080805 is marked invalid
 882025-02-04T19:31:58Z [validation] BlockChecked: block hash=000000008d9dc510f23c2657fc4f67bea30078cc05a90eb89e84cc475c080805 state=duplicate-invalid
 892025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 902025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 912025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 000000002c05cc2e78923c34df87fd108b22221ac6076c18f3ade378a4d915e9 is marked invalid
 922025-02-04T19:31:58Z [validation] BlockChecked: block hash=000000002c05cc2e78923c34df87fd108b22221ac6076c18f3ade378a4d915e9 state=duplicate-invalid
 932025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 942025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 952025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 0000000097be56d606cdd9c54b04d4747e957d3608abe69198c661f2add73073 is marked invalid
 962025-02-04T19:31:58Z [validation] BlockChecked: block hash=0000000097be56d606cdd9c54b04d4747e957d3608abe69198c661f2add73073 state=duplicate-invalid
 972025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
 982025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
 992025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 0000000027c2488e2510d1acf4369787784fa20ee084c258b58d9fbd43802b5e is marked invalid
1002025-02-04T19:31:58Z [validation] BlockChecked: block hash=0000000027c2488e2510d1acf4369787784fa20ee084c258b58d9fbd43802b5e state=duplicate-invalid
1012025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
1022025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
1032025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 000000005c51de2031a895adc145ee2242e919a01c6d61fb222a54a54b4d3089 is marked invalid
1042025-02-04T19:31:58Z [validation] BlockChecked: block hash=000000005c51de2031a895adc145ee2242e919a01c6d61fb222a54a54b4d3089 state=duplicate-invalid
1052025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
1062025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
1072025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 0000000080f17a0c5a67f663a9bc9969eb37e81666d9321125f0e293656f8a37 is marked invalid
1082025-02-04T19:31:58Z [validation] BlockChecked: block hash=0000000080f17a0c5a67f663a9bc9969eb37e81666d9321125f0e293656f8a37 state=duplicate-invalid
1092025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
1102025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
1112025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 00000000b3322c8c3ef7d2cf6da009a776e6a99ee65ec5a32f3f345712238473 is marked invalid
1122025-02-04T19:31:58Z [validation] BlockChecked: block hash=00000000b3322c8c3ef7d2cf6da009a776e6a99ee65ec5a32f3f345712238473 state=duplicate-invalid
1132025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
1142025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
1152025-02-04T19:31:58Z [validation] AcceptBlockHeader: block 00000000174a25bb399b009cc8deff1c4b3ea84df7e93affaaf60dc3416cc4f5 is marked invalid
1162025-02-04T19:31:58Z [validation] BlockChecked: block hash=00000000174a25bb399b009cc8deff1c4b3ea84df7e93affaaf60dc3416cc4f5 state=duplicate-invalid
1172025-02-04T19:31:58Z [error] ProcessNewBlock: AcceptBlock FAILED (duplicate-invalid)
1182025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
1192025-02-04T19:31:58Z [validation] header 00000000000000000001b3fc0c62b55c57a0d46b4069100ab041af9c345a1059 has prev block invalid: 0000000000000000000095dd5c0c8e176a6498eb335c491b96df1a1ae178bfbd
1202025-02-04T19:31:58Z Warning: not punishing manually connected peer 0!
121^C2025-02-04T19:32:03Z Shutdown: In progress...
1222025-02-04T19:32:03Z opencon thread exit
1232025-02-04T19:32:03Z addcon thread exit
1242025-02-04T19:32:03Z net thread exit
1252025-02-04T19:32:03Z msghand thread exit
1262025-02-04T19:32:03Z scheduler thread exit
1272025-02-04T19:32:03Z Writing 0 mempool transactions to file...
1282025-02-04T19:32:03Z Writing 0 unbroadcast transactions to file.
1292025-02-04T19:32:03Z Dumped mempool: 0.000s to copy, 0.006s to dump, 27 bytes dumped to file
1302025-02-04T19:32:03Z Flushed fee estimates to fee_estimates.dat.
1312025-02-04T19:32:03Z [bench] FlushStateToDisk: write block and undo data to disk started
1322025-02-04T19:32:03Z [bench] FlushStateToDisk: write block and undo data to disk completed (16.65ms)
1332025-02-04T19:32:03Z [bench] FlushStateToDisk: write block index to disk started
1342025-02-04T19:32:05Z [bench] FlushStateToDisk: write block index to disk completed (1348.48ms)
1352025-02-04T19:32:05Z [bench] FlushStateToDisk: write coins cache to disk (0 coins, 256KiB) started
1362025-02-04T19:32:05Z [bench] FlushStateToDisk: write coins cache to disk (0 coins, 256KiB) completed (0.03ms)
1372025-02-04T19:32:05Z [validation] Enqueuing ChainStateFlushed: block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f
1382025-02-04T19:32:05Z [validation] ChainStateFlushed: block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f
1392025-02-04T19:32:05Z [bench] FlushStateToDisk: write block and undo data to disk started
1402025-02-04T19:32:05Z [bench] FlushStateToDisk: write block and undo data to disk completed (27.61ms)
1412025-02-04T19:32:05Z [bench] FlushStateToDisk: write block index to disk started
1422025-02-04T19:32:05Z [bench] FlushStateToDisk: write block index to disk completed (5.74ms)
1432025-02-04T19:32:05Z [bench] FlushStateToDisk: write coins cache to disk (0 coins, 256KiB) started
1442025-02-04T19:32:05Z [bench] FlushStateToDisk: write coins cache to disk (0 coins, 256KiB) completed (0.03ms)
1452025-02-04T19:32:05Z [validation] Enqueuing ChainStateFlushed: block hash=000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f
1462025-02-04T19:32:05Z Shutdown: done

I connected to a single manual peer which is why Warning: not punishing manually connected peer 0! appears. With a selection of network peers I observed an endless header presync->sync->presync loop.

DrahtBot commented at 11:05 pm on February 4, 2025: contributor

feature_unsupported_utxo_db.py fails (times out)

darosior force-pushed on Feb 5, 2025

darosior commented at 4:13 am on February 5, 2025: member

I seem to have a problem running IBD with this change @ dd2714e, which I have not investigated yet:

This is because we’d now perform the checkpoints check twice, but it’d fail the second time since a checkpoint with a higher height would already be in the index. Instead of introducing more complexity there i rebased on top of #31649.

feature_unsupported_utxo_db.py fails (times out)

This was failing in the final restart of the node, because the upgraded node is now checking all consensus rules (including Segwit which is always active on testnet) and was tripping on a bad-witness-nonce-size since the block was created by the unupgraded node.

DrahtBot removed the label CI failed on Feb 5, 2025

[refactor] validation: split out future timestamp check

In the following commits we'll make it so all consensus rules are checked when
connecting blocks. However we should not check again there that the block
timestamp is not more than 2 hours in the future, just in case our system clock
went backward.

To achieve this we first split out the future timestamp check in its own function.

0dad68ec06

[refactor]: move future timestamp check out of CtxCheckBlockHeader

We want to check all consensus rules when connecting a block, and therefore to
call CtxCheckBlockHeader in ConnectBlock. But we don't want to perform the future
timestamp check there again in case our system clock went backware. Therefore move
the future timestamp check out of CtxCheckBlockHeader to its callers.

9655edaec3

validation: (re-)check all consensus rules when connecting blocks

At the moment we only sanity check part of the consensus rules when connecting blocks (only
CheckBlock() is called in ConnectBlock()). This makes it hard to reason about software upgrades
if we were to introduce new consensus rules. For instance a timewarp fix or a BIP34 supplementation
would go into CtxCheckBlock{Header}, and a block invalid according to the new rules could be let in
by an unupgraded version and still be connected by an upgraded one.

These additional checks should not noticeably increase block propagation at tip since their expensive
part (the witness merkle root check) has been cached since 1ec6bbeb8d27d31647d1433ccb87b362f6d81f90.
They would however have a performance impact on IBD as the cache is not leveraged there.

ee05eb4403

qa: sanity check ConnectBlock now checks all rules ece9ac177d

darosior force-pushed on Feb 5, 2025

darosior commented at 8:21 pm on February 5, 2025: member

(should also check -reindex-chainstate sees them too?)

I tried to add a functional test for this but our test framework assumes an RPC interface is always available, and this error would happen before it is. This would make the test code really clunky, plus verifychain tests the same already and feature_unsupported_utxo_db.py implicitly tests this (not setting -testactivationheight=segwit@2 with -rescan-chainstate would trigger a ConnectBlock error).

I think it would be better to just add declarations for the functions rather than shuffling the code around, and I would have separated the commits a bit differently

I have now re-arranged the commits as you suggested (essentially just splitting the first commit in two), and used forward declarations instead of moving the function definitions. I also added more details to the commit, notably Martin’s point that this change would probably have an impact on IBD.

It might be good to replace this comment […] with the actual post-this-PR upgrade-behaviour, rather than just deleting it?

I couldn’t come up with any useful comment to replace it with. The original comment is a warning about the surprising original behavior. Now that the surprising behavior is removed, it makes sense to just delete the warning and not replace it with “this does what you’d expect”?

I started an IBD on top of this branch. EDIT: IBD completed successfully.

ajtowns commented at 5:05 am on February 6, 2025: contributor

It might be good to replace this comment […] with the actual post-this-PR upgrade-behaviour, rather than just deleting it?

I couldn’t come up with any useful comment to replace it with. The original comment is a warning about the surprising original behavior. Now that the surprising behavior is removed, it makes sense to just delete the warning and not replace it with “this does what you’d expect”?

I guess it’s not obvious to me what you should expect here.

I believe the current high-level behaviour with the PR is:

Running normally will continue from the current tip as if it were valid, even if it strictly isn’t, you’ll reorg to a valid chain automatically if it ever has more work. If you run invalidateblock on the first invalid block, you’ll reorg back to that point and continue on to a valid chain, if one exists.
Running with -checklevel=4 will detect errors within the last 6 blocks (-checkblocks), and cause startup failure
Running with -reindex will detect errors anywhere, and leave you at the last valid block
Running with -reindex-chainstate will leave you stuck in an infinite loop revalidating an invalid tip?

As far as the low-level behaviour goes, eg, it’s not clear to me why TestBlockValidity() needs to duplicate the checks (previously, it would have made some sense in order to ensure CheckBlock errors are reported prior to ContextualCheckBlock errors, but it seems less useful now).

I guess it might make more sense to add a docstring for ConnectBlock that it (re)validates the block its connecting (and to what degree it does so), and move the “Check it again in case a previous version” comment to VerifyDB saying that this will catch errors in the examined blocks, and that invalidateblock, a -reindex or special handling like for segwit is necessary if you want to force a reorg onto an valid chain?

Sjors commented at 9:15 am on February 6, 2025: member

Can you mention in the PR description that this builds on #31649? That also helps prevent review comments on the checkpoint change from being spread between two pull requests.

The first two [refactor] commits 0dad68ec06af4c53bf4a3cd6c266e8cb8fe5ef55 and 9655edaec317f0e8aa14ff7feab68ec668a86076 look correct to me.

Although the CheckHeaderNotFuture is now performed earlier than before, it’s cheap enough that this doesn’t seem dangerous. The tests that it moves ahead of are similarly cheap.

Technically 9655edaec317f0e8aa14ff7feab68ec668a86076 should update this comment in ConnectBlock():

0// Also, currently the rule against blocks more than 2 hours in the future
1// is enforced in ContextualCheckBlockHeader(); we wouldn't want to

But you’re already doing that in the next commit. It might just be confusing to update the comment twice.

Will review the actual change of this PR later.

in src/validation.cpp:4671 in 9655edaec3 outdated

4666@@ -4669,6 +4667,10 @@ bool TestBlockValidity(BlockValidationState& state,
4667     indexDummy.phashBlock = &block_hash;
4668 
4669     // NOTE: CheckBlockHeader is called by CheckBlock
4670+    if (!CheckHeaderNotFuture(block, state)) {
4671+        LogError("%s: Consensus::CheckHeaderNotFuture: %s\n", __func__, state.ToString());

Sjors commented at 9:58 am on February 6, 2025:

9655edaec317f0e8aa14ff7feab68ec668a86076 nit: you can drop __func__.

The use of LogError here is fine; it’s consistent with the other checks, failure here implies our mining subsystem is broken, and this code isn’t accessible via p2p.

It also reminded me that I need to preserve this logging behavior in #31564 (review)

in src/validation.cpp:2461 in ee05eb4403 outdated

2459-    // consensus rule that is enforced in one of those two functions, then we
2460-    // may have let in a block that violates the rule prior to updating the
2461-    // software, and we would NOT be enforcing the rule here. Fully solving
2462-    // upgrade from one software version to the next after a consensus rule
2463-    // change is potentially tricky and issue-specific (see NeedsRedownload()
2464-    // for one approach that was used for BIP 141 deployment).

Sjors commented at 10:24 am on February 6, 2025:

ee05eb440310a1b9d6def8a26442433dffd1a122: dropping the “Fully solving” part of this remark seems too optimistic. When you upgrade a node after a soft-fork activates, we don’t roll back and recheck.

The only thing this commit fixes are -reindex(-chainstate) and (manually called) verifychain scenarios.

in src/validation.cpp:2460 in ee05eb4403 outdated

2465-    // Also, currently the rule against blocks more than 2 hours in the future
2466-    // is enforced in ContextualCheckBlockHeader(); we wouldn't want to
2467+    // Note the rule against blocks more than 2 hours in the future
2468+    // is not enforced in ContextualCheckBlockHeader(); we wouldn't want to
2469     // re-enforce that rule here (at least until we make it impossible for
2470     // the clock to go backward).

Sjors commented at 10:28 am on February 6, 2025:

ee05eb440310a1b9d6def8a26442433dffd1a122: this seems nonsensical to me, but it’s pre-existing behavior. If you run -reindex(-chainstate) on a node with a broken clock, it seems perfectly fine to me that it’s going to stop validation with an error. The user can then just fix their clock and continue.

Additionally, what if the users clock was too far in the future and they accidentally accepted a block they shouldn’t have. It seems fine that during a reindex, after the user fixed their clock, such a block gets rejected if it’s still too far in the future.

But maybe I’m missing some consideration that went into this.

In any case I think it’s fine to preserve the existing behavior in this regard, to keep the PR focussed.

Sjors commented at 1:18 pm on February 6, 2025:

cc @sdaftuar who wrote this comment in #11737

Sjors commented at 1:31 pm on February 6, 2025: member

Some historical context. The 2 hour future check used to be in CheckBlockHeader, but was moved in #8068 (Compact Blocks) in order to “prevent yet more includes and the crazy-looking GetAdjustedTime call from within the GetBlock stuff.”: https://github.com/bitcoin/bitcoin/pull/8068/files#r66835690

In order to avoid checking the merkle tree twice (which is incredibly expensive), I had to call CheckBlock, which required three #includes (including main.h) to blockencodings.cpp, despite them being really unrelated.

mzumsande commented at 3:58 pm on February 6, 2025: contributor

Running with -reindex-chainstate will leave you stuck in an infinite loop revalidating an invalid tip?

I wouldn’t expect that. ConnectTip() will call InvalidBlockFound() which should mark the block index as failed, and we won’t revalidate / instead will stay at the last valid block - same scenario as if it failed the script checks.

I can see the infinite loop happening only if a previously admitted block fails with BLOCK_MUTATED during connection, but mutation rules shouldn’t be affected by any of the currently discussed softforks?!

ajtowns commented at 4:08 pm on February 6, 2025: contributor

Running with -reindex-chainstate will leave you stuck in an infinite loop revalidating an invalid tip?

I wouldn’t expect that. ConnectTip() will call InvalidBlockFound() which should mark the block index as failed, and we won’t revalidate / instead will stay at the last valid block - same scenario as if it failed the script checks.

I can see the infinite loop happening only if a previously admitted block fails with BLOCK_MUTATED during connection, but mutation rules shouldn’t be affected by any of the currently discussed softforks?!

Ah, that’s fair: I’m testing by mining some blocks then changing the segwit activation, so it’s getting a “CheckWitnessMalleation” failure, because the blocks are marked as having been witness-verified.

mzumsande commented at 6:24 pm on February 6, 2025: contributor

Given that this will likely make IBD a bit slower due to repeated checks, I think there should be more tangible benefits than just simpler reasoning about edge cases - or maybe it would be good to explain these edge cases a bit more to be able to judge how relevant they are.

Once we are synced to the tip, block acception and connection typically happen right after each other in ProcessNewBlock(), so I can only think of of rather far-fetched scenarios for the repeated checks to become relevant out of IBD: E.g. Bitcoind could be shutting down right after AcceptBlock() but before ActivateBestChain() finished, and then restart with different consensus rules under which the accepted block is invalid - this doesn’t sound very likely to happen in practice due to timing-issues - and even if did, it should resolve itself quickly when more blocks arrive and we reorg out of the invalid chain.

Are there more realistic scenarios in which the double check would improve behavior compared to master?

darosior commented at 6:33 pm on February 6, 2025: member

This becomes a bit likelier in IBD, but i agree. As i said yesterday on IRC i am not sure this change is worth doing especially since we can never really solve the issue of a non-upgraded node having accepted invalid blocks short of re-connecting the whole chain when upgrading.

darosior commented at 6:34 pm on February 6, 2025: member

I’ll try to bug @sdaftuar, who introduced this comment, to see if we are missing something.

Sjors commented at 6:55 pm on February 6, 2025: member

short of re-connecting the whole chain when upgrading

As a generic method for all soft forks, it’s the only way. And it’s a potential pain, especially because for some reason unwinding is much slower than IBD (last time I tried). And doesn’t work on a too-far-pruned node.

But for specific soft-forks there may be quicker ways. E.g. it’s easy to scan the headers for timewarp violations. Checking coinbases for the new proposed BIP34 rule is only possible for non-pruned blocks, but would be quick (well, maybe not on a spinning disk without -txindex).

The new proposed sigops check on the other hand probably just requires an unwind. It would still be good to log a warning that we didn’t retroactively check the new rule, and suggest using the RPC method to check if they want (with the caveat that it’s slow and won’t work on a too-far-pruned node).

l0rinc commented at 1:07 pm on February 12, 2025: contributor

Given that this seemed like a change that would affect IBD, I ran a few reindex-chainstates and a few full IBDs to check for regressions. The reindexes are usually very stable so I only ran 2 iterations of before and after, comparing a full reindex-chainstate until 880k against master.

 0c59cbba518 update comment on MinimumChainWork check
 1ece9ac177d qa: sanity check ConnectBlock now checks all rules
 2
 3COMMITS="c59cbba518fc8835b9b26040dd9974ae3cddedfa ece9ac177d701da3a4fdb548453d9d87cc4eba49"; \
 4STOP_HEIGHT=880000; DBCACHE=10000; \
 5C_COMPILER=gcc; CXX_COMPILER=g++; \
 6hyperfine \
 7--export-json "/mnt/my_storage/rdx-${COMMITS/ /-}-${STOP_HEIGHT}-${DBCACHE}-${C_COMPILER}.json" \
 8--runs 2 \
 9--parameter-list COMMIT ${COMMITS/ /,} \
10--prepare "killall bitcoind; rm -f /mnt/my_storage/BitcoinData/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard; cmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER=$C_COMPILER -DCMAKE_CXX_COMPILER=$CXX_COMPILER && cmake --build build -j$(nproc) --target bitcoind && ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=$STOP_HEIGHT -dbcache=10000 -printtoconsole=0 || true" \
11--cleanup "cp /mnt/my_storage/BitcoinData/debug.log /mnt/my_storage/logs/debug-{COMMIT}-$(date +%s).log || true" \
12"COMPILER=$C_COMPILER COMMIT={COMMIT} ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=$STOP_HEIGHT -dbcache=$DBCACHE -printtoconsole=0 -reindex-chainstate -connect=0"
13
14Benchmark 1: COMPILER=gcc COMMIT=c59cbba518fc8835b9b26040dd9974ae3cddedfa ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -printtoconsole=0 -reindex-chainstate -connect=0
15  Time (mean ± σ):     25451.781 s ± 34.971 s    [User: 38952.582 s, System: 869.341 s]
16  Range (min … max):   25427.053 s … 25476.510 s    2 runs
17
18Benchmark 2: COMPILER=gcc COMMIT=ece9ac177d701da3a4fdb548453d9d87cc4eba49 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -printtoconsole=0 -reindex-chainstate -connect=0
19  Time (mean ± σ):     25707.027 s ± 53.363 s    [User: 39183.014 s, System: 869.561 s]
20  Range (min … max):   25669.293 s … 25744.760 s    2 runs
21
22Summary
23  COMPILER=gcc COMMIT=c59cbba518fc8835b9b26040dd9974ae3cddedfa ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -printtoconsole=0 -reindex-chainstate -connect=0 ran
24    1.01 ± 0.00 times faster than COMPILER=gcc COMMIT=ece9ac177d701da3a4fdb548453d9d87cc4eba49 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -printtoconsole=0 -reindex-chainstate -connect=0

this results in a 1% slowdown - which is unfortunate, but I personally could live with that.

The full IBD however resulted in a bigger diff (which is more volatile so I ran each 3 times to get meaningful results):

 0c59cbba518 update comment on MinimumChainWork check
 1ece9ac177d qa: sanity check ConnectBlock now checks all rules
 2
 3COMMITS="c59cbba518fc8835b9b26040dd9974ae3cddedfa ece9ac177d701da3a4fdb548453d9d87cc4eba49"; \
 4STOP_HEIGHT=880000; DBCACHE=10000; \
 5C_COMPILER=gcc; CXX_COMPILER=g++; \
 6hyperfine \
 7--export-json "/mnt/my_storage/ibd-${COMMITS/ /-}-${STOP_HEIGHT}-${DBCACHE}-${C_COMPILER}.json" \
 8--runs 3 \
 9--parameter-list COMMIT ${COMMITS/ /,} \
10--prepare "killall bitcoind; rm -rf /mnt/my_storage/BitcoinData/*; git checkout {COMMIT}; git clean -fxd; git reset --hard; cmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER=$C_COMPILER -DCMAKE_CXX_COMPILER=$CXX_COMPILER && cmake --build build -j$(nproc) --target bitcoind && ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=1 -printtoconsole=0 || true" \
11--cleanup "cp /mnt/my_storage/BitcoinData/debug.log /mnt/my_storage/logs/debug-{COMMIT}-$(date +%s).log || true" \
12"COMPILER=$C_COMPILER COMMIT={COMMIT} ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=$STOP_HEIGHT -dbcache=$DBCACHE -printtoconsole=0"
13Benchmark 1: COMPILER=gcc COMMIT=c59cbba518fc8835b9b26040dd9974ae3cddedfa ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -printtoconsole=0
14  Time (mean ± σ):     35311.267 s ± 32.492 s    [User: 58346.765 s, System: 2046.013 s]
15  Range (min … max):   35275.243 s … 35338.360 s    3 runs
16
17Benchmark 2: COMPILER=gcc COMMIT=ece9ac177d701da3a4fdb548453d9d87cc4eba49 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -printtoconsole=0
18  Time (mean ± σ):     36975.379 s ± 964.756 s    [User: 59059.701 s, System: 2312.492 s]
19  Range (min … max):   36213.629 s … 38060.212 s    3 runs
20
21Summary
22  COMPILER=gcc COMMIT=c59cbba518fc8835b9b26040dd9974ae3cddedfa ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -printtoconsole=0 ran
23    1.05 ± 0.03 times faster than COMPILER=gcc COMMIT=ece9ac177d701da3a4fdb548453d9d87cc4eba49 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -printtoconsole=0

So on average it’s ~4.7% slower and in the best case it was ~2.6% slower. I’m measuring the same again on a different server in reverse order to make sure this is representative, but this is a significant slowdown that I’d like us to eliminate - let me know how I can help, but until we do, it’s an Approach NACK from my part.

darosior commented at 2:40 pm on February 12, 2025: member

Thanks, it’s helpful to have numbers. I don’t think a 3-4% IBD slowdown should necessarily be a deal breaker, but as discussed above i do not think this change is useful enough to be worth it.

darosior commented at 2:46 pm on February 12, 2025: member

I’ll try to bug @sdaftuar, who introduced this comment, to see if we are missing something.

Had a quick chat about this with Suhas and a couple others at Chaincode. My takeaway from the discussion is that if we wanted this we should probably go all the way to actually re-validate past connected blocks that were not fully validated, not only those received but not yet connected. Also there is no reason to see this comment as a blocker to introduce new consensus rules (depending on the nature of the rule of course) when we have historically never supported this edge case.

darosior commented at 2:48 pm on February 12, 2025: member

I’ll have a look at whether it’s worth storing flags representing what rules have been checked against a block in the index, to enable re-validating past connected blocks against new rules when upgrading. In the meantime, this can be closed as i do not think it is worth doing on its own.

darosior closed this on Feb 12, 2025

Double check all block rules in ConnectBlock, not only CheckBlock #31792

Code Coverage & Benchmarks

Reviews

Conflicts

Double check all block rules in `ConnectBlock`, not only `CheckBlock` #31792