rpc: Add submitheader

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.

MarcoFalke added the label RPC/REST/ZMQ on Jun 5, 2018

DrahtBot commented at 5:23 pm on June 5, 2018: member

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

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?

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.

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:”.

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:”.

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?

promag commented at 5:38 pm on June 5, 2018: member

Is there a reason to include #13395?

MarcoFalke force-pushed on Jun 5, 2018

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.

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

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.

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.

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:

0from test_framework.util import (
1    assert_equal,
2    assert_raises_rpc_error,
3    bytes_to_hex_str as b2x,
4+)

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:

0        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:

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

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

jnewbery commented at 3:29 pm on June 11, 2018: member

Looks great. utACK 2595028d97fc08ea0d27c3133c44d63f3e8bbc0f

A few nits inline. Feel free to ignore.

DrahtBot added the label Needs rebase on Jun 15, 2018

jnewbery commented at 4:31 pm on June 20, 2018: member

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

MarcoFalke force-pushed on Jun 26, 2018

MarcoFalke removed the label Needs rebase on Jun 26, 2018

DrahtBot added the label Needs rebase on Jul 7, 2018

MarcoFalke force-pushed on Jul 11, 2018

DrahtBot removed the label Needs rebase on Jul 11, 2018

MarcoFalke force-pushed on Jul 11, 2018

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.

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”…

MarcoFalke commented at 6:57 pm on July 14, 2018:

See the TODO https://github.com/bitcoin/bitcoin/blob/8803c9132a78d8182bd828a29f7051fc7688f934/src/rpc/mining.cpp#L756

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…

MarcoFalke renamed this:
~~rpc: Add submitblockheader~~
rpc: Add submitheader
on Jul 14, 2018

promag commented at 9:37 pm on July 14, 2018: member

utACK fa7d7dd.

laanwj added this to the milestone 0.18.0 on Aug 2, 2018

laanwj added the label Feature on Aug 2, 2018

DrahtBot added the label Needs rebase on Aug 13, 2018

rpc: Expose ProcessNewBlockHeaders 36b1b63f20

MarcoFalke force-pushed on Aug 13, 2018

qa: Add tests for submitheader fa091b0016

MarcoFalke force-pushed on Aug 13, 2018

MarcoFalke commented at 6:32 pm on August 13, 2018: member

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

DrahtBot removed the label Needs rebase on Aug 13, 2018

laanwj commented at 3:51 pm on August 15, 2018: member

utACK fa091b001605c4481fb4eca415929a98d3478549

ken2812221 referenced this in commit b5591ca0b0 on Aug 15, 2018

laanwj merged this on Aug 15, 2018

laanwj closed this on Aug 15, 2018

MarcoFalke deleted the branch on Aug 15, 2018

PastaPastaPasta referenced this in commit 24364741f9 on May 26, 2021

Munkybooty referenced this in commit 7b91992a4f on Jun 29, 2021

5tefan referenced this in commit 8395100467 on Aug 9, 2021

5tefan referenced this in commit daee6164f4 on Aug 9, 2021

UdjinM6 referenced this in commit 97b3ad18af on Aug 10, 2021

5tefan referenced this in commit 3befcc2c12 on Aug 12, 2021

MarcoFalke locked this on Sep 8, 2021

rpc: Add submitheader #13399