rpc: Avoid “duplicate” return value for invalid submitblock #13395

1. 
   MarcoFalke commented at 0: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.

2. MarcoFalke added the label RPC/REST/ZMQ on Jun 5, 2018
3. MarcoFalke added the label Mining on Jun 5, 2018
4. 
   DrahtBot commented at 0:45 am on June 5, 2018: member

   • #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.

5. 
   practicalswift commented at 7:10 am on June 5, 2018: contributor

   Is @DrahtBot a new addition to the project? Who is the bot owner? :-)
6. 
   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

7. MarcoFalke requested review from TheBlueMatt on Jun 5, 2018
8. 
   promag commented at 4:00 pm on June 5, 2018: member

   utACK fa41256. Nit, Result: help could be improved.
9. 
   TheBlueMatt commented at 5:18 pm on June 6, 2018: member

   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.
10. 
    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.
11. 
    TheBlueMatt commented at 5:32 pm on June 6, 2018: member

    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.
12. MarcoFalke force-pushed on Jun 6, 2018
13. MarcoFalke force-pushed on Jun 6, 2018
14. 
    skeees commented at 6:36 pm on June 6, 2018: contributor

    utACK

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

15. MarcoFalke force-pushed on Jun 6, 2018
16. MarcoFalke force-pushed on Jun 6, 2018
17. 
    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

18. MarcoFalke force-pushed on Jun 6, 2018
19. 
    jnewbery commented at 9:17 pm on June 6, 2018: member

    utACK fa08ebd7f24fc911b1af14644e4e235b6f3f9316
20. 
    promag commented at 9:48 pm on June 10, 2018: member

    re-utACK fa08ebd.
21. rpc: Avoid "duplicate" return value for invalid submitblock fa6e49731b
22. MarcoFalke force-pushed on Jun 11, 2018
23. MarcoFalke closed this on Jun 11, 2018

    ---

24. MarcoFalke deleted the branch on Jun 11, 2018
25. 
    jnewbery commented at 7:25 pm on June 11, 2018: member

    Why close?
26. 
    MarcoFalke commented at 7:39 pm on June 11, 2018: member

    Why close?

    To indicate that it was up for grabs.

27. 
    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?

28. 
    TheBlueMatt commented at 8:14 pm on June 11, 2018: member

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

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

Contributors
MarcoFalke DrahtBot practicalswift laanwj promag TheBlueMatt skeees jnewbery

Review Requested
TheBlueMatt

Labels
RPC/REST/ZMQ Mining