rpc: Add submitheader #13399

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:Mf1806-rpcBlockHeader changing 4 files +128 −6
  1. MarcoFalke commented at 5:07 PM on June 5, 2018: member

    This exposes ProcessNewBlockHeaders as an rpc called submitheader. This can be used to check for invalid block headers and submission of valid block headers via the rpc.

  2. MarcoFalke added the label RPC/REST/ZMQ on Jun 5, 2018
  3. DrahtBot commented at 5:23 PM on June 5, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #13945 (Refactoring CRPCCommand with enum category by isghe)
    • #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)

    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. laanwj commented at 5:27 PM on June 5, 2018: member

    Your OP is empty - can you provide rationale what this is to be used for?

  5. TheBlueMatt commented at 5:31 PM on June 5, 2018: member

    I asked for this as a part of my BetterHash mining protocol work, however I've also wanted this at several points in order to be able to implement chain-sync logic outside of bitcoind over RPC. You'd need this or something like it to do headers-first sync in such a system.

  6. in src/validation.h:224 in 1c4268a500 outdated 
     233 | @@ -234,7 +234,6 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
 234 |   * (and possibly also) BlockChecked will have been called.
 235 |   *
 236 |   *
 237 | - *
    promag commented at 5:34 PM on June 5, 2018:

    Commit "rpc: Expose ProcessNewBlockHeaders"

    This should be in commit "doc: Rewrite some validation doc to be machine-readable:".

  7. in src/validation.h:248 in 1c4268a500 outdated 
     243 | @@ -245,8 +244,6 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
 244 |  /**
 245 |   * Process incoming block headers.
 246 |   *
 247 | - *
    promag commented at 5:34 PM on June 5, 2018:

    Commit "rpc: Expose ProcessNewBlockHeaders"

    This should be in commit "doc: Rewrite some validation doc to be machine-readable:".

  8. in src/validation.h:224 in 7f619acaf0 outdated 
     231 | @@ -232,28 +232,25 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
 232 |   *
 233 |   * Note that we guarantee that either the proof-of-work is valid on pblock, or
 234 |   * (and possibly also) BlockChecked will have been called.
 235 | - * 
 236 | - * Call without cs_main held.
 237 | + *
    promag commented at 5:36 PM on June 5, 2018:

    Could be removed?

  9. promag commented at 5:38 PM on June 5, 2018: member

    Is there a reason to include #13395?

  10. MarcoFalke force-pushed on Jun 5, 2018
  11. MarcoFalke force-pushed on Jun 5, 2018
  12. MarcoFalke force-pushed on Jun 5, 2018
  13. in test/functional/mining_basic.py:144 in 2595028d97 outdated 
     140 | @@ -131,5 +141,61 @@ def run_test(self):
 141 |          bad_block.hashPrevBlock = 123
 142 |          assert_template(node, bad_block, 'inconclusive-not-best-prevblk')
 143 |  
 144 | +        self.log.info('submitheader tests')
 145 | +        assert_raises_rpc_error(-22, 'Block header decode failed', lambda: node.submitheader(hexdata='xx' * 80))
 146 | +        assert_raises_rpc_error(-22, 'Block header decode failed', lambda: node.submitheader(hexdata='ff' * 78))
 147 | +        assert_raises_rpc_error(-25, 'Must submit previous header', lambda: node.submitheader(hexdata='ff' * 80))
    promag commented at 8:45 AM on June 11, 2018:

    nit, could include the previous hash.

  14. in src/rpc/mining.cpp:789 in 2595028d97 outdated 
     783 | +    if (!DecodeHexBlockHeader(h, request.params[0].get_str())) {
 784 | +        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block header decode failed");
 785 | +    }
 786 | +    {
 787 | +        LOCK(cs_main);
 788 | +        if (LookupBlockIndex(h.GetHash())) return NullUniValue;
    promag commented at 8:51 AM on June 11, 2018:

    These verifications are done in ProcessNewBlockHeaders -> CChainState::AcceptBlockHeader. Could use ProcessNewBlockHeaders return value.

    jnewbery commented at 3:19 PM on June 11, 2018:

    I think it'd be slightly nicer to have a return value passed to the user, if only to differentiate between whether the header was already in the block index or if it newly added to the block index.

    promag commented at 3:54 PM on June 11, 2018:

    @jnewbery do you see a use case for that?

    MarcoFalke commented at 3:20 PM on July 11, 2018:

    See the docstring above: The result is always None and not the return value of PNBH

  15. in src/rpc/mining.cpp:794 in 2595028d97 outdated 
     790 | +            throw JSONRPCError(RPC_VERIFY_ERROR, "Must submit previous header (" + h.hashPrevBlock.GetHex() + ") first");
 791 | +        }
 792 | +    }
 793 | +
 794 | +    CValidationState state;
 795 | +    ProcessNewBlockHeaders({h}, state, Params(), /* ppindex */ nullptr, /* first_invalid */ nullptr);
    promag commented at 8:52 AM on June 11, 2018:

    Since the lock is released above, there is a point where the block can be processed in between.

    MarcoFalke commented at 3:27 PM on July 11, 2018:

    That race shouldn't affect the return value.

  16. in src/validation.h:235 in 2595028d97 outdated 
     231 | @@ -232,28 +232,24 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
 232 |   *
 233 |   * Note that we guarantee that either the proof-of-work is valid on pblock, or
 234 |   * (and possibly also) BlockChecked will have been called.
 235 | - *
    jnewbery commented at 2:42 PM on June 11, 2018:

    supernit: you remove this text in commit doc: Rewrite some validation doc to be machine-readable, and then remove the lines in commit rpc: Expose ProcessNewBlockHeaders. Just remove the lines in the first commit.

    Same for ProcessNewBlockHeaders comment below.

  17. in test/functional/mining_basic.py:30 in 2595028d97 outdated 
      28 | +    assert_equal,
  29 | +    assert_raises_rpc_error,
  30 | +    bytes_to_hex_str,
  31 | +)
  32 | +
  33 | +b2x = bytes_to_hex_str
    jnewbery commented at 2:43 PM on June 11, 2018:

    Can just use:

    from test_framework.util import (
    assert_equal,
    assert_raises_rpc_error,
    bytes_to_hex_str as b2x,
+)
  18. in test/functional/mining_basic.py:180 in 2595028d97 outdated 
     178 | +        assert_equal(node.submitblock(hexdata=b2x(bad_block.serialize())), 'bad-txns-nonfinal')
 179 | +        # Build a "good" block on top of the submitted bad block
 180 | +        bad_block2 = copy.deepcopy(block)
 181 | +        bad_block2.hashPrevBlock = bad_block.sha256
 182 | +        bad_block2.solve()
 183 | +        assert_raises_rpc_error(-25, 'bad-prevblk', lambda: node.submitheader(hexdata=b2x(CBlockHeader(bad_block2).serialize())))
    jnewbery commented at 2:53 PM on June 11, 2018:

    Normal way we do this is:

            assert_raises_rpc_error(-25, 'bad-prevblk', node.submitheader, b2x(CBlockHeader(bad_block2).serialize()))

    Any reason you've chosen to use a lambda?

    MarcoFalke commented at 3:43 PM on June 11, 2018:

    I prefer to use named arguments

    promag commented at 3:52 PM on June 11, 2018:

    This should work:

    assert_raises_rpc_error(-25, 'bad-prevblk', node.submitheader, hexdata=b2x(CBlockHeader(bad_block2).serialize()))
  19. in test/functional/mining_basic.py:156 in 2595028d97 outdated 
     154 | +        assert chain_tip(block.hash) not in node.getchaintips()
 155 | +        node.submitheader(hexdata=b2x(block.serialize()))
 156 | +        assert chain_tip(block.hash) in node.getchaintips()
 157 | +        node.submitheader(hexdata=b2x(CBlockHeader(block).serialize()))  # Noop
 158 | +        assert chain_tip(block.hash) in node.getchaintips()
 159 | +
    jnewbery commented at 3:28 PM on June 11, 2018:

    perhaps test submitting a block header that isn't the most-work tip (ie a fork from some earlier block)

    MarcoFalke commented at 3:56 PM on July 11, 2018:

    Done at the end of this function

  20. jnewbery commented at 3:29 PM on June 11, 2018: member

    Looks great. utACK 2595028d97fc08ea0d27c3133c44d63f3e8bbc0f

    A few nits inline. Feel free to ignore.

  21. DrahtBot added the label Needs rebase on Jun 15, 2018
  22. jnewbery commented at 4:31 PM on June 20, 2018: member

    #13439 has been merged. Is this PR ready for rebase and rereview?

  23. MarcoFalke force-pushed on Jun 26, 2018
  24. MarcoFalke removed the label Needs rebase on Jun 26, 2018
  25. DrahtBot added the label Needs rebase on Jul 7, 2018
  26. MarcoFalke force-pushed on Jul 11, 2018
  27. DrahtBot removed the label Needs rebase on Jul 11, 2018
  28. MarcoFalke force-pushed on Jul 11, 2018
  29. MarcoFalke force-pushed on Jul 11, 2018
  30. MarcoFalke force-pushed on Jul 11, 2018
  31. sipa commented at 10:12 PM on July 13, 2018: member

    utACK fa7d7dd34cbd180ec9587c640078dfb7bf2ead04.

    Having this functionality can't hurt, as it available with identical semantics via P2P anyway.

    I have no opinion about the test code style.

  32. in test/functional/mining_basic.py:175 in fa7d7dd34c outdated 
     170 | +        bad_block_lock = copy.deepcopy(block)
 171 | +        bad_block_lock.vtx[0].nLockTime = 2**32 - 1
 172 | +        bad_block_lock.vtx[0].rehash()
 173 | +        bad_block_lock.hashMerkleRoot = bad_block_lock.calc_merkle_root()
 174 | +        bad_block_lock.solve()
 175 | +        assert_equal(node.submitblock(hexdata=b2x(bad_block_lock.serialize())), 'invalid')
    luke-jr commented at 5:32 PM on July 14, 2018:

    Does master have a bug? This should be returning "bad-txns-nonfinal", not "invalid"...

  33. luke-jr commented at 5:33 PM on July 14, 2018: member

    Please rename either the PR or the RPC method to match the other...

  34. MarcoFalke renamed this:
    rpc: Add submitblockheader
    rpc: Add submitheader
    on Jul 14, 2018
  35. promag commented at 9:37 PM on July 14, 2018: member

    utACK fa7d7dd.

  36. laanwj added this to the milestone 0.18.0 on Aug 2, 2018
  37. laanwj added the label Feature on Aug 2, 2018
  38. DrahtBot added the label Needs rebase on Aug 13, 2018
  39. rpc: Expose ProcessNewBlockHeaders 36b1b63f20
  40. MarcoFalke force-pushed on Aug 13, 2018
  41. qa: Add tests for submitheader fa091b0016
  42. MarcoFalke force-pushed on Aug 13, 2018
  43. MarcoFalke commented at 6:32 PM on August 13, 2018: member

    Rebased. Only conflict was in tests, no other changes.

  44. DrahtBot removed the label Needs rebase on Aug 13, 2018
  45. laanwj commented at 3:51 PM on August 15, 2018: member

    utACK fa091b001605c4481fb4eca415929a98d3478549

  46. ken2812221 referenced this in commit b5591ca0b0 on Aug 15, 2018
  47. laanwj merged this on Aug 15, 2018
  48. laanwj closed this on Aug 15, 2018
  49. MarcoFalke deleted the branch on Aug 15, 2018
  50. PastaPastaPasta referenced this in commit 24364741f9 on May 26, 2021
  51. Munkybooty referenced this in commit 7b91992a4f on Jun 29, 2021
  52. 5tefan referenced this in commit 8395100467 on Aug 9, 2021
  53. 5tefan referenced this in commit daee6164f4 on Aug 9, 2021
  54. UdjinM6 referenced this in commit 97b3ad18af on Aug 10, 2021
  55. 5tefan referenced this in commit 3befcc2c12 on Aug 12, 2021
  56. MarcoFalke locked this on Sep 8, 2021
Contributors
MarcoFalkeDrahtBotlaanwjTheBlueMattpromagjnewberysipaluke-jr
Labels
FeatureRPC/REST/ZMQ

Milestone
0.18.0

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: 2026-05-03 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me