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

TheBlueMatt · 2018-06-11T19:27:48Z

This is #13395 with one more commit tacked on. MarcoFalke got tired of dealing with the stupidity of fixing a return code with too many rounds of review (not that I blame him). Honestly we should probably have no return whatsoever, but for now, this fixes it (as well as nLastBlockTime for eviction purposes).

Original description:

When submitblock of an invalid block, the return value should not be "duplicate".

This is only seen when the header was previously found (denoted by the incorrectly named boolean fBlockPresent). Fix this bug by removing fBlockPresent.

maflcko · 2018-06-11T19:38:01Z

utACK fa6e497. (Haven't looked at the second commit)

The only affect this should have is fixing the return code in submitblock in cases where a block fails ContextualCheckBlock and not setting nLastBlockTime on peers that provide blocks which fail ContextualCheckBlock (which is only used in eviction and cosmetic).

promag · 2018-06-12T15:33:39Z

src/validation.cpp

@@ -3530,6 +3529,7 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali
        GetMainSignals().NewPoWValidBlock(pindex, pblock);

    // Write block to history file
+    if (fNewBlock) *fNewBlock = true;


Sorry, can't this be just before return true?

Yes, this could be anywhere between this line and the line you mentioned. Though, since this went through more than a week of bike-shedding, I'd prefer if we just fixed the bug and then followed up with nits in later pull requests.

Is it a nit? Is it a new block if anything fails below?

If anything below fails, the node is supposed to shut down immediately

luke-jr · 2018-06-12T22:30:17Z

I can't tell what this PR is trying to do. It's "duplicate-inconclusive" when the full block checks have not been executed. For example, if the block is not on tip, but a shorter forked chain, we don't complete validation of it.

maflcko · 2018-06-12T22:35:39Z

@luke-jr

Create fresh invalid block
Submit it's header somehow
Submitblock should not return "duplicate" for this fresh invalid block

DrahtBot · 2018-06-14T20:19:10Z

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

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413 ([net,mempool] Call AcceptToMemoryPool() asynchronously in p2p by skeees)
rpc: Add submitheader #13399 (rpc: Add submitblockheader by MarcoFalke)
[net] [validation] Call ProcessNewBlock() asynchronously #12934 ([net] [validation] Call ProcessNewBlock() asynchronously by skeees)

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.

sipa · 2018-06-14T23:52:13Z

utACK f748944

luke-jr · 2018-06-15T00:01:41Z

@MarcoFalke It already shouldn't... BlockChecked should get called with the validation failure.

maflcko · 2018-06-15T01:02:40Z

Added original description to OP

maflcko · 2018-06-15T14:00:50Z

@luke-jr Sorry I wasn't clear that you had to submit the header first.

maflcko · 2018-06-15T14:26:14Z

utACK f748944

jonasschnelli

utACK f748944

…lock f748944 Only set fNewBlock to true in AcceptBlock when we write to disk (Matt Corallo) fa6e497 rpc: Avoid "duplicate" return value for invalid submitblock (MarcoFalke) Pull request description: This is #13395 with one more commit tacked on. MarcoFalke got tired of dealing with the stupidity of fixing a return code with too many rounds of review (not that I blame him). Honestly we should probably have no return whatsoever, but for now, this fixes it (as well as nLastBlockTime for eviction purposes). Original description: When `submitblock` of an invalid block, the return value should not be `"duplicate"`. This is only seen when the header was previously found (denoted by the incorrectly named boolean `fBlockPresent`). Fix this bug by removing `fBlockPresent`. Tree-SHA512: 0ce3092655d5d904b4c8c5ff7479f73ce387144a738f20472b8af132564005c6db5594ae366e589508f6258506ee7a28b1c7995a83a8328b334f99316006bf2d

…itblock fa6ab8a rpc: Return more specific reject reason for submitblock (MarcoFalke) Pull request description: The second commit in bitcoin#13439 made the `TODO` in the first commit impossible to solve. The meaning of `fNewBlock` changed from "This is the first time we process this block" to "We are about to write the new *valid* block". So whenever `fNewBlock` is true, the block was valid. And whenever the `fNewBlock` is false, the block is either valid or invalid. If it was valid and not new, we know it is a `"duplicate"`. In all other cases, the `BIP22ValidationResult()` will return the reason why it is invalid. Tree-SHA512: 4b6edf7a912339c3acb0fccfabbdd6d812a0321fb1639c244c2714e58dc119aa2b8c6bf8f7d61ea609a1b861bbc23f920370fcf989c48452721e259a8ce93d24

…submitblock f748944 Only set fNewBlock to true in AcceptBlock when we write to disk (Matt Corallo) fa6e497 rpc: Avoid "duplicate" return value for invalid submitblock (MarcoFalke) Pull request description: This is bitcoin#13395 with one more commit tacked on. MarcoFalke got tired of dealing with the stupidity of fixing a return code with too many rounds of review (not that I blame him). Honestly we should probably have no return whatsoever, but for now, this fixes it (as well as nLastBlockTime for eviction purposes). Original description: When `submitblock` of an invalid block, the return value should not be `"duplicate"`. This is only seen when the header was previously found (denoted by the incorrectly named boolean `fBlockPresent`). Fix this bug by removing `fBlockPresent`. Tree-SHA512: 0ce3092655d5d904b4c8c5ff7479f73ce387144a738f20472b8af132564005c6db5594ae366e589508f6258506ee7a28b1c7995a83a8328b334f99316006bf2d

rpc: Avoid "duplicate" return value for invalid submitblock

fa6e497

maflcko added the Validation label Jun 11, 2018

TheBlueMatt mentioned this pull request Jun 11, 2018

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

Closed

TheBlueMatt force-pushed the 2018-06-marcos-submitblock-fix branch from 2437c42 to f748944 Compare June 11, 2018 21:21

promag reviewed Jun 12, 2018

View reviewed changes

This was referenced Jun 14, 2018

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

rpc: Add submitheader #13399

Merged

jonasschnelli approved these changes Jun 19, 2018

View reviewed changes

jonasschnelli merged commit f748944 into bitcoin:master Jun 19, 2018

Bushstar mentioned this pull request Jun 20, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#348

Merged

Sjors mentioned this pull request Aug 10, 2018

Track best-possible-headers (TheBlueMatt) #13937

Closed

maflcko mentioned this pull request Aug 15, 2018

rpc: Return more specific reject reason for submitblock #13983

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

TheBlueMatt commented Jun 11, 2018 •

edited by maflcko

Loading

Uh oh!

maflcko commented Jun 11, 2018

Uh oh!

promag Jun 12, 2018 •

edited

Loading

Uh oh!

maflcko Jun 12, 2018

Uh oh!

promag Jun 12, 2018

Uh oh!

maflcko Jun 12, 2018

Uh oh!

luke-jr commented Jun 12, 2018

Uh oh!

maflcko commented Jun 12, 2018 •

edited

Loading

Uh oh!

DrahtBot commented Jun 14, 2018

Uh oh!

sipa commented Jun 14, 2018

Uh oh!

luke-jr commented Jun 15, 2018

Uh oh!

maflcko commented Jun 15, 2018

Uh oh!

maflcko commented Jun 15, 2018

Uh oh!

maflcko commented Jun 15, 2018

Uh oh!

jonasschnelli left a comment

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

maflcko commented Jun 11, 2018

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jun 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

DrahtBot commented Jun 14, 2018

Uh oh!

sipa commented Jun 14, 2018

Uh oh!

luke-jr commented Jun 15, 2018

Uh oh!

maflcko commented Jun 15, 2018

Uh oh!

maflcko commented Jun 15, 2018

Uh oh!

maflcko commented Jun 15, 2018

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Jun 11, 2018 •

edited by maflcko

Loading

promag Jun 12, 2018 •

edited

Loading

maflcko commented Jun 12, 2018 •

edited

Loading