Add checkblock RPC and checkBlock() to Mining interface #31564

pull Sjors wants to merge 5 commits into bitcoin:master from Sjors:2024/12/check-block changing 15 files +453 −66
  1. Sjors commented at 5:07 pm on December 24, 2024: member

    Rationale

    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 There’s currently no way for it to do so, short of re-implementing Bitcoin Core validation code.

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

    Verifying weak blocks (reduced PoW)

    Stratum v2 decentralises block template generation, but still hinge on a centralised entity to approve these templates.

    There used to be a decentralised mining protocol P2Pool4 which relied on (what we would today call) weak blocks. In such a system all participants perform verification and there is no privileged pool operator (that can reject a template).

    P2Pool shares form a “sharechain” with each share referencing the previous share’s hash. Each share contains a standard Bitcoin block header, some P2Pool-specific data that is used to compute the generation transaction (total subsidy, payout script of this share, a nonce, the previous share’s hash, and the current target for shares), and a Merkle branch linking that generation transaction to the block header’s Merkle hash.

    IIUC only the header and coinbase of these shares / weak blocks were distributed amongst all pool participants and verified. This was sufficient at the time because fees were negligible, so payouts could simply be distributed based on work.

    However, if P2Pool were to be resurrected today5, it would need to account for fees in a similar manner as PPLNS2 above. In that case the share chain should include the full weak blocks6. The client software would need a way to verify those weak blocks.

    A similar argument applies to Braidpool.

    Enter checkblock and its IPC counterpart checkBlock()

    Given a serialised block, it performs the same checks as submitblock, but without updating the block index, chainstate, etc. The proof-of-work check can be bypassed entirely, for the first use of verifying block templates. Alternatively a custom target can be provided for the use case of weak blocks.

    When called with check_pow=false the checkblock RPC is (almost) the same as getblocktemplate in proposal mode, and indeed they both call checkBlock() on the Mining interface.

    Implementation details:

    Builds on:

    The ChainstateManager::CheckNewBlock() implementation is based on ProcessNewBlock(), but refactored in a number of ways (see https://github.com/Sjors/bitcoin/pull/75 for previous incarnations):

    • it drops the use of a StateCatcher
    • it requires building (directly) on the tip
    • if calls CheckWeakProofOfWork before the other checks
    • instead of AcceptBlock() it performs a (documented) subset of that function
    • it creates a throwaway CCoinsViewCache on top of the tip on which it connects the block (needed for full transaction verification)
    • it updates validation caches (and does not delete them). This should make subsequent validation (of a real block) faster if it contains overlapping transactions that are not in our mempool.

    This CheckNewBlock() method is then called from the Mining interface createBlock() method. This in turn is called from the RPC checkblock code, which is a simplified clone of submitblock.

    This method ended up being very similar to TestBlockValidity, so the last commit replaces the latter entirely (renaming CheckNewBlock).

    This PR contains a simple unit test and a more elaborate functional test.

    Potential followups:

    • if the block contains “better” transactions, (optionally) add them to our mempool 5
    • keep track of non-standard transactions7
    • allow rollback (one or two blocks) for pools to verify stale work / uncles

    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 ↩︎

    4. https://en.bitcoin.it/wiki/P2Pool ↩︎

    5. this improves compact block relay once the real block comes in ↩︎ ↩︎

    6. The share chain client software could use compact block encoding to reduce its orphan / uncle rate. To make that easier, we could provide a reconstructblock RPC (reconstructBlock IPC) that takes the header and short ids, fills in a CBlock from the mempool (and jail) and returns a list of missing items. The share chain client then goes and fetches missing items and calls the method again. It then passes the complete block to checkblock. This avoids the need for others to fully implement compact block relay. Note that even if we implemented p2p weak block relay, it would not be useful for share chain clients, because they need to relay additional pool-specific data. ↩︎

    7. https://delvingbitcoin.org/t/second-look-at-weak-blocks/805 ↩︎

  2. DrahtBot commented at 5:07 pm on December 24, 2024: 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/31564.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK tdb3

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

    Conflicts

    No conflicts as of last run.

  3. Sjors commented at 5:09 pm on December 24, 2024: member

    I wrote on IRC:

    Does anyone know what UpdateUncommittedBlockStructures is good for in the submitblock RPC?

    To which @sipa responded:

    Sjors[m]: it looks like it adds the “witness nonce” if missing (the witness for the coinbase input is a reserved field for adding future commitments, but it must be present, so that function adds a 0)

    This PR omits UpdateUncommittedBlockStructures. It would be nice to come up with a test that shows what it does and fails on the current code.

  4. mzumsande commented at 8:51 pm on December 26, 2024: contributor
    The newly introduced CheckNewBlock() looks very similar to the existing TestBlockValidity() which was just removed from the mining interface in bfc4e029d41ec3052d68f174565802016cb05d41 - it just seems to be a bit more verbose and allows the multiplier for the PoW. Did you consider merging these functions, so that we don’t have multiple functions in validation that basically do the same thing?
  5. in src/pow.cpp:165 in 9887db317c outdated
    160@@ -161,3 +161,19 @@ bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Par
    161 
    162     return true;
    163 }
    164+
    165+bool CheckWeakProofOfWork(uint256 hash, unsigned int nBits, unsigned int multiplier, const Consensus::Params& params)
    


    tdb3 commented at 11:13 pm on December 26, 2024:
    Feel free to ignore before more concept acks arrive. CheckWeakProofOfWork() seems really similar to CheckProofOfWorkImpl(). Maybe I’m missing something simple. What was the rationale for creating the new function vs adding an argument?

    Sjors commented at 2:31 pm on December 27, 2024:
    Just that I didn’t want to touch CheckProofOfWorkImpl.

    Sjors commented at 5:01 pm on December 30, 2024:
    This also applies to the new DeriveTarget method. I added the motivation for not changing CheckProofOfWorkImpl to the commit message in: https://github.com/bitcoin/bitcoin/pull/31564/commits/d3710b0ffe416337cd2c21ba871348b0e7b4aa9e
  6. tdb3 commented at 0:21 am on December 27, 2024: contributor

    Concept ACK

    Really liking the idea of facilitating these type of systems.

    Instead of a multiplier I could also allow a custom target. This would give weak block systems more flexibility on how to derive their difficulty.

    I’d be in favor of this. It’s more flexible, could prevent interface churn, and if the user wishes to create the custom target from a simple multiplication they could do the multiplication on their end.

  7. Sjors commented at 2:32 pm on December 27, 2024: member

    The newly introduced CheckNewBlock() looks very similar to the existing TestBlockValidity()

    It looks like I ended up with a similar design after a long detour. I’ll look into combining these.

    Instead of a multiplier I could also allow a custom target @instagibbs is there any reason you went for a multiplier?

  8. Sjors force-pushed on Dec 30, 2024
  9. instagibbs commented at 4:47 pm on December 30, 2024: member

    is there any reason you went for a multiplier?

    nothing off the top of my head

  10. Sjors commented at 4:48 pm on December 30, 2024: member

    I refactored checkblock to take a target argument rather than a multiplier. In order to make this easier to implement and test, I also introduced a gettarget RPC and DeriveTarget() helper.

    I then dropped the original TestBlockValidity and renamed CheckNewBlock to it. The generateblock and getblocktemplate RPC calls, as well as BlockAssembler::CreateNewBlock use the new method.

    If people prefer I could rewrite the git history to directly refactor TestBlockValidity, though I suspect the current approach is easier to review.

    I split the test changes out into #31581, so will mark this PR as draft for now. While working on that I noticed a rather big difference between the original TestBlockValidity and my incarnation of it: the former required holding cs_main, which I think was wrong.

  11. Sjors force-pushed on Dec 30, 2024
  12. DrahtBot added the label CI failed on Dec 30, 2024
  13. DrahtBot commented at 4:58 pm on December 30, 2024: contributor

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

    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.

  14. Sjors force-pushed on Dec 30, 2024
  15. Sjors force-pushed on Dec 30, 2024
  16. Sjors force-pushed on Dec 30, 2024
  17. Sjors marked this as a draft on Dec 30, 2024
  18. Sjors commented at 6:27 pm on December 30, 2024: member

    I might make a separate PR to consider whether to drop AssertLockHeld(cs_main) from the original TestBlockValidity.

    If that ends up rejected, then I’ll keep the existing TestBlockValidity calls as they are, i.e. drop the last two commits from this PR and leave them as a followup.

    Related: #31563


    While working on that I noticed a rather big difference between the original TestBlockValidity and my incarnation of it: the former required holding cs_main, which I think was wrong.

    Update: I was confused, CheckBlock needs to be called with a lock on cs_main, and there’s no need to “drop AssertLockHeld(cs_main)”. That should allow for some simplifications.

  19. DrahtBot added the label Needs rebase on Dec 30, 2024
  20. Sjors force-pushed on Dec 30, 2024
  21. Sjors commented at 8:13 pm on December 30, 2024: member

    Untangled the mess and rebased after #31562.

    Also split out #31583.

  22. DrahtBot removed the label Needs rebase on Dec 30, 2024
  23. DrahtBot removed the label CI failed on Dec 31, 2024
  24. achow101 referenced this in commit c506f2cee7 on Jan 6, 2025
  25. DrahtBot added the label Needs rebase on Jan 6, 2025
  26. luke-jr commented at 6:15 am on January 7, 2025: member

    The new RPC interface is redundant with the existing BIP 23 block proposals support.

    P.S. DATUM does not provide a way for the pool to reject valid blocks. There is no approval of templates.

  27. consensus: add DeriveTarget() to pow.h
    Split CheckProofOfWorkImpl() to introduce a helper function
    DeriveTarget() which converts the nBits value to the target.
    
    The function takes pow_limit as an argument so later commits can
    avoid having to pass ChainstateManager through the call stack.
    
    Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
    bd28370957
  28. Sjors force-pushed on Jan 7, 2025
  29. Sjors commented at 8:36 am on January 7, 2025: member

    Rebased after #31581 landed. I also cherry-picked the latest relevant commits from #31583. @luke-jr I dropped DATUM from the description, since there’s still no spec it’s hard to understand what it actually does.

    The checkblock RPC is indeed almost the same as getblocktemplate in proposal mode. I updated the description to point this out. The main difference is that it can check (reduced) proof-of-work.

    My long term goal for this new method is to make it more powerful. It could optionally add new transactions to the mempool to improve compact block relay. It could perform small reorgs to validate forks, which can be useful for (stale) share accounting (and perhaps uncle and DAG schemes).

    An alternative approach could be drop the checkblock RPC and leave (weak) PoW checking as an IPC-only feature. If people prefer that I can convert the tests in rpc_checkblock.py to instead increase coverage for getblocktemplate.

    But I think it’s nice to offer weak block checking support for mining pool projects that want to experiment with it, without forcing them to use IPC. The new RPC method is short and test covered.

    (in the above I’m assuming that getblocktemplate should be considered more or less ossified)

  30. Sjors force-pushed on Jan 7, 2025
  31. DrahtBot added the label CI failed on Jan 7, 2025
  32. DrahtBot commented at 8:52 am on January 7, 2025: contributor

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

    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.

  33. DrahtBot removed the label Needs rebase on Jan 7, 2025
  34. validation: CheckNewBlock() 9cc580d9b2
  35. Add checkBlock() to Mining interface 953a4bd591
  36. Sjors force-pushed on Jan 7, 2025
  37. Sjors commented at 11:02 am on January 7, 2025: member
    The weak block test was broken, because regtest uses the almost highest possible target. Using a multiplier would overflow it. I dropped 180ce81182e840f01e6e9534872ebf92647c2e39 and adjusted the tests. I also dropped b61f8cca13320088a83aadef5f6af66375342fae and b74e145bc46e6dbf7d1496f078ff22cf80d8c92a because I don’t need them for the test.
  38. rpc: add checkblock 20521415fa
  39. validation: replace TestBlockValidity
    Delete the original TestBlockValidity and rename CheckNewBlock.
    
    Have the generateblock and getblocktemplate RPC calls, as well
    as BlockAssembler::CreateNewBlock  use the new method.
    8cd4e51beb
  40. Sjors force-pushed on Jan 7, 2025
  41. DrahtBot removed the label CI failed on Jan 7, 2025
  42. luke-jr commented at 12:28 pm on January 7, 2025: member

    What would use the “weak block” check that doesn’t need the ability to do it independently from proposals anyway?

    While the existing GBT BIPs are Final, the protocol was designed to allow for extensions. If there’s a use case for this feature, doing it that way seems fine

  43. Sjors commented at 12:45 pm on January 7, 2025: member

    @luke-jr do you mean that every client that wants to call checkblock $block_hex or getblocktemplate '{"mode": "proposal", "data": "$block_hex"}' for a weak block, probably implements its own proof-of-work check? That’s probably true, since it most likely needs the ability to parse the block anyway in order to inspect the coinbase. Compared to that calculating proof-of-work and comparing against a target is trivial.

    What would an extension look like to pass check_pow and target arguments?


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-01-21 09:12 UTC

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