Add checkBlock() to Mining interface #31981

pull Sjors wants to merge 4 commits into bitcoin:master from Sjors:2025/03/checkBlock changing 15 files +452 −119
  1. Sjors commented at 11:04 am on March 4, 2025: member

    This PR adds the IPC equivalent of the getblocktemplate RPC in proposal mode.

    In order to do so it has TestBlockValidity return error reasons as a string instead of BlockValidationState. This avoids complexity in IPC code for handling the latter struct.

    The new Mining interface method is used in miner_tests and the getblocktemplate and generateblock RPC calls, so it has test coverage.

    The inconclusive-not-best-prevblk check is moved from RPC code to TestBlockValidity.

    Test coverage is increased by mining_template_verification.py.

    Superseedes #31564

    Background

    Verifying block templates (no PoW)

    Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.1 This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation2.

    The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.3. It could use the getblocktemplate RPC in proposal mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.

    (although SRI could use this PR, the Template Provider role doesn’t need it, so this is not part of #31098)

    1. https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md ↩︎

    2. https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors ↩︎

    3. https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196 ↩︎

  2. DrahtBot commented at 11:04 am on March 4, 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/31981.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Stale ACK ryanofsky

    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:

    • #32317 (kernel: Separate UTXO set access from validation functions by TheCharlatan)
    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

    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.

  3. Sjors commented at 11:39 am on March 4, 2025: member
    I also salvaged the functional tests from 15d841e41bb61f435dd3e60e99fa4db47d34a3a6.
  4. laanwj added the label Mining on Mar 4, 2025
  5. DrahtBot added the label Needs rebase on Mar 12, 2025
  6. Sjors force-pushed on Mar 13, 2025
  7. Sjors commented at 8:30 am on March 13, 2025: member
    Rebased after #31283.
  8. DrahtBot removed the label Needs rebase on Mar 13, 2025
  9. DrahtBot added the label Needs rebase on Mar 14, 2025
  10. Sjors force-pushed on Mar 14, 2025
  11. Sjors commented at 9:03 am on March 14, 2025: member
    Rebased after #31974 (trivial conflict with 3b33fd9424a2b4edbdc1745339e3541ab4b90af5).
  12. DrahtBot removed the label Needs rebase on Mar 14, 2025
  13. ryanofsky requested review from Copilot on Apr 8, 2025
  14. Copilot commented at 8:40 pm on April 8, 2025: none

    Copilot reviewed 13 out of 15 changed files in this pull request and generated 1 comment.

    • src/ipc/capnp/mining.capnp: Language not supported
    • src/test/ipc_test.capnp: Language not supported

    src/node/interfaces.cpp:1098

    • Correct the comment formatting by removing the ‘=’ in ‘/=check_merkle_root=/’ to read ‘/check_merkle_root=/’ for clarity.
    0return chainman().TestBlockValidity(block, reason, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root);
  15. in src/node/types.h:91 in e6e170cf6c outdated 
    85@@ -85,6 +86,17 @@ struct BlockWaitOptions {
86     CAmount fee_threshold{MAX_MONEY};
87 };
88 
89+struct BlockCheckOptions {
90+    /**
91+     * Set false to omit the merkle root heck
    Copilot commented at 8:40 pm on April 8, 2025:

    Typo in the comment: change ‘heck’ to ‘check’.

    0     * Set false to omit the merkle root check
    l0rinc commented at 8:42 pm on April 8, 2025:
    Can we turn this off?
    maflcko commented at 6:38 am on April 9, 2025:

    Can we turn this off?

    Probably best discussed in a separate brainstorming issue, but if this can find typos without false positives before real review and before merge, it seems acceptable. Obviously it can’t do any real review and this is the first time I’ve seen it used, so I am likely missing downsides.

    ryanofsky commented at 4:47 pm on April 9, 2025:

    re: #31981 (review)

    I actually requested the review since I was curious what it would do. It seems a bit afraid to be opinionated though and might be more useful if we added guidelines https://docs.github.com/en/enterprise-cloud@latest/copilot/using-github-copilot/code-review/configuring-coding-guidelines

    Sjors commented at 11:04 am on April 25, 2025:
    Fixed typo since I had to retouch anyway.
  16. in src/rpc/mining.cpp:744 in 1d029c23a1 outdated 
    746-            }
747-            BlockValidationState state;
748-            TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/true);
749-            return BIP22ValidationResult(state);
750+            std::string reason;
751+            bool res{chainman.TestBlockValidity(block, reason, /*check_pow=*/false, /*check_merkle_root=*/true)};
    ryanofsky commented at 4:01 pm on April 9, 2025:

    In commit “validation: refactor TestBlockValidity” (1d029c23a143f1b9110377967a7c4a20ee75058d)

    it seems like there could be change in behavior here since BIP22ValidationResult would previously throw an exception on error and now it will just return an error string.

    Sjors commented at 11:04 am on April 25, 2025:

    Good point. The original code would return a string in some cases and throw an error in others. Although it’s possible nobody uses the getblocktemplate RPC in proposal mode at all, there could be (unmaintained) proprietary software out there using it. So the safest option is to maintain the behavior.

    However it seems there’s no change in behavior: BIP22ValidationResult only throws for state.IsError(), not for state.IsInvalid(). I looked through CheckBlock and the functions it calls and none of those can set state.IsError().

    I expanded the commit message to explain this.

  17. in src/rpc/mining.cpp:391 in 1d029c23a1 outdated 
    385@@ -386,9 +386,9 @@ static RPCHelpMan generateblock()
386         block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
387         RegenerateCommitments(block, chainman);
388 
389-        BlockValidationState state;
390-        if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
391-            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString()));
392+        std::string reason;
393+        if (!chainman.TestBlockValidity(block, reason, /*check_pow=*/false, /*check_merkle_root=*/false)) {
394+            throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", reason));
    ryanofsky commented at 4:10 pm on April 9, 2025:

    In commit “validation: refactor TestBlockValidity” (1d029c23a143f1b9110377967a7c4a20ee75058d)

    There seems to be a change in behavior here if ValidationState::m_debug_message is set because now it would previous be included in ToString output but now will be dropped.

    Sjors commented at 12:14 pm on April 25, 2025:
    I changed TestBlockValidity to also take a debug argument.
  18. in src/validation.cpp:4644 in 1d029c23a1 outdated 
    4646-                       Chainstate& chainstate,
4647-                       const CBlock& block,
4648-                       CBlockIndex* pindexPrev,
4649-                       bool fCheckPOW,
4650-                       bool fCheckMerkleRoot)
4651+bool ChainstateManager::TestBlockValidity(const CBlock& block, std::string& reason, const bool check_pow, const bool check_merkle_root)
    ryanofsky commented at 4:20 pm on April 9, 2025:

    In commit “validation: refactor TestBlockValidity” (1d029c23a143f1b9110377967a7c4a20ee75058d)

    Most of the changes here seem good, but there are a few things I think could be improved:

    • It seems better to me if this would return a BlockValidationState object instead of a string and a bool, so the valid/invalid/error states could be clearly distinguished, debug information could be returned, and this would fit in better with other validation.h functions using BlockValidationState. Returning BlockValidationState would also let callers use the BIP22ValidationResult univalue conversion function so we could reduce inconsistencies in how RPCs return validation information.
    • Also think it would be good if this function still took a chainstate parameter, since it shouldn’t care about which chainstate is active and could work on any chainstate.
    • I think it would be better if this remained a standalone function instead of becoming a ChainstateManager method since it shouldn’t need access to private ChainstateManager state and we might want to move it out of validation.cpp at some point to someplace like node/miner.cpp since it is only used by mining code.
    • Probably also better for it not to LOCK(cs_main) to avoid unnecessary recursive mutex locking.
    • Using Assert() consistently here would seem better than using a mix of Assert and Assume. Or if Assume() should be used it would be better to check return value and return an error status, instead of the ignoring failures in release builds.

    Made a commit with all these changes which looks like: bf15692ab9b5f8d68cb84e925c067caebe03efba (branch)

    Sjors commented at 11:15 am on April 25, 2025:
    Making this return a BlockValidationState means we have to pass it over the interface, which means we can’t drop the special handling, see cca5993b26e6223af31fe1ef5bf8a319cb87cf93.
    Sjors commented at 1:27 pm on April 25, 2025:

    None of the Assume checks seem problematic in production. They would just be inconsistent with (my understanding of) previous behavior. I dropped them, since none of the tests and fuzzers tripped over them.

    I dropped the !block.fChecked Assert. The remaining Assert`s are where we would crash anyway.

    Finally I changed the pre-existing assert at the end into an Assume along with a LogError.

  19. ryanofsky approved
  20. ryanofsky commented at 5:00 pm on April 9, 2025: contributor
    Code review ACK e6e170cf6c67a56b9c14cece66fdc4fab5f3ec6b. Change is nicely implemented and background in the PR description is very helpful. Am a little concerned about possible changes in RPC behavior though and left some comments below.
  21. Sjors commented at 8:22 am on April 22, 2025: member
    Note to self, figure out if the sigops check needs to be fixed here or in another PR: https://github.com/bitcoin/bitcoin/pull/31624/files#r2053597806
  22. Sjors force-pushed on Apr 25, 2025
  23. Sjors commented at 12:16 pm on April 25, 2025: member

    Rebased, fixed typo found by LLM, clarified non behavior change for proposal mode.

    I added a debug argument to pass along more detailed failure reasons.

    I’m a bit on the fence regarding this suggestion: #31981 (review)

    figure out if the sigops check needs to be fixed here

    No it doesn’t. TestBlockValidity first calls CheckBlock which performs the limited sigops check documented in #31624. Later it calls ConnectBlock() which does the thorough check.

  24. maflcko commented at 12:49 pm on April 25, 2025: member

    I’ve implemented a typo check in DrahtBot and there are more, see #31981 (comment)

    Also, the CI is failing.

  25. DrahtBot added the label CI failed on Apr 25, 2025
  26. DrahtBot commented at 12:50 pm on April 25, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/41155656918

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  27. Sjors force-pushed on Apr 25, 2025
  28. Sjors commented at 1:27 pm on April 25, 2025: member

    Forgot to adjust the capnp file. Fixed more typos.

    I moved TestBlockValidity out of ChainstateManager again, and pass in a chainstate parameter as suggested in #31981 (review).

    Probably also better for it not to LOCK(cs_main) to avoid unnecessary recursive mutex locking.

    Can you elaborate on this?

    I also moved the TestBlockValidity arguments around a bit to reduce churn.

  29. validation: refactor TestBlockValidity 
    A later commit adds checkBlock() to the Mining interface. In order to
avoid passing BlockValidationState over IPC, this commit first
refactors TestBlockValidity to return a boolean instead, and pass failure
reasons via a string.

Comments are expanded.

The ContextualCheckBlockHeader check is moved to after CheckBlock,
which is more similar to normal validation where context-free checks
are done first.

Validation failure reasons are no longer printed through LogError(),
since it depends on the caller whether this implies an actual bug
in the node, or an externally sourced block that happens to be invalid.
When called from getblocktemplate, via BlockAssembler::CreateNewBlock(),
this method already throws an std::runtime_error if validation fails.

Additionally it moves the inconclusive-not-best-prevblk check from RPC
code to TestBlockValidity.

There is no behavior change when callling getblocktemplate with proposal.
Previously this would return a BIP22ValidationResult which can throw for
state.IsError(). But CheckBlock() and the functions it calls only use
state.IsValid().

The final assert is changed into Assume, with a LogError.
    a0a6dbbe75
  30. ipc: drop BlockValidationState special handling 
    The Mining interface avoids using BlockValidationState.
    9822bd64d2
  31. Add checkBlock to Mining interface 
    And use it in miner_tests, getblocktemplate and generateblock.
    6176f9b4c4
  32. test: more template verification tests c1939c43c3
  33. Sjors force-pushed on Apr 25, 2025
  34. DrahtBot removed the label CI failed on Apr 25, 2025


Sjors DrahtBot Copilot l0rinc maflcko ryanofsky

Labels
Mining

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-04-28 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me