Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jun 5, 2018

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 5, 2018 •
edited
Loading

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

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.

@practicalswift
Copy link
Contributor

Is @DrahtBot a new addition to the project? Who is the bot owner? :-)

@laanwj
Copy link
Member

laanwj commented Jun 5, 2018

An evil paperclip robot, I don't know whose it is. But it seems useful.

utACK fa4125621554f2f94fded1896ba7eae10ef954da

@maflcko maflcko requested a review from TheBlueMatt June 5, 2018 15:21
@promag
Copy link
Contributor

promag commented Jun 5, 2018 •
edited
Loading

utACK fa41256. Nit, Result: help could be improved.

@TheBlueMatt
Copy link
Contributor

This breaks having two submitblocks in paralell. Now instead of one returning true and the other duplicate, you might have one return true and one simply return "inconclusive". I think the correct fix is to just set fBlockPresent to pindex->nStatus & HAVE_DATA.

@maflcko
Copy link
Member Author

maflcko commented Jun 6, 2018

@TheBlueMatt I don't see how this fixes the race. The only other option I see is to partially revert the commit that introduced the bug (eb63bf8), i.e. read from mapBlockIndex after ProcessNewBlock.

@TheBlueMatt
Copy link
Contributor

Ah, indeed, must have been introduced on master when ProcessNewBlock lost its cs_main at call-time. Obvious fix would be to duplicate the fBlockPresent check after PNB by re-acquiring cs_main.

@maflcko maflcko force-pushed the Mf1806-rpcMiningSubmitblock branch 2 times, most recently from fa88011 to fa41256 Compare June 6, 2018 17:55
@skeees
Copy link
Contributor

skeees commented Jun 6, 2018

utACK

Maybe DrahtBot can rebase my PR for me once this is merged ;-)

@maflcko maflcko force-pushed the Mf1806-rpcMiningSubmitblock branch 2 times, most recently from fa189e0 to faf9233 Compare June 6, 2018 18:46
@maflcko
Copy link
Member Author

maflcko commented Jun 6, 2018 •
edited
Loading

I force pushed the commit with two changes:

  • return "duplicate" when fNewBlock is given as false from PNB
  • Add a TODO to properly set fNewBlock when a new invalid block is sent, that fails ABH. (I am not going to touch that code in this pull request, since my main goal is to fix invalid return for the rpc)

Should be easy to re-ACK

@maflcko maflcko force-pushed the Mf1806-rpcMiningSubmitblock branch from faf9233 to fa08ebd Compare June 6, 2018 19:13
@jnewbery
Copy link
Contributor

jnewbery commented Jun 6, 2018

utACK fa08ebd7f24fc911b1af14644e4e235b6f3f9316

This was referenced Jun 7, 2018
Merged
Closed
@promag
Copy link
Contributor

promag commented Jun 10, 2018

re-utACK fa08ebd.

@maflcko maflcko force-pushed the Mf1806-rpcMiningSubmitblock branch from fa08ebd to fa6e497 Compare June 11, 2018 19:09
@maflcko maflcko closed this Jun 11, 2018
@maflcko maflcko deleted the Mf1806-rpcMiningSubmitblock branch June 11, 2018 19:12
@jnewbery
Copy link
Contributor

Why close?

@TheBlueMatt TheBlueMatt mentioned this pull request Jun 11, 2018
Merged
@maflcko
Copy link
Member Author

maflcko commented Jun 11, 2018

Why close?

To indicate that it was up for grabs.

@jnewbery
Copy link
Contributor

To indicate that it was up for grabs.

I don't get it. If this is up for grabs you think it should be merged, but you don't want to maintain it? It's currently mergeable and has ACKs. Why not just leave it open?

@TheBlueMatt
Copy link
Contributor

Please see #13439. The changes here weren't sufficient, needed one more commit to actually fix the issue mentioned.

@jnewbery
Copy link
Contributor

Got it. Good to know. Thanks Matt!

jonasschnelli added a commit that referenced this pull request Jun 19, 2018
@jonasschnelli
Merge #13439: rpc: Avoid "duplicate" return value for invalid submitb…
3f398d7 
…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
@maflcko maflcko restored the Mf1806-rpcMiningSubmitblock branch August 13, 2018 18:29
@maflcko maflcko deleted the Mf1806-rpcMiningSubmitblock branch August 13, 2018 18:30
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 16, 2020
@jonasschnelli @PastaPastaPasta
Merge bitcoin#13439: rpc: Avoid "duplicate" return value for invalid …
db0d4dd 
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 18, 2020
@jonasschnelli @PastaPastaPasta
Merge bitcoin#13439: rpc: Avoid "duplicate" return value for invalid …
cfb4242 
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 18, 2020
@jonasschnelli @PastaPastaPasta
Merge bitcoin#13439: rpc: Avoid "duplicate" return value for invalid …
50e746a 
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TheBlueMatt TheBlueMatt Awaiting requested review from TheBlueMatt

Assignees
No one assigned
Labels
Mining RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@maflcko @DrahtBot @practicalswift @laanwj @promag @TheBlueMatt @skeees @jnewbery