Check block header when accepting headers from peers. #5270

1. 
   domob1812 commented at 8:46 AM on November 13, 2014: contributor

   Actually check block headers received from peers. If a peer sends headers with invalid PoW, they were accepted and added to the disk block index previously. This would then lead to a failure in checking the PoW during the next startup while loading the block index.

   This is a better version of #5269.

2. laanwj added the label P2P on Nov 13, 2014
3. 
   sipa commented at 10:33 AM on November 13, 2014: member

   Wow, thanks for catching this.

   I was convinced that AcceptBlockHeader called CheckBlockHeader.

4. 
   domob1812 commented at 10:49 AM on November 13, 2014: contributor

   Hm, now that you mention it, AcceptBlock does call CheckBlock. So maybe the call to CheckBlockHeader should be moved inside of AcceptBlockHeader? (Doesn't matter for the functionality, but is maybe cleaner and more consistent with the rest.) So far I understood it as "check first, then accept to only update the state" (not quite, but more or less).

5. 
   sipa commented at 10:57 AM on November 13, 2014: member

   Indeed, that seems like the right place for it. Functionally it shouldn't matter, but it's strange that AcceptBlockHeader would accept an invalid header...

6. domob1812 force-pushed on Nov 13, 2014
7. 
   domob1812 commented at 7:07 PM on November 13, 2014: contributor

   Updated accordingly. Note that AcceptBlock now calls the PoW check twice: Once via CheckBlock and once via AcceptBlockHeader. I guess that doesn't hurt, though.

8. 
   ghost commented at 11:38 PM on November 13, 2014: none

   Regarding repeated calls, see #5163.

9. in src/main.cpp:None in bfaf3f0c39 outdated

   2350 | @@ -2351,6 +2351,12 @@ bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBloc
   2351 |          return true;
   2352 |      }
   2353 |  
   2354 | +    // Check that the header is valid (particularly PoW).  This is mostly
   

   ---

   ---

   

   sipa commented at 5:08 PM on November 14, 2014:

   I think it's incorrect to call this one redundant. It's the other one that is...

10. 
    sipa commented at 5:09 PM on November 14, 2014: member

    untested ACK

11. sipa added the label Priority High on Nov 14, 2014
12. sipa added the label Bug on Nov 14, 2014
13. sipa added this to the milestone 0.10.0 on Nov 17, 2014
14. 
    TheBlueMatt commented at 1:21 AM on November 19, 2014: member

    utACK only commithash bfaf3f0c39954239726410546990a229af89b70d (+/- the semi-invalid comment): http://bitcoin.ninja/TheBlueMatt-5270.txt

15. 
    domob1812 commented at 6:19 AM on November 19, 2014: contributor

    I can completely remove the comment. Do you like that better? Or any suggestions on what to say there instead?

16. 
    sipa commented at 10:27 AM on November 19, 2014: member

    Can you move the comment to the CheckBlockHeader() call in CheckBlock()?

17. Check block header before accepting it.

    Previously, AcceptBlockHeader did not check the header (in particular
    PoW).  This made the client accept invalid-PoW-headers from peers in
    headers-first sync.

    57425a2425
18. domob1812 force-pushed on Nov 20, 2014
19. 
    domob1812 commented at 7:29 AM on November 20, 2014: contributor

    Done.

20. 
    laanwj commented at 12:27 PM on November 20, 2014: member

    utACK commithash 57425a24255c5af439241d59ad5a878b7a3771a7 https://dev.visucore.com/bitcoin/acks/5270

21. laanwj merged this on Nov 20, 2014
22. laanwj closed this on Nov 20, 2014

    ---

23. laanwj referenced this in commit 5c4dffd188 on Nov 20, 2014
24. domob1812 deleted the branch on Nov 20, 2014
25. DrahtBot locked this on Sep 8, 2021

Milestone
0.10.0