rpc: Add submitheader by maflcko · Pull Request #13399 · bitcoin/bitcoin

maflcko · 2018-06-05T17:07:32Z

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.

DrahtBot · 2018-06-05T17:23:35Z

Note to reviewers: This pull request conflicts with the following ones:

Refactoring CRPCCommand with enum category #13945 (Refactoring CRPCCommand with enum category by isghe)
util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #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 · 2018-06-05T17:27:58Z

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

TheBlueMatt · 2018-06-05T17:31:04Z

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.

promag

Is there a reason to include #13395?

promag · 2018-06-05T17:34:35Z

src/validation.h

Commit "rpc: Expose ProcessNewBlockHeaders"

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

promag · 2018-06-05T17:34:50Z

src/validation.h

Commit "rpc: Expose ProcessNewBlockHeaders"

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

promag · 2018-06-05T17:36:59Z

src/validation.h

Could be removed?

promag · 2018-06-11T08:45:32Z

test/functional/mining_basic.py

+        self.log.info('submitheader tests')
+        assert_raises_rpc_error(-22, 'Block header decode failed', lambda: node.submitheader(hexdata='xx' * 80))
+        assert_raises_rpc_error(-22, 'Block header decode failed', lambda: node.submitheader(hexdata='ff' * 78))
+        assert_raises_rpc_error(-25, 'Must submit previous header', lambda: node.submitheader(hexdata='ff' * 80))


nit, could include the previous hash.

promag · 2018-06-11T08:51:42Z

src/rpc/mining.cpp

+    }
+    {
+        LOCK(cs_main);
+        if (LookupBlockIndex(h.GetHash())) return NullUniValue;


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

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

promag · 2018-06-11T08:52:06Z

src/rpc/mining.cpp

+    }
+
+    CValidationState state;
+    ProcessNewBlockHeaders({h}, state, Params(), /* ppindex */ nullptr, /* first_invalid */ nullptr);


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

That race shouldn't affect the return value.

jnewbery

Looks great. utACK 2595028

A few nits inline. Feel free to ignore.

jnewbery · 2018-06-11T14:42:15Z

src/validation.h

 *
 * Note that we guarantee that either the proof-of-work is valid on pblock, or
 * (and possibly also) BlockChecked will have been called.
- * 


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.

jnewbery · 2018-06-11T14:43:00Z

test/functional/mining_basic.py

+    bytes_to_hex_str,
+)
+
+b2x = bytes_to_hex_str


Can just use:

from test_framework.util import ( assert_equal, assert_raises_rpc_error, bytes_to_hex_str as b2x, +)

jnewbery · 2018-06-11T14:53:15Z

test/functional/mining_basic.py

+        bad_block2 = copy.deepcopy(block)
+        bad_block2.hashPrevBlock = bad_block.sha256
+        bad_block2.solve()
+        assert_raises_rpc_error(-25, 'bad-prevblk', lambda: node.submitheader(hexdata=b2x(CBlockHeader(bad_block2).serialize())))


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?

I prefer to use named arguments

This should work:

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

jnewbery · 2018-06-11T15:19:02Z

src/rpc/mining.cpp

+    }
+    {
+        LOCK(cs_main);
+        if (LookupBlockIndex(h.GetHash())) return NullUniValue;


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.

@jnewbery do you see a use case for that?

jnewbery · 2018-06-11T15:28:34Z

test/functional/mining_basic.py

+        assert chain_tip(block.hash) in node.getchaintips()
+        node.submitheader(hexdata=b2x(CBlockHeader(block).serialize()))  # Noop
+        assert chain_tip(block.hash) in node.getchaintips()
+


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

Done at the end of this function

jnewbery · 2018-06-20T16:31:34Z

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

sipa · 2018-07-13T22:12:13Z

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.

luke-jr · 2018-07-14T17:32:39Z

test/functional/mining_basic.py

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

See the TODO

bitcoin/src/rpc/mining.cpp

Line 756 in 8803c91

// TODO Maybe pass down fNewBlock to AcceptBlockHeader, so it is properly set to true in this case?

luke-jr · 2018-07-14T17:33:13Z

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

promag · 2018-07-14T21:37:43Z

utACK fa7d7dd.

maflcko · 2018-08-13T18:32:58Z

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

laanwj · 2018-08-15T15:51:46Z

utACK fa091b0

fa091b0 qa: Add tests for submitheader (MarcoFalke) 36b1b63 rpc: Expose ProcessNewBlockHeaders (MarcoFalke) Pull request description: 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. Tree-SHA512: a61e850470f15465f88e450609116df0a98d5d9afadf36b2033d820933d8b6a4012f9f2b3246319c08a0e511bef517f5d808cd0f44ffca91d10895a938004f0b

* Merge bitcoin#13399: rpc: Add submitheader fa091b0 qa: Add tests for submitheader (MarcoFalke) 36b1b63 rpc: Expose ProcessNewBlockHeaders (MarcoFalke) Pull request description: 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. Tree-SHA512: a61e850470f15465f88e450609116df0a98d5d9afadf36b2033d820933d8b6a4012f9f2b3246319c08a0e511bef517f5d808cd0f44ffca91d10895a938004f0b * Update test/functional/mining_basic.py Co-authored-by: UdjinM6 <[email protected]> Co-authored-by: UdjinM6 <[email protected]>

maflcko added the RPC/REST/ZMQ label Jun 5, 2018

promag reviewed Jun 5, 2018

View reviewed changes

maflcko force-pushed the Mf1806-rpcBlockHeader branch 3 times, most recently from 9e2b936 to 2595028 Compare June 5, 2018 19:32

promag reviewed Jun 11, 2018

View reviewed changes

jnewbery reviewed Jun 11, 2018

View reviewed changes

This was referenced Jun 14, 2018

rpc: Avoid "duplicate" return value for invalid submitblock #13439

Merged

Drop ParseHashUV in favor of calling ParseHashStr #13422

Merged

DrahtBot added the Needs rebase label Jun 15, 2018

TheBlueMatt mentioned this pull request Jun 26, 2018

Block Header Tracking TheBlueMatt/mining-proxy#4

Closed

maflcko force-pushed the Mf1806-rpcBlockHeader branch from 2595028 to 03ac2b4 Compare June 26, 2018 09:23

maflcko removed the Needs rebase label Jun 26, 2018

DrahtBot added the Needs rebase label Jul 7, 2018

maflcko force-pushed the Mf1806-rpcBlockHeader branch from 03ac2b4 to 3ea4750 Compare July 11, 2018 15:03

DrahtBot removed the Needs rebase label Jul 11, 2018

maflcko force-pushed the Mf1806-rpcBlockHeader branch 3 times, most recently from dfc8cf6 to fa7d7dd Compare July 11, 2018 15:57

luke-jr reviewed Jul 14, 2018

View reviewed changes

maflcko changed the title ~~rpc: Add submitblockheader~~ rpc: Add submitheader Jul 14, 2018

laanwj added this to the 0.18.0 milestone Aug 2, 2018

laanwj added the Feature label Aug 2, 2018

DrahtBot mentioned this pull request Aug 2, 2018

util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #13815

Merged

DrahtBot mentioned this pull request Aug 12, 2018

Refactoring CRPCCommand with enum category #13945

Closed

DrahtBot added the Needs rebase label Aug 13, 2018

rpc: Expose ProcessNewBlockHeaders

36b1b63

maflcko force-pushed the Mf1806-rpcBlockHeader branch from fa7d7dd to 5ce7446 Compare August 13, 2018 18:29

qa: Add tests for submitheader

fa091b0

maflcko force-pushed the Mf1806-rpcBlockHeader branch from 5ce7446 to fa091b0 Compare August 13, 2018 18:30

DrahtBot removed the Needs rebase label Aug 13, 2018

laanwj merged commit fa091b0 into bitcoin:master Aug 15, 2018

maflcko deleted the Mf1806-rpcBlockHeader branch August 15, 2018 16:16

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

maflcko commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 5, 2018

Uh oh!

TheBlueMatt commented Jun 5, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 20, 2018

Uh oh!

sipa commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jul 14, 2018

Uh oh!

promag commented Jul 14, 2018

Uh oh!

maflcko commented Aug 13, 2018

Uh oh!

laanwj commented Aug 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maflcko commented Jun 5, 2018 •

edited

Loading

DrahtBot commented Jun 5, 2018 •

edited

Loading