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

1. 
   TheBlueMatt commented at 7:27 pm on June 11, 2018: member

   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.

2. rpc: Avoid "duplicate" return value for invalid submitblock fa6e49731b
3. 
   MarcoFalke commented at 7:38 pm on June 11, 2018: member

   utACK fa6e49731bb7d390c9ed66fc2d8ce3c4fa9b074f. (Haven’t looked at the second commit)
4. MarcoFalke added the label Validation on Jun 11, 2018
5. Only set fNewBlock to true in AcceptBlock when we write to disk

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

   f74894480d
6. TheBlueMatt force-pushed on Jun 11, 2018
7. in src/validation.cpp:3532 in f74894480d

   3528@@ -3530,6 +3529,7 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali
   3529         GetMainSignals().NewPoWValidBlock(pindex, pblock);
   3530 
   3531     // Write block to history file
   3532+    if (fNewBlock) *fNewBlock = true;
   

   ---

   ---

   

   promag commented at 3:33 pm on June 12, 2018:

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

   ---

   

   MarcoFalke commented at 3:46 pm on June 12, 2018:

   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.

   ---

   

   promag commented at 3:51 pm on June 12, 2018:

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

   ---

   

   MarcoFalke commented at 4:04 pm on June 12, 2018:

   If anything below fails, the node is supposed to shut down immediately
8. 
   luke-jr commented at 10:30 pm on June 12, 2018: member

   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.
9. 
   MarcoFalke commented at 10:35 pm on June 12, 2018: member

   @luke-jr

   • Create fresh invalid block
   • Submit it’s header somehow
   • Submitblock should not return “duplicate” for this fresh invalid block
10. laanwj added this to the "Blockers" column in a project

    ---

11. 
    DrahtBot commented at 8:19 pm on June 14, 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.

12. 
    sipa commented at 11:52 pm on June 14, 2018: member

    utACK f74894480db5dfbf49534499f489357ee9984183
13. 
    luke-jr commented at 0:01 am on June 15, 2018: member

    @MarcoFalke It already shouldn’t… BlockChecked should get called with the validation failure.
14. 
    MarcoFalke commented at 1:02 am on June 15, 2018: member

    Added original description to OP
15. 
    MarcoFalke commented at 2:00 pm on June 15, 2018: member

    @luke-jr Sorry I wasn’t clear that you had to submit the header first.
16. 
    MarcoFalke commented at 2:26 pm on June 15, 2018: member

    utACK f74894480db5dfbf49534499f489357ee9984183
17. jonasschnelli approved
18. 
    jonasschnelli commented at 7:23 am on June 19, 2018: contributor

    utACK f74894480db5dfbf49534499f489357ee9984183
19. jonasschnelli merged this on Jun 19, 2018
20. jonasschnelli closed this on Jun 19, 2018

    ---

21. jonasschnelli referenced this in commit 3f398d7a17 on Jun 19, 2018
22. sipa removed this from the "Blockers" column in a project

    ---

23. MarcoFalke referenced this in commit 962c302710 on Sep 13, 2018
24. PastaPastaPasta referenced this in commit db0d4dd74c on Dec 16, 2020
25. PastaPastaPasta referenced this in commit cfb42423da on Dec 18, 2020
26. PastaPastaPasta referenced this in commit 50e746ac2e on Dec 18, 2020
27. DrahtBot locked this on Sep 8, 2021

Contributors
TheBlueMatt MarcoFalke promag luke-jr DrahtBot sipa jonasschnelli

Labels
Validation