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

pull darosior wants to merge 6 commits into bitcoin:master from darosior:2502_ctx_checks_in_connectblock changing 21 files +54 −766
  1. 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.

  2. update comment on MinimumChainWork check c59cbba518
  3. 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
  4. 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

    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.

  5. DrahtBot added the label CI failed on Feb 3, 2025
  6. Armss9936 approved
  7. Armss9936 approved
  8. 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.
  9. 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.

  10. TheCharlatan commented at 11:17 am on February 4, 2025: contributor
    Concept ACK
  11. 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.
  12. 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?

  13. darosior force-pushed on Feb 4, 2025
  14. darosior force-pushed on Feb 4, 2025
  15. 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.

  16. 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.

  17. DrahtBot commented at 11:05 pm on February 4, 2025: contributor
    feature_unsupported_utxo_db.py fails (times out)
  18. darosior force-pushed on Feb 5, 2025
  19. 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.

  20. DrahtBot removed the label CI failed on Feb 5, 2025
  21. [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
  22. [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
  23. 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
  24. qa: sanity check ConnectBlock now checks all rules ece9ac177d
  25. darosior force-pushed on Feb 5, 2025
  26. 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.

  27. 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?

  28. 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.

  29. 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)

  30. 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.

  31. 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
  32. 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.

  33. 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?!

  34. 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.

  35. 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?

  36. 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.
  37. 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.
  38. 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).


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-07 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me