log: always print initial signature verification state #33336

pull l0rinc wants to merge 5 commits into bitcoin:master from l0rinc:l0rinc/log-initial-signature-verification-state changing 3 files +126 −55
  1. l0rinc commented at 6:40 am on September 8, 2025: contributor

    Summary

    Users can encounter cases where script checks are unexpectedly enabled (e.g. after reindex, or when assumevalid/minimumchainwork gates fail). Without an explicit line, they must infer state from the absence of a message, which is error-prone. The existing “Assuming ancestors of block …” line does not reliably indicate whether script checks are actually enabled, which makes debugging/benchmarking confusing.

    What this changes

    We make the initial script-verification state explicit and log why checks are enabled to avoid confusion.

    • Always log the first script-verification state on startup, before the first UpdateTip.
    • Flatten the nested assumevalid conditionals into a linear gating sequence for readability.
    • Extend the functional test to assert the old behavior with the new reason strings.

    This is a logging-only test change it shouldn’t change any other behavior.

    Safety

    • We log on the first observation only, using std::optional<bool>; this is safe because ConnectBlock already holds cs_main.
    • No consensus or networking changes; strictly diagnostic.

    Example output

    The state (with reason) is logged at startup and whenever the reason changes, e.g.:

    • Disabling signature validations at block [#904336](/bitcoin-bitcoin/904336/) (000000000000000000014106b2082b1a18aaf3091e8b337c6fed110db8c56620).
    • Enabling signature validations at block [#912527](/bitcoin-bitcoin/912527/) (000000000000000000010bb6aa3ecabd7d41738463b6c6621776c2e40dbe738a): too recent relative to best header.
    • Enabling signature validations at block [#912684](/bitcoin-bitcoin/912684/) (00000000000000000001375cf7b90b2b86e559d05ed92ca764d376702ead3858): block not part of assumevalid chain.

    Follow-up to #32975 (review)

  2. DrahtBot added the label Validation on Sep 8, 2025
  3. DrahtBot commented at 6:40 am on September 8, 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/33336.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK yuvicc, Eunovo
    Stale ACK TheCharlatan, hodlinator, w0xlt

    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:

    • #31615 (validation: ensure assumevalid is always used during reindex by Eunovo)

    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.

  4. l0rinc renamed this:
    validation: log initial signature verification state
    log: always print initial signature verification state
    on Sep 8, 2025
  5. yuvicc commented at 7:54 am on September 8, 2025: contributor

    Concept ACK.

    The approach of switching to std::optional<bool> here avoids the overhead of atomic operations relying on cs_main for synchronization makes sense.

    02025-09-08T07:43:58Z Disabling signature validations at block [#16893](/bitcoin-bitcoin/16893/) (00000000113c355415c3417c734a45b62790b91b41ff08028580254c584f5ea8).
    
  6. l0rinc commented at 4:28 am on September 10, 2025: contributor
    For more context, even just restarting the node that was reindexed before (and is at around 200k blocks) with -assumevalid=0000000000000000000087564caa77e7b3f29d0464256c04d5539e43663f8698 (block 914005), it still starts with signature verification enabled - using 70% of CPU when it should have been disabled in the first place, see: This PR makes it at least obvious that something’s off. #31615 (comment) doesn’t seem to fix the issue, but that’s orthogonal with at least making it obvious via the logs.
  7. in src/validation.h:563 in 64ba2821b5 outdated
    559@@ -560,7 +560,7 @@ class Chainstate
    560     //! Cached result of LookupBlockIndex(*m_from_snapshot_blockhash)
    561     mutable const CBlockIndex* m_cached_snapshot_base GUARDED_BY(::cs_main){nullptr};
    562 
    563-    std::atomic_bool m_prev_script_checks_logged{true};
    564+    std::optional<bool> m_prev_script_checks_logged{};
    


    Eunovo commented at 9:39 am on September 10, 2025:
    https://github.com/bitcoin/bitcoin/pull/33336/commits/64ba2821b5a55aa6d51fbb7a16470d4f7c41c92e: I believe you now need to add GUARDED_BY(::cs_main) here to use Clang’s Thread Safety Analysis

    l0rinc commented at 6:24 pm on September 10, 2025:
    Excellent, added you as a coathor, thanks.
  8. l0rinc force-pushed on Sep 10, 2025
  9. l0rinc force-pushed on Sep 10, 2025
  10. in test/functional/feature_assumevalid.py:151 in 8cb75ee0a6 outdated
    145@@ -146,7 +146,8 @@ def run_test(self):
    146         p2p0.send_header_for_blocks(self.blocks[2000:])
    147 
    148         # Send blocks to node0. Block 102 will be rejected.
    149-        self.send_blocks_until_disconnected(p2p0)
    150+        with self.nodes[0].assert_debug_log(expected_msgs=['Enabling signature validations at block #1']):
    151+            self.send_blocks_until_disconnected(p2p0)
    152         self.wait_until(lambda: self.nodes[0].getblockcount() >= COINBASE_MATURITY + 1)
    


    hodlinator commented at 8:02 pm on September 10, 2025:

    nit: In case we are ever able to send away all blocks before detecting the expected log message, we could also wait for the block height before stopping the search of the log:

    0            self.wait_until(lambda: self.nodes[0].getblockcount() >= COINBASE_MATURITY + 1)
    

    (Edit: I was modifying send_blocks_until_disconnected() and noticed a fairly high variability in how many blocks would be sent off, CI probably would have higher variability).


    l0rinc commented at 9:36 pm on September 10, 2025:
    This seems less racy, indeed, updated, added you as coauthor, thanks
  11. hodlinator approved
  12. hodlinator commented at 8:26 pm on September 10, 2025: contributor

    ACK 8cb75ee0a63880b55ac695fe884748aa7d604c38

    While it is unsettling that we don’t fully understand what causes script checking to occur prior to the assumevalid height (https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2329269037) - having an initial “Enabling signature validations at block [#1](/bitcoin-bitcoin/1/) ...” in -assumevalid=0 cases as per this PR seems fine regardless.

    (Empty std::optional does not match bool fScriptChecks until set).

    https://github.com/bitcoin/bitcoin/blob/8cb75ee0a63880b55ac695fe884748aa7d604c38/src/validation.cpp#L2566-L2569

    (Memory behavior when locking/unlocking a mutex (cs_main) does AFAIK imply atomic_ on master was never necessary).

  13. DrahtBot requested review from yuvicc on Sep 10, 2025
  14. TheCharlatan approved
  15. TheCharlatan commented at 9:33 pm on September 10, 2025: contributor

    ACK 8cb75ee0a63880b55ac695fe884748aa7d604c38

    The log message is a bit clunky though. Since it now appears on startup I think it would be nice if it also mentioned that this just reflects assumevalid.

  16. l0rinc commented at 9:36 pm on September 10, 2025: contributor

    that this just reflects assumevalid

    Unfortunately it’s more complicated than that, see #33336 (comment) If you have a concrete suggestion for how to rephrase it, I don’t mind applying it.

  17. l0rinc force-pushed on Sep 10, 2025
  18. l0rinc commented at 5:38 am on September 11, 2025: contributor

    I have instrumented the code, ran some RPC commands and reindexed the whole thing again to see why script verification is always on. After some discussions with @ajtowns it turns out I needed both -assumevalid=<in-range header> and -minimumchainwork=0 to be able to disable script verification.


    After reindex, my node has ~841k headers, a lot lower than the current tip of ~914k. The default assumevalid block (height 912,683) is in that missing range, so the header lookup fails since the block isn’t found in the block index (even though the log claims “Assuming ancestors of block … have valid signatures”). Even when I set a custom assumevalid to a block I actually have, my chainwork is still below the bumped nMinimumChainWork from 755152ac.

    So the results were:

    • Default params: ./build/bin/bitcoind - script verification enabled since block is not among the headers
    • Custom assumevalid from the headers (841,149): ./build/bin/bitcoind -assumevalid=00000000000000000000529087e51068a98d16a071401d7d43494c4a3bf75cd6 - script verification still enabled. Header lookup works fine, but my chainwork (769fbaeb...) is below the required minimum (dee8e2a3...).
    • Default assumevalid with low chainwork: ./build/bin/bitcoind -minimumchainwork=0 - still doesn’t work because it falls back to the default assumevalid which isn’t in my headers.
    • Custom assumevalid with low chainwork: ./build/bin/bitcoind -assumevalid=00000000000000000000529087e51068a98d16a071401d7d43494c4a3bf75cd6 -minimumchainwork=0 - script verification finally gets disabled.

    So maybe we should update the logs to give the reason for the script verification more precisely: script verification is enabled because the assumevalid block cannot be found / the available nChainWork isn’t sufficient / the blocks are too close to the current tip.

    The early “Assuming ancestors of block …” line is emitted even when that header is not present locally yet - maybe we should adjust this, too.

  19. ajtowns commented at 6:04 am on September 11, 2025: contributor

    After reindex, my node has ~841k headers, a lot lower than the current tip of ~914k. The default assumevalid block (height 912,683) is in that missing range,

    I think the behaviour here is probably fine – it’s a weak way of preventing you from triggering assumevalid behaviour on an invalid chain with substantially lower total work than the main chain. It might be nice to have a warning that your headers chain doesn’t meet minchainwork though. It would probably be nicer to be able to obtain the missing headers though.

    EDIT: probably fine, compared to the alternative of dropping the best_header->nChainWork >= MinimumChainWork check, and just relying on the assumevalid block being in the best_header’s ancestry. That didn’t seem to have much rationale when it was introduced afaics: #9484 (comment)

  20. w0xlt commented at 6:55 am on September 11, 2025: contributor

    Approach ACK

    Using std::optional<bool> introduces a third state (unset), which guarantees exactly one initial log message—regardless of whether the first real value is true or false.

    For reviewers, here’s a simple way to verify the behavior change:

    0mkdir temp_data_dir
    1./build/bin/bitcoind -regtest -datadir=temp_data_dir -daemon
    2./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir createwallet w
    3./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir getnewaddress
    4./build/bin/bitcoin-cli -regtest -datadir=temp_data_dir generatetoaddress 1 <address>
    

    With this PR, the logs will include:

    0Enabling signature validations at block [#1](/bitcoin-bitcoin/1/)
    

    In master, this line does not appear.

  21. Eunovo commented at 8:02 am on September 11, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/39f41135c764c6c4705ce2cd36781ca8d43f0114:

    Previously, users could only verify that script checks were enabled by confirming the absence of a “Disabling signature validations at block #1” message in the logs, which is a confusing negative confirmation pattern. This PR adds explicit positive feedback by ensuring an “Enabling signature validations at block #1” message appears when script checks are enabled, making the system state clearer and easier to verify.

  22. DrahtBot requested review from hodlinator on Sep 11, 2025
  23. DrahtBot requested review from TheCharlatan on Sep 11, 2025
  24. DrahtBot requested review from w0xlt on Sep 11, 2025
  25. Eunovo commented at 8:15 am on September 11, 2025: contributor

    After reindex, my node has ~841k headers, a lot lower than the current tip of ~914k. The default assumevalid block (height 912,683) is in that missing range, so the header lookup fails since the block isn’t found in the block index (even though the log claims “Assuming ancestors of block … have valid signatures”). Even when I set a custom assumevalid to a block I actually have, my chainwork is still below the bumped nMinimumChainWork from 755152ac.

    It’s surprising that #31615 didn’t help with this. If the block wasn’t found in the index, it’s supposed to skip script checks while reindexing. If the block was found in the index, it will skip the MinimumChainwork check. The only reason why script checks will not get skipped is if the chosen assumed valid block is in the index and it is less than 2 weeks away from the tip of the chain, due to this line fScriptChecks = (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, params.GetConsensus()) <= 60 * 60 * 24 * 7 * 2);

  26. hodlinator approved
  27. hodlinator commented at 8:18 am on September 11, 2025: contributor

    re-ACK 39f41135c764c6c4705ce2cd36781ca8d43f0114

    Only made test less racy (https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2337774261) since the first ACK.


    After reindex, my node has ~841k headers, a lot lower than the current tip of ~914k. The default assumevalid block (height 912,683) is in that missing range, [would say: is outside that range]

    I think the behaviour here is probably fine – it’s a weak way of preventing you from triggering assumevalid behaviour on an invalid chain with substantially lower total work than the main chain. It might be nice to have a warning that your headers chain doesn’t meet minchainwork though. It would probably be nicer to be able to obtain the missing headers though.

    EDIT: probably fine, compared to the alternative of dropping the best_header->nChainWork >= MinimumChainWork check, and just relying on the assumevalid block being in the best_header’s ancestry. That didn’t seem to have much rationale when it was introduced afaics: #9484 (comment)

    (Bottom of https://github.com/bitcoin/bitcoin/pull/9484/commits/e440ac7ef3b6f3ad1cd8fc7027cece40413202d9 is interesting, enforcing script validation within the last 2 weeks regardless of assumevalid settings. It’s annoying that the current default assume valid block height of 912'683 is ~10 days ago, but shouldn’t matter much).

    I would hope this condition kicked in and filled in the missing headers from 841k to the nMinimumChainWork at 912'683 so that the best header could be updated and script validation could be turned off again until the last 2 weeks of blocks:

    https://github.com/bitcoin/bitcoin/blob/39f41135c764c6c4705ce2cd36781ca8d43f0114/src/net_processing.cpp#L5526-L5539

    Any clue to why missing headers are not being fetched / best header isn’t updated?

  28. achow101 added this to the milestone 30.0 on Sep 11, 2025
  29. w0xlt commented at 0:11 am on September 12, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/39f41135c764c6c4705ce2cd36781ca8d43f0114

    Simple test on mainnet:

    0./build/bin/bitcoind -datadir=new_data_dir/ -assumevalid=000000009b7262315dbf071787ad3656097b892abffd1f95a1a022f896f533fc -stopatheight=10
    
  30. l0rinc commented at 1:12 am on September 14, 2025: contributor

    It might be nice to have a warning that your headers chain doesn’t meet minchainwork though

    Given how confusing #33336 (comment) was for many of us, it may be beneficial to extend this PR to show the reason for why script verification was enabled.

    Given the number of acks and time constraints, I have to ask @yuvicc, @Eunovo, @hodlinator, @TheCharlatan, @ajtowns, @w0xlt: would that be welcome change, see https://github.com/l0rinc/bitcoin/pull/38/files

    Edit: split the above suggestion into small focused commits and pushed here since I think it’s arguably superior to previous logging attempts.

  31. l0rinc force-pushed on Sep 14, 2025
  32. w0xlt commented at 4:24 am on September 15, 2025: contributor

    Approach ACK.

    beneficial to extend this PR to show the reason for why script verification was enabled.

    I agree.

    split the above suggestion into small focused commits

    Commits could be squashed but but keeping them separate makes it clearer to review.

  33. in src/validation.cpp:2459 in 63dc937eea outdated
    2475+            assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID;
    2476+        } else if (m_chainman.m_best_header->GetAncestor(pindex->nHeight) != pindex) {
    2477+            assume_valid = CHECKED_OFF_BESTHEADER_PATH;
    2478+        } else if (m_chainman.m_best_header->nChainWork < m_chainman.MinimumChainWork()) {
    2479+            assume_valid = CHECKED_BELOW_MIN_CHAINWORK;
    2480+        } else if (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, params.GetConsensus()) <= 60 * 60 * 24 * 7 * 2) {
    


    w0xlt commented at 5:21 am on September 15, 2025:

    Just to make clearer what 60 * 60 * 24 * 7 * 2 means.

    0constexpr int64_t TWO_WEEKS = 14 * 24 * 60 * 60;
    1if (GetBlockProofEquivalentTime(... ) <= TWO_WEEKS) { ... }
    

    Or (not tested)

    0#include <chrono>
    1using namespace std::chrono;
    2
    3constexpr auto TWO_WEEKS = 14d; // 14 days
    4
    5if (GetBlockProofEquivalentTime(...) <= duration_cast<seconds>(TWO_WEEKS).count()) {
    6    ...
    7}
    

    l0rinc commented at 5:34 am on September 15, 2025:
    Seems a bit risky this close to the release, maybe we can do that in a follow-up

    l0rinc commented at 5:47 pm on September 15, 2025:
    But extracting TWO_WEEKS_IN_SECONDS is safe, did that, thanks. Added you as coauthor.
  34. l0rinc requested review from hodlinator on Sep 15, 2025
  35. l0rinc requested review from Eunovo on Sep 15, 2025
  36. l0rinc requested review from w0xlt on Sep 15, 2025
  37. in src/validation.cpp:122 in 63dc937eea outdated
    117+    case CHECKED_HASH_NOT_IN_HEADERS:   return "assumevalid hash not in headers";
    118+    case CHECKED_NOT_UNDER_ASSUMEVALID: return "block not under assumevalid anchor";
    119+    case CHECKED_OFF_BESTHEADER_PATH:   return "block not on best header path";
    120+    case CHECKED_BELOW_MIN_CHAINWORK:   return "best header chainwork below nMinimumChainWork";
    121+    case CHECKED_NOT_BURIED_ENOUGH:     return "too recent relative to best header";
    122+    default:                            std::abort();
    


    w0xlt commented at 5:36 am on September 15, 2025:

    Provide safe fallback in release builds. Aborting solely because of logging seems unnecessary.

    0    default:                            return "unknown reason";
    

    l0rinc commented at 5:51 am on September 15, 2025:
    I want a very obvious error if this isn’t correct - I don’t like these soft failures. What do other reviewers think?

    Eunovo commented at 8:48 am on September 15, 2025:

    l0rinc commented at 5:45 pm on September 15, 2025:
    Done

    hodlinator commented at 7:02 pm on September 15, 2025:

    Please consider triggering a compiler warning instead.

    0₿ git grep "// no default case, so the compiler can warn about missing cases" |wc -l
    170
    

    https://github.com/bitcoin/bitcoin/blob/9b04e4f96200daf7516d1b8b6b0dfc4c077837e8/src/deploymentinfo.cpp#L36-L37 Edit: could of course return “unknown reason” rather than empty string in this case, just outside the switch.

  38. in src/validation.cpp:116 in 63dc937eea outdated
    112+{
    113+    using enum AssumeValid;
    114+    switch (v) {
    115+    case SKIPPED:                       return "";
    116+    case CHECKED:                       return "assumevalid=0 (always verify)";
    117+    case CHECKED_HASH_NOT_IN_HEADERS:   return "assumevalid hash not in headers";
    


    w0xlt commented at 5:40 am on September 15, 2025:

    nit:

    0    case CHECKED_HASH_NOT_IN_HEADERS:   return "assumevalid hash not found in headers";
    

    l0rinc commented at 5:49 am on September 15, 2025:
    This was deliberate, are you suggesting that’s not grammatically correct? “File not found” -> we don’t usually say “File is not found”. The internets tell me this is called “elliptical sentence” with “implied copula”.
  39. in src/validation.cpp:120 in 63dc937eea outdated
    115+    case SKIPPED:                       return "";
    116+    case CHECKED:                       return "assumevalid=0 (always verify)";
    117+    case CHECKED_HASH_NOT_IN_HEADERS:   return "assumevalid hash not in headers";
    118+    case CHECKED_NOT_UNDER_ASSUMEVALID: return "block not under assumevalid anchor";
    119+    case CHECKED_OFF_BESTHEADER_PATH:   return "block not on best header path";
    120+    case CHECKED_BELOW_MIN_CHAINWORK:   return "best header chainwork below nMinimumChainWork";
    


    w0xlt commented at 5:42 am on September 15, 2025:

    nit: logs shouldn’t expose internal variable names

    0    case CHECKED_BELOW_MIN_CHAINWORK:   return "best header chainwork below -minimumchainwork";
    

    l0rinc commented at 5:50 am on September 15, 2025:
    I’m curious what others think. The current one makes the error searchable, that’s why I left it as such.

    l0rinc commented at 5:44 pm on September 15, 2025:
    Changed in latest push, thanks
  40. w0xlt commented at 5:46 am on September 15, 2025: contributor
  41. in test/functional/feature_assumevalid.py:194 in 3ddd4c84af outdated
    189+        assert_equal(self.nodes[3].getblockcount(), 1)
    190+
    191+        self.restart_node(3, extra_args=["-reindex-chainstate", "-assumevalid=" + block102.hash_hex, "-minimumchainwork=0xffff"])
    192+        assert_equal(self.nodes[3].getblockcount(), 1)
    193+
    194+        # TODO test what happens when block is not on the best header path
    


    Eunovo commented at 9:47 am on September 15, 2025:
    https://github.com/bitcoin/bitcoin/pull/33336/commits/3ddd4c84af2bf0b5d3e403b71f4750b2326118e1: I think it’s best to take out this TODO from the commit. You should also adjust the commit message to focus on what this commit is adding specifically.

    l0rinc commented at 5:51 pm on September 15, 2025:
    Lol, valid point, let’s document what we are doing, not just what we’re not :D Added comment and updated the commit message (but kept the todo for now), added you as coauthor.
  42. in test/functional/feature_assumevalid.py:68 in 3ddd4c84af outdated
    64@@ -64,11 +65,11 @@ def send_header_for_blocks(self, new_blocks):
    65 class AssumeValidTest(BitcoinTestFramework):
    66     def set_test_params(self):
    67         self.setup_clean_chain = True
    68-        self.num_nodes = 3
    69+        self.num_nodes = 4
    


    Eunovo commented at 9:48 am on September 15, 2025:
    https://github.com/bitcoin/bitcoin/pull/33336/commits/3ddd4c84af2bf0b5d3e403b71f4750b2326118e1: This should probably still be set to 3 since the 4th node is not used in this commit.

    l0rinc commented at 5:49 pm on September 15, 2025:
    the 4th is doing a reindex even in the first commit, right?
  43. in test/functional/feature_assumevalid.py:188 in 3ddd4c84af outdated
    183+        p2p3 = self.nodes[3].add_p2p_connection(BaseNode())
    184+        p2p3.send_header_for_blocks(self.blocks[0:200])
    185+        p2p3.send_without_ping(msg_block(self.blocks[0]))
    186+        self.wait_until(lambda: self.nodes[3].getblockcount())
    187+
    188+        self.restart_node(3, extra_args=["-reindex-chainstate", "-assumevalid=1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"])
    


    Eunovo commented at 9:52 am on September 15, 2025:

    l0rinc commented at 5:58 pm on September 15, 2025:
    Done
  44. in test/functional/feature_assumevalid.py:191 in 3ddd4c84af outdated
    186+        self.wait_until(lambda: self.nodes[3].getblockcount())
    187+
    188+        self.restart_node(3, extra_args=["-reindex-chainstate", "-assumevalid=1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"])
    189+        assert_equal(self.nodes[3].getblockcount(), 1)
    190+
    191+        self.restart_node(3, extra_args=["-reindex-chainstate", "-assumevalid=" + block102.hash_hex, "-minimumchainwork=0xffff"])
    


    Eunovo commented at 9:52 am on September 15, 2025:

    l0rinc commented at 5:58 pm on September 15, 2025:
    Added one above + explained lightly in commit message
  45. in src/validation.cpp:2441 in 6222d8cced outdated
    2422@@ -2423,7 +2423,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
    2423         return true;
    2424     }
    2425 
    2426-    auto assume_valid{true};
    2427+    auto assume_valid{AssumeValid::CHECKED};
    


    Eunovo commented at 10:08 am on September 15, 2025:
    https://github.com/bitcoin/bitcoin/pull/33336/commits/6222d8cced4828b83a9dde20b9da9df44848b9bd: Why not AssumeValid::DISABLED instead of AssumeValid::CHECKED and AssumeValid::ENABLED instead of AssumeValid:SKIPPED? I find the current names to be confusing. I think using AssumeValid::ENABLED to indicate that assume_valid is enabled is better.

    l0rinc commented at 5:32 pm on September 15, 2025:

    I tried explaining it in 6222d8c (#33336)

    Instead of true/false or enabled/disabled, the checked/skipped naming should help with understanding when signature verification is actually performed.

    I found it confusing to say that we disable assume valid - so are we doing the script checks or not, will IBD be faster or slower is it’s disabled?! So what’s a better expression to avoid the confusion?


    l0rinc commented at 8:50 pm on September 16, 2025:
    Reverted since it caused more confusion that it fixed
  46. in src/validation.cpp:119 in 63dc937eea outdated
    114+    switch (v) {
    115+    case SKIPPED:                       return "";
    116+    case CHECKED:                       return "assumevalid=0 (always verify)";
    117+    case CHECKED_HASH_NOT_IN_HEADERS:   return "assumevalid hash not in headers";
    118+    case CHECKED_NOT_UNDER_ASSUMEVALID: return "block not under assumevalid anchor";
    119+    case CHECKED_OFF_BESTHEADER_PATH:   return "block not on best header path";
    


    Eunovo commented at 10:24 am on September 15, 2025:

    l0rinc commented at 5:43 pm on September 15, 2025:
    Done, thanks, added you as coauthor
  47. in src/validation.cpp:118 in 63dc937eea outdated
    113+    using enum AssumeValid;
    114+    switch (v) {
    115+    case SKIPPED:                       return "";
    116+    case CHECKED:                       return "assumevalid=0 (always verify)";
    117+    case CHECKED_HASH_NOT_IN_HEADERS:   return "assumevalid hash not in headers";
    118+    case CHECKED_NOT_UNDER_ASSUMEVALID: return "block not under assumevalid anchor";
    


    Eunovo commented at 10:25 am on September 15, 2025:

    l0rinc commented at 5:43 pm on September 15, 2025:
    Done, thanks
  48. Eunovo commented at 10:28 am on September 15, 2025: contributor
  49. l0rinc force-pushed on Sep 15, 2025
  50. l0rinc commented at 6:00 pm on September 15, 2025: contributor
    Thanks for the suggestions, applied most of them
  51. DrahtBot requested review from Eunovo on Sep 16, 2025
  52. in src/validation.cpp:2426 in e4077fe0d7 outdated
    2422@@ -2423,7 +2423,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
    2423         return true;
    2424     }
    2425 
    2426-    bool fScriptChecks = true;
    2427+    auto assume_valid{true};
    


    hodlinator commented at 6:27 am on September 16, 2025:
    In e4077fe0d7ae245e1dd3e89903e818703c0ab130: fScriptChecks seems to be the opposite of what I would expect assume_valid to mean (no script checking)?

    l0rinc commented at 9:05 pm on September 16, 2025:
    Ii have reverted the naming
  53. in src/validation.h:525 in 9b04e4f962 outdated
    520+    CHECKED_HASH_NOT_IN_HEADERS = 2,   //!< assumevalid hash not found in m_block_index
    521+    CHECKED_NOT_UNDER_ASSUMEVALID = 3, //!< pindex is not an ancestor of the assumevalid anchor
    522+    CHECKED_OFF_BESTHEADER_PATH = 4,   //!< pindex is not an ancestor of m_best_header
    523+    CHECKED_BELOW_MIN_CHAINWORK = 5,   //!< best header's cumulative work is below the built-in minimum
    524+    CHECKED_NOT_BURIED_ENOUGH = 6,     //!< too recent compared to best header
    525+};
    


    hodlinator commented at 6:39 am on September 16, 2025:

    Again, AssumeValid::SKIPPED meaning script checking is off seems like some weird double-negation to me. Below naming would make more sense to me.

    0enum class ScriptCheck : uint8_t
    1{
    2    DISABLED_ASSUME_VALID,         //!< skip script verification
    3    ENABLED_NO_ASSUME_VALID,       //!< always verify scripts
    4    ENABLED_HASH_NOT_IN_HEADERS,   //!< assumevalid hash not found in m_block_index
    5    ENABLED_NOT_UNDER_ASSUMEVALID, //!< pindex is not an ancestor of the assumevalid anchor
    6    ENABLED_OFF_BESTHEADER_FORK,   //!< pindex is not an ancestor of m_best_header
    7    ENABLED_BELOW_MIN_CHAINWORK,   //!< best header's cumulative work is below the built-in minimum
    8    ENABLED_NOT_BURIED_ENOUGH,     //!< too recent compared to best header
    9};
    

    (Also removed explicit numbers as we aren’t serializing them to disk or even writing them to the log, but keep if you want).

    (PATH -> FORK).


    l0rinc commented at 6:43 pm on September 16, 2025:

    Cheap script checks (such as sigop counts) are still checked regardless of this option, it’s the signatures that aren’t (so some script sanity checking is still happening, but the scripts aren’t interpreted - that’s my understanding, please correct me if I’m wrong. Maybe I should update the warning message in that case to make it clear that a few other checks are also skipped, not just the signatures).

    And I want to propose a change to skip bip30 checks as part of “assumevalid” (and to skip other script related checks like the mentioned sigop count), so I don’t want to introduce name that forces the option to only apply to signatures. I’ll just revert the rename for now and we can decide later.

  54. in test/functional/feature_assumevalid.py:154 in 9b04e4f962 outdated
    151 
    152         # Send blocks to node0. Block 102 will be rejected.
    153-        self.send_blocks_until_disconnected(p2p0)
    154-        self.wait_until(lambda: self.nodes[0].getblockcount() >= COINBASE_MATURITY + 1)
    155+        with self.nodes[0].assert_debug_log(expected_msgs=['Enabling signature validations at block #1',
    156+                                                           'assumevalid=0 (always verify)']):  # AssumeValid::CHECKED
    


    hodlinator commented at 7:51 am on September 16, 2025:

    Could be more precise in that these strings are on the same log line:

     0diff --git a/test/functional/feature_assumevalid.py b/test/functional/feature_assumevalid.py
     1index 544edfdb99..9ebaddb74f 100755
     2--- a/test/functional/feature_assumevalid.py
     3+++ b/test/functional/feature_assumevalid.py
     4@@ -150,8 +150,9 @@ class AssumeValidTest(BitcoinTestFramework):
     5         p2p0.send_header_for_blocks(self.blocks[2000:])
     6 
     7         # Send blocks to node0. Block 102 will be rejected.
     8-        with self.nodes[0].assert_debug_log(expected_msgs=['Enabling signature validations at block [#1](/bitcoin-bitcoin/1/)',
     9-                                                           'assumevalid=0 (always verify)']):  # AssumeValid::CHECKED
    10+        block_1_hash = self.blocks[0].hash_hex
    11+        with self.nodes[0].assert_debug_log(expected_msgs=[f'Enabling signature validations at block [#1](/bitcoin-bitcoin/1/) ({block_1_hash}): assumevalid=0 (always verify)',  # AssumeValid::CHECKED
    12+                                                           'Block validation error: block-script-verify-flag-failed']):
    13             self.send_blocks_until_disconnected(p2p0)
    14             self.wait_until(lambda: self.nodes[0].getblockcount() >= COINBASE_MATURITY + 1)
    15         assert_equal(self.nodes[0].getblockcount(), COINBASE_MATURITY + 1)
    16@@ -162,8 +163,7 @@ class AssumeValidTest(BitcoinTestFramework):
    17         p2p1.send_header_for_blocks(self.blocks[0:2000])
    18         p2p1.send_header_for_blocks(self.blocks[2000:])
    19         with self.nodes[1].assert_debug_log(expected_msgs=['Disabling signature validations at block [#1](/bitcoin-bitcoin/1/)',  # AssumeValid::SKIPPED
    20-                                                           'Enabling signature validations at block [#103](/bitcoin-bitcoin/103/)',
    21-                                                           'block not part of assumevalid chain']):  # AssumeValid::CHECKED_NOT_UNDER_ASSUMEVALID
    22+                                                           f'Enabling signature validations at block [#103](/bitcoin-bitcoin/103/) ({self.blocks[102].hash_hex}): block not part of assumevalid chain']):  # AssumeValid::CHECKED_NOT_UNDER_ASSUMEVALID
    23             # Send all blocks to node1. All blocks will be accepted.
    24             for i in range(2202):
    25                 p2p1.send_without_ping(msg_block(self.blocks[i]))
    26@@ -177,8 +177,8 @@ class AssumeValidTest(BitcoinTestFramework):
    27         p2p2.send_header_for_blocks(self.blocks[0:200])
    28 
    29         # Send blocks to node2. Block 102 will be rejected.
    30-        with self.nodes[2].assert_debug_log(expected_msgs=["Enabling signature validations at block [#1](/bitcoin-bitcoin/1/)",
    31-                                                           "too recent relative to best header"]):  # AssumeValid::CHECKED_NOT_BURIED_ENOUGH
    32+        with self.nodes[2].assert_debug_log(expected_msgs=[f"Enabling signature validations at block [#1](/bitcoin-bitcoin/1/) ({block_1_hash}): too recent relative to best header",  # AssumeValid::CHECKED_NOT_BURIED_ENOUGH
    33+                                                           "Block validation error: block-script-verify-flag-failed"]):
    34             self.send_blocks_until_disconnected(p2p2)
    35             self.wait_until(lambda: self.nodes[2].getblockcount() >= COINBASE_MATURITY + 1)
    36         assert_equal(self.nodes[2].getblockcount(), COINBASE_MATURITY + 1)
    37@@ -191,13 +191,11 @@ class AssumeValidTest(BitcoinTestFramework):
    38         self.wait_until(lambda: self.nodes[3].getblockcount())
    39 
    40         # Reindex to deterministically hit specific assumevalid gates (no races with header downloads/chainwork during startup).
    41-        with self.nodes[3].assert_debug_log(expected_msgs=["Enabling signature validations at block [#1](/bitcoin-bitcoin/1/)",
    42-                                                           "assumevalid hash not in headers"]):  # AssumeValid::CHECKED_HASH_NOT_IN_HEADERS
    43+        with self.nodes[3].assert_debug_log(expected_msgs=[f"Enabling signature validations at block [#1](/bitcoin-bitcoin/1/) ({block_1_hash}): assumevalid hash not in headers"]):  # AssumeValid::CHECKED_HASH_NOT_IN_HEADERS
    44             self.restart_node(3, extra_args=["-reindex-chainstate", "-assumevalid=1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"])
    45         assert_equal(self.nodes[3].getblockcount(), 1)
    46 
    47-        with self.nodes[3].assert_debug_log(expected_msgs=["Enabling signature validations at block [#1](/bitcoin-bitcoin/1/)",
    48-                                                           "best header chainwork below minimumchainwork"]):  # AssumeValid::CHECKED_BELOW_MIN_CHAINWORK
    49+        with self.nodes[3].assert_debug_log(expected_msgs=[f"Enabling signature validations at block [#1](/bitcoin-bitcoin/1/) ({block_1_hash}): best header chainwork below minimumchainwork"]):  # AssumeValid::CHECKED_BELOW_MIN_CHAINWORK
    50             self.restart_node(3, extra_args=["-reindex-chainstate", "-assumevalid=" + block102.hash_hex, "-minimumchainwork=0xffff"])
    51         assert_equal(self.nodes[3].getblockcount(), 1)
    52 
    

    (Also adds check for “Block validation error”).


    l0rinc commented at 5:32 pm on September 16, 2025:
    Was trying to recreate production code logic in test, but I don’t mind making it more explicit if you think it’s more descriptive. The block validation errors seem out-of-scope, but I don’t mind them either, thanks, added you as coauthor to the last commit as well.
  55. in test/functional/feature_assumevalid.py:166 in 9b04e4f962 outdated
    164         p2p1.send_header_for_blocks(self.blocks[0:2000])
    165         p2p1.send_header_for_blocks(self.blocks[2000:])
    166-        with self.nodes[1].assert_debug_log(expected_msgs=['Disabling signature validations at block #1', 'Enabling signature validations at block #103']):
    167+        with self.nodes[1].assert_debug_log(expected_msgs=['Disabling signature validations at block #1',  # AssumeValid::SKIPPED
    168+                                                           'Enabling signature validations at block #103',
    169+                                                           'block not part of assumevalid chain']):  # AssumeValid::CHECKED_NOT_UNDER_ASSUMEVALID
    


    hodlinator commented at 8:55 am on September 16, 2025:

    It feels a bit lacking to have CHECKED_NOT_UNDER_ASSUMEVALID when the block is based upon the assumevalid block. Would expect something like CHECKED_EXCEEDS_ASSUMEVALID. For validation.cpp:

    0        } else if (it->second.nHeight < pindex->nHeight) {
    1            assume_valid = CHECKED_EXCEEDS_ASSUMEVALID;
    2        } else if (it->second.GetAncestor(pindex->nHeight) != pindex) {
    3            assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID;
    

    l0rinc commented at 8:57 pm on September 16, 2025:
    I’m not sure I fully understand - do you want me to add a different reason when the current block is valid, but just after the assumevalid block? That’s a very good idea, but I think we should do it separately since it modifies the behavior a tiny bit
  56. in src/validation.cpp:2456 in 9b04e4f962 outdated
    2472+        if (it == m_blockman.m_block_index.end()) {
    2473+            assume_valid = CHECKED_HASH_NOT_IN_HEADERS;
    2474+        } else if (it->second.GetAncestor(pindex->nHeight) != pindex) {
    2475+            assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID;
    2476+        } else if (m_chainman.m_best_header->GetAncestor(pindex->nHeight) != pindex) {
    2477+            assume_valid = CHECKED_OFF_BESTHEADER_PATH;
    


    hodlinator commented at 9:36 am on September 16, 2025:

    If we get here, we know from the previous check (if (it->second.GetAncestor(pindex->nHeight) != pindex) assume_valid = CHECKED_NOT_UNDER_ASSUMEVALID) that pindex now does exist below the assumevalid header (or is the assumevalid block).

    For the check against best header chain to fail for pindex, it could either be:

    1. The best header height somehow being below both pindex and assumevalid. (Edit: I guess this could theoretically happen if a really strong miner ramped up the difficulty and mined a fork of the chain from below the assumevalid height, but still with more total chain work).
    2. The best header chain being a totally different chain that doesn’t even include the assumevalid hash (unless someone has broken SHA256).

    Why would we want to do script checking of the pindex block which is in the chain below the assumevalid hash for any of those cases? This is keeping the existing behavior on master, but that behavior is weird to me.

    I guess this is also why you’ve had issues adding a test for CHECKED_OFF_BESTHEADER_PATH.


    hodlinator commented at 10:31 am on September 16, 2025:

    Found a way to test 2):

     0        # nodes[4]
     1        self.start_node(4, extra_args=["-assumevalid=" + block102.hash_hex])
     2        second_chain_tip = int(self.nodes[4].getbestblockhash(), 16)
     3        second_chain_time = self.nodes[4].getblock(self.nodes[4].getbestblockhash())['time'] + 1
     4        second_chain_height = self.nodes[4].getblock(self.nodes[4].getbestblockhash())['height'] + 1
     5
     6        second_chain = []
     7        for _ in range(2100):
     8            block = create_block(second_chain_tip, create_coinbase(second_chain_height), second_chain_time)
     9            block.solve()
    10            second_chain.append(block)
    11            second_chain_tip = block.hash_int
    12            second_chain_time += 1
    13            second_chain_height += 1
    14
    15        p2p4 = self.nodes[4].add_p2p_connection(BaseNode())
    16        p2p4.send_header_for_blocks(second_chain[0:2000])
    17        p2p4.send_header_for_blocks(second_chain[2000:])
    18        p2p4.send_header_for_blocks(self.blocks[0:103])
    19
    20        with self.nodes[4].assert_debug_log(expected_msgs=[f'Enabling signature validations at block [#1](/bitcoin-bitcoin/1/) ({block_1_hash}): block not on best header chain']):  # AssumeValid::CHECKED_OFF_BESTHEADER_PATH
    21            # Send all blocks to node1. All blocks will be accepted.
    22            p2p4.send_without_ping(msg_block(self.blocks[0]))
    23            self.wait_until(lambda: self.nodes[2].getblockcount() >= 1)
    

    l0rinc commented at 8:52 pm on September 16, 2025:
    fantastic, I had something similar but only got to 90%. Added the change as node 3 since it has a similar starting condition as the rest of the nodes, simplifies the header send to only create the needed headers and send all of them and fixed the waiting condition. Added you as coauthor.
  57. hodlinator commented at 9:58 am on September 16, 2025: contributor
    Reviewed 9b04e4f96200daf7516d1b8b6b0dfc4c077837e8
  58. DrahtBot requested review from hodlinator on Sep 16, 2025
  59. in test/functional/feature_assumevalid.py:186 in 4cd11cef0f outdated
    178@@ -170,5 +179,21 @@ def run_test(self):
    179         assert_equal(self.nodes[2].getblockcount(), COINBASE_MATURITY + 1)
    180 
    181 
    182+        # nodes[3]
    183+        p2p3 = self.nodes[3].add_p2p_connection(BaseNode())
    184+        p2p3.send_header_for_blocks(self.blocks[0:200])
    185+        p2p3.send_without_ping(msg_block(self.blocks[0]))
    186+        self.wait_until(lambda: self.nodes[3].getblockcount())
    


    TheCharlatan commented at 10:11 am on September 16, 2025:
    I’m a bit confused what the condition in this test is supposed to trigger if all we are testing are invalid args. What is the rationale here?

    l0rinc commented at 5:36 pm on September 16, 2025:
    The args are only invalid in relation to the blocks, so we need at least one block to invalidate the args.
  60. in src/validation.cpp:2597 in 9b04e4f962 outdated
    2595+                    pindex->nHeight, block_hash.ToString());
    2596+        } else {
    2597+            LogInfo("Enabling signature validations at block #%d (%s): %s.",
    2598+                    pindex->nHeight, block_hash.ToString(), assumevalid_reason(assume_valid));
    2599+        }
    2600+        m_prev_assume_valid_logged = assume_valid;
    


    TheCharlatan commented at 11:15 am on September 16, 2025:
    Are the enums really only added for logging? That seems a bit heavy. How about moving this if block up and making it a lambda that gets called directly from your if else branches? afaict that could remove the need for them, which judged by the comments left here so far, seem to confuse people a bit with their communicated intent.

    l0rinc commented at 8:55 pm on September 16, 2025:
    I have added a reason string with limited scope next to the bool, let me know what you think. Added you ad coauthor.
  61. DrahtBot requested review from TheCharlatan on Sep 16, 2025
  62. log: reword `signature validations` to `script verification` in `assumevalid` log
    Even though not all script verification is turned off currently (e.g. we're still doing the cheaper sigop counts), this naming is more consistent with other usages.
    788e45a5b2
  63. test: add most of the missing assumevalid conditions
    Covers the assumevalid gating permutations across five nodes:
    * Node 0 (no -assumevalid): baseline; rejects block 102 (invalid signature) while syncing from genesis.
    * Node 1 (-assumevalid=<block102>, deeply buried): verifies that script checks are disabled at height 1 while syncing the full chain to 2202.
    * Node 2 (-assumevalid=<block102>, not buried enough): feeds only the first ~200 blocks; still rejects block 102, demonstrating that checks remain enabled when the anchor isn’t yet sufficiently buried.
    * Node 3 (-assumevalid=<block102>, off best‑header path): preloads a longer competing headers‑only branch, then our chain, exercising the "not on best header path" gate (anchor’s path is not the best‑header path).
    * Node 4 (reindex cases):
      -reindex-chainstate -assumevalid=<invalid-hash> to exercise "assumevalid hash not in headers".
      -reindex-chainstate -assumevalid=<block102> -minimumchainwork=0xffff to exercise "below minimum chainwork".
    9ecaadc94a
  64. validation: log initial signature verification state
    Replaced the atomic cache with `std::optional<AssumeValid>` and will log on first observation. This is safe because `ConnectBlock` already holds `cs_main`.
    The enum acts as a boolean in this change and will gain more states in follow-up commits.
    
    After this change, the state is logged before the very first `UpdateTip` line.
    
    Instead of true/false or enabled/disabled, the checked/skipped naming should help with understanding when signature verification is actually performed.
    
    Follow-up to https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2329269037
    
    Co-authored-by: Eunovo <eunovo9@gmail.com>
    Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    Co-authored-by: w0xlt <woltx@protonmail.com>
    b577becd71
  65. refactor: untangle `assume_valid` branches
    Flatten nested conditionals into a linear gating sequence for readability and precise logging. No functional change, TODOs are addressed in next commit
    37760a8337
  66. l0rinc force-pushed on Sep 16, 2025
  67. l0rinc commented at 9:03 pm on September 16, 2025: contributor

    Thanks for the reviews, I have simplified the commits, removed the enum and replaced it with direct reason string, added an off best‑header path (thanks to @hodlinator), reworded the log to say “script verification” instead of “signature validations” to be in alignment with the “Script verification uses … additional threads” and reverted the fScriptChecks and m_prev_script_checks_logged symbols.

    There’s a remaining suggestion to have friendlier reason in case of:

    02025-09-16T21:00:03Z Enabling script verification at block [#915016](/bitcoin-bitcoin/915016/) (000000000000000000001a6f6115c32c340a2159b30f9ffd632f63424aa563f9): block not part of assumevalid chain.
    

    to separate when the block has the assumevalid as an ancestor. I found that a bit riskier change than the rest, I think we should do that in a follow-up instead.

  68. l0rinc requested review from w0xlt on Sep 16, 2025
  69. log: separate signature verification reasons
    Introduced explicit `AssumeValid` reasons for the checks-enabled cases.
    
    Updated logging to print the reason when enabling; disabling prints without a reason.
    Adjusted the functional test to assert the new reason strings.
    
    No functional change to gating logic, logging only.
    
    Co-authored-by: w0xlt <woltx@protonmail.com>
    Co-authored-by: Eunovo <eunovo9@gmail.com>
    Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    Co-authored-by: TheCharlatan <seb.kung@gmail.com>
    f6951cb74b
  70. l0rinc force-pushed on Sep 16, 2025

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-09-17 03:12 UTC

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