Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Add submitheader #13399

Merged
merged 2 commits into from Aug 15, 2018

Conversation

Projects
None yet
8 participants
@MarcoFalke
Copy link
Member

commented Jun 5, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

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
Copy link
Member

left a comment

Is there a reason to include #13395?

src/validation.h Outdated
@@ -234,7 +234,6 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
* (and possibly also) BlockChecked will have been called.
*
*
*

This comment has been minimized.

Copy link
@promag

promag Jun 5, 2018

Member

Commit "rpc: Expose ProcessNewBlockHeaders"

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

src/validation.h Outdated
@@ -245,8 +244,6 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
/**
* Process incoming block headers.
*
*

This comment has been minimized.

Copy link
@promag

promag Jun 5, 2018

Member

Commit "rpc: Expose ProcessNewBlockHeaders"

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

src/validation.h Outdated
@@ -232,28 +232,25 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
*
* Note that we guarantee that either the proof-of-work is valid on pblock, or
* (and possibly also) BlockChecked will have been called.
*
* Call without cs_main held.
*

This comment has been minimized.

Copy link
@promag

promag Jun 5, 2018

Member

Could be removed?

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))

This comment has been minimized.

Copy link
@promag

promag Jun 11, 2018

Member

nit, could include the previous hash.

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

This comment has been minimized.

Copy link
@promag

promag Jun 11, 2018

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 11, 2018

Author Member

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

}

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

This comment has been minimized.

Copy link
@promag

promag Jun 11, 2018

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 11, 2018

Author Member

That race shouldn't affect the return value.

@jnewbery
Copy link
Member

left a comment

Looks great. utACK 2595028

A few nits inline. Feel free to ignore.

@@ -232,28 +232,24 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
*
* Note that we guarantee that either the proof-of-work is valid on pblock, or
* (and possibly also) BlockChecked will have been called.
*

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jun 11, 2018

Member

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.

bytes_to_hex_str,
)

b2x = bytes_to_hex_str

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jun 11, 2018

Member

Can just use:

from test_framework.util import (
    assert_equal,
    assert_raises_rpc_error,
    bytes_to_hex_str as b2x,
+)
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())))

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jun 11, 2018

Member

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?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jun 11, 2018

Author Member

I prefer to use named arguments

This comment has been minimized.

Copy link
@promag

promag Jun 11, 2018

Member

This should work:

assert_raises_rpc_error(-25, 'bad-prevblk', node.submitheader, hexdata=b2x(CBlockHeader(bad_block2).serialize()))
}
{
LOCK(cs_main);
if (LookupBlockIndex(h.GetHash())) return NullUniValue;

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jun 11, 2018

Member

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.

This comment has been minimized.

Copy link
@promag

promag Jun 11, 2018

Member

@jnewbery do you see a use case for that?

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jun 11, 2018

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 11, 2018

Author Member

Done at the end of this function

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

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

@TheBlueMatt TheBlueMatt referenced this pull request Jun 26, 2018

Closed

Block Header Tracking #4

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1806-rpcBlockHeader branch from 2595028 Jun 26, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1806-rpcBlockHeader branch Jul 11, 2018

@DrahtBot DrahtBot removed the Needs rebase label Jul 11, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1806-rpcBlockHeader branch 3 times, most recently Jul 11, 2018

@sipa

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

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.

bad_block_lock.vtx[0].rehash()
bad_block_lock.hashMerkleRoot = bad_block_lock.calc_merkle_root()
bad_block_lock.solve()
assert_equal(node.submitblock(hexdata=b2x(bad_block_lock.serialize())), 'invalid')

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jul 14, 2018

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 14, 2018

Author Member

See the TODO

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

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jul 14, 2018

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

@MarcoFalke MarcoFalke changed the title rpc: Add submitblockheader rpc: Add submitheader Jul 14, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Jul 14, 2018

utACK fa7d7dd.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1806-rpcBlockHeader branch Aug 13, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1806-rpcBlockHeader branch to fa091b0 Aug 13, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

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

@DrahtBot DrahtBot removed the Needs rebase label Aug 13, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

utACK fa091b0

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Aug 15, 2018

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1806-rpcBlockHeader branch Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.