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

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1806-rpcMiningSubmitblock changing 1 files +6 −7

MarcoFalke commented at 12:30 AM on June 5, 2018: member

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.
MarcoFalke added the label RPC/REST/ZMQ on Jun 5, 2018
MarcoFalke added the label Mining on Jun 5, 2018
DrahtBot commented at 12:45 AM on June 5, 2018: contributor
Note to reviewers: This pull request conflicts with the following ones:
- #13413 ([net,mempool] Call AcceptToMemoryPool() asynchronously in p2p by skeees)
- #13399 (rpc: Add submitblockheader by MarcoFalke)
- #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.
practicalswift commented at 7:10 AM on June 5, 2018: contributor

Is @DrahtBot a new addition to the project? Who is the bot owner? :-)
laanwj commented at 1:39 PM on June 5, 2018: member

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

utACK fa4125621554f2f94fded1896ba7eae10ef954da
MarcoFalke requested review from TheBlueMatt on Jun 5, 2018
promag commented at 4:00 PM on June 5, 2018: member

utACK fa41256. Nit, Result: help could be improved.
TheBlueMatt commented at 5:18 PM on June 6, 2018: 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.
MarcoFalke commented at 5:30 PM on June 6, 2018: member

@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 (eb63bf86cf6dc99f150574463df6ffb013a34493), i.e. read from mapBlockIndex after ProcessNewBlock.
TheBlueMatt commented at 5:32 PM on June 6, 2018: 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.
MarcoFalke force-pushed on Jun 6, 2018
MarcoFalke force-pushed on Jun 6, 2018
skeees commented at 6:36 PM on June 6, 2018: contributor

utACK

Maybe DrahtBot can rebase my PR for me once this is merged ;-)
MarcoFalke force-pushed on Jun 6, 2018
MarcoFalke force-pushed on Jun 6, 2018
MarcoFalke commented at 6:49 PM on June 6, 2018: member
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
MarcoFalke force-pushed on Jun 6, 2018
jnewbery commented at 9:17 PM on June 6, 2018: member

utACK fa08ebd7f24fc911b1af14644e4e235b6f3f9316
DrahtBot cross-referenced this on Jun 7, 2018 from issue rpc: Add submitheader by MarcoFalke
DrahtBot cross-referenced this on Jun 7, 2018 from issue [net,mempool] Call AcceptToMemoryPool() asynchronously in p2p by skeees
promag commented at 9:48 PM on June 10, 2018: member

re-utACK fa08ebd.
rpc: Avoid "duplicate" return value for invalid submitblock fa6e49731b
MarcoFalke force-pushed on Jun 11, 2018
MarcoFalke closed this on Jun 11, 2018
MarcoFalke deleted the branch on Jun 11, 2018
jnewbery commented at 7:25 PM on June 11, 2018: member

Why close?
TheBlueMatt cross-referenced this on Jun 11, 2018 from issue rpc: Avoid "duplicate" return value for invalid submitblock by TheBlueMatt
MarcoFalke commented at 7:39 PM on June 11, 2018: member

Why close?

To indicate that it was up for grabs.
jnewbery commented at 8:09 PM on June 11, 2018: member

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 commented at 8:14 PM on June 11, 2018: contributor

Please see #13439. The changes here weren't sufficient, needed one more commit to actually fix the issue mentioned.
jnewbery commented at 8:31 PM on June 11, 2018: member

Got it. Good to know. Thanks Matt!
jonasschnelli referenced this in commit 3f398d7a17 on Jun 19, 2018
MarcoFalke restored the branch on Aug 13, 2018
MarcoFalke deleted the branch on Aug 13, 2018
PastaPastaPasta referenced this in commit db0d4dd74c on Dec 16, 2020
PastaPastaPasta referenced this in commit cfb42423da on Dec 18, 2020
PastaPastaPasta referenced this in commit 50e746ac2e on Dec 18, 2020
bitcoin locked this on Sep 8, 2021