Allow first cmpctblock/blocktxn received to be processed (rather than first requested) #9325

1. 
   rebroad commented at 7:10 am on December 12, 2016: contributor

   Speeds up block propagation as avoids an unnecessary wait for the first requested cmpctblock and instead allows cmpctblocks sent as announcement to be processed when they arrive before the requested cmpctblock.

   Fixes #9242

2. 
   rebroad commented at 7:13 am on December 12, 2016: contributor

   Details of testing done so far mentioned in #9242
3. jonasschnelli added the label P2P on Dec 12, 2016
4. 
   sdaftuar commented at 10:16 pm on December 14, 2016: member

   I agree that we shouldn’t ignore compactblock messages just because a block is in flight from another peer, but this code could result in downloading the same block multiple times, and this patch would allow a mischievous peer to interfere with block relay to an honest peer (by resetting the PartiallyDownloadedBlock object inside the QueuedBlock).

   I have a patch that will allow us to try to reconstruct the block for the situations where no additional data is needed: #9352.

   In the future I do think it would be reasonable to allow some additional data transfer as well to make the network more robust to stalling peers, but we’ll need to be careful about bounding how much additional overhead we might incur.

5. 
   rebroad commented at 5:56 am on December 15, 2016: contributor

   @sdaftuar Have you tested this PR? I am not sure how it would interfere with relay - can you elaborate?

   I do think the code in this PR could be refined further to give a middle-road between what it currently does and what #9352 does - i.e. fetch blocktxns is they are small or base the size based on how much has already been downloaded.

   Regarding the “downloading the same block multiple times”, this doesn’t happen. i.e. this only causes compact-blocks-sent-as-announcements to be responded to, so the only change is more getblocktxns requests - 3 as a maximum (as this is the limit on compact block announcements we can received). Admittedly I could add code to reduce this to 1 or 2, and that would be my next proposed improvement to this code.

   Also, ideally we should soon only connect to nodes that can provide compact blocks, so the case where we are downloading a full block AND compact block(s) should no longer occur soon once this change is made, so the worst case scenario with this code will be that we send three getblocktxn requests - and this in my testing so far happens very rarely - most often the first blocktxn received is enough to make the block and this occurs before the 2nd compact block is received. Certainly I have never seen 3 blocktxns requested so far - and like I say, the code could be changed to avoid this.

6. 
   sdaftuar commented at 3:07 pm on December 15, 2016: member

   I am not sure how it would interfere with relay - can you elaborate?

   There are too many issues here for me to go through all of them, but I can give you a couple examples of ways an attacker can break this code; hopefully that will provide enough a flavor for how to think about these kinds of changes better.

   First example: what happens if high-bandwidth peer A and high-bandwidth peer B are using different algorithms for prefilling their compact blocks? This is explicitly allowed in BIP 152. Yet if you receive peer A’s cmpctblock, and then send back a GETBLOCKTXN, and then receive peer B’s cmpctblock before A send back the BLOCKTXN message, this patch would overwrite the PartiallyDownloadedBlock with information from B, causing A’s response to not decode properly. In this case, peer A might even get banned for sending a BLOCKTXN message with the wrong number of transactions(!).

   Another example: imagine you have two peers, A and B, controlled by a common attacker. Peer A and Peer B alternate in sending you cmpctblock messages that don’t decode (one easy way to do this would be to just omit the coinbase transaction from the prefilled transaction list; there are other ways to do this as well). Each time you receive a new cmpctblock message, you switch the download to being from the peer with the latest message, causing the disconnect logic for a stalling peer to not trigger. This can allow an attacker to blind you to a block (potentially indefinitely).

   Regarding the “downloading the same block multiple times”, this doesn’t happen. i.e. this only causes compact-blocks-sent-as-announcements to be responded to, so the only change is more getblocktxns requests - 3 as a maximum (as this is the limit on compact block announcements we can received). Admittedly I could add code to reduce this to 1 or 2, and that would be my next proposed improvement to this code.

   There is logic in the cmpctblock handling code to request a full block in the event that there’s a txid collision, so I think more care needs to be taken to ensure that we don’t download the block multiple times. (Note also that the GETBLOCKTXN message can, in the worst case, request all the transactions in the block.)

7. 
   rebroad commented at 8:23 am on December 16, 2016: contributor

   @sdaftuar Thanks for this - clearly the travis checks are insufficient, given they all passed It might be worth creating some tests to test it detects the scenarios you mention.

   It seems the partialblock needs to be kept separate for each peer that sent a compact block (up to 3 therefore, and not allow partial blocks to be created for peers where we have not requested compact block announcements).

   Given your feedback this is clearly still WIP. I realise it can be difficult to spot all the flaws in code without an extra set of eyes, so thank you for lending yours.

8. Allow blocktxns from a different peer than the requested from 5a1f545d27
9. Allow first cmpctblock received to be processed (rather than first

   requested).
   
   Speeds up block propagation as avoids an unncessary wait for the first
   requested cmpctblock and instead allows cmpctblocks sent as announcement
   to be processed when they arrive before the requested cmpctblock.
   
   Fixes #9242

   424529e081
10. rebroad force-pushed on Dec 27, 2016
11. fanquake closed this on May 17, 2017

    ---

12. DrahtBot locked this on Sep 8, 2021

Contributors
rebroad sdaftuar

Labels
P2P