net: Document what happens to getdata of unknown type #16188

1. 
   MarcoFalke commented at 3:30 PM on June 11, 2019: member

   Any getdata of unknown type will never be processed and blocks all future messages from a peer. This isn't obviously clear from reading the code, so document it.

2. DrahtBot added the label P2P on Jun 11, 2019
3. in src/net_processing.cpp:1544 in fa38f54378 outdated

   1539 | @@ -1540,6 +1540,11 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
   1540 |          }
   1541 |      }
   1542 |  
   1543 | +    // Unknown types in the GetData stay in vRecvGetData and block any future
   1544 | +    // getdata from this peer.
   

   ---

   ---

   

   mzumsande commented at 4:25 PM on June 11, 2019:

   Wouldn't this not only block future GetData, but also all other messages from this peer, because of the line if (!pfrom->vRecvGetData.empty()) return true; in ProcessMessages()?

   ---

   

   MarcoFalke commented at 5:24 PM on June 11, 2019:

   Indeed. In which case we might actually disconnect the peer, since there isn't anything that could happen on that connection anyway.

   ---

   

   naumenkogs commented at 9:11 PM on June 11, 2019:

   This is also the only thing that prevents vRecvGetData from unbounded growth.

   ---

   

   naumenkogs commented at 9:14 PM on June 11, 2019:

   Perhaps I'd suggest adding this note to if (!pfrom->vRecvGetData.empty()) return true;, otherwise it's not obvious why we should do this check before processing new messages.

   ---

   

   MarcoFalke commented at 9:17 PM on June 11, 2019:

   The comment there reads // this maintains the order of responses

   ---

   

   naumenkogs commented at 9:22 PM on June 11, 2019:

   Which does not really explain that removing this line would enable DoS :)

   ---

   

   MarcoFalke commented at 2:49 PM on June 20, 2019:

   Thanks, done.

4. mzumsande approved
5. MarcoFalke renamed this:
   net: Document what happens to getdata of unknonw type
   net: Document what happens to getdata of unknown type
   on Jun 11, 2019
6. fanquake requested review from sdaftuar on Jun 14, 2019
7. fanquake requested review from morcos on Jun 14, 2019
8. net: Document what happens to getdata of unknonw type dddd9270f8
9. 
   MarcoFalke commented at 2:49 PM on June 20, 2019: member

   Addressed review by @mzumsande and @naumenkogs

10. MarcoFalke force-pushed on Jun 20, 2019
11. MarcoFalke added the label Docs on Jun 20, 2019
12. 
    naumenkogs commented at 3:26 PM on June 20, 2019: member

    ACK

13. 
    mzumsande commented at 3:39 PM on June 20, 2019: member

    ACK as well. This construction (upholding the connection with a peer we can never process a message from) doesn't make a lot of sense to me, but documenting it is good.

14. 
    MarcoFalke commented at 3:51 PM on June 20, 2019: member

    Yeah, I'd say to disconnect the peer, but that should be done in a follow-up pull request.

15. 
    sdaftuar commented at 2:38 AM on June 25, 2019: member

    Looks right to me, but I haven't had a chance to verify that this is true with a test.

16. fanquake merged this on Jun 25, 2019
17. fanquake closed this on Jun 25, 2019

    ---

18. fanquake referenced this in commit 21bd6eb782 on Jun 25, 2019
19. MarcoFalke deleted the branch on Jul 2, 2019
20. deadalnix referenced this in commit d022eaaeac on Jun 17, 2020
21. ftrader referenced this in commit 659385f552 on Aug 17, 2020
22. PastaPastaPasta referenced this in commit c3dbd066aa on Jun 27, 2021
23. PastaPastaPasta referenced this in commit b19a2100c8 on Jun 28, 2021
24. PastaPastaPasta referenced this in commit 371ec30a53 on Jun 29, 2021
25. PastaPastaPasta referenced this in commit 4d20b42841 on Jul 1, 2021
26. PastaPastaPasta referenced this in commit 20ed20b7cf on Jul 1, 2021
27. PastaPastaPasta referenced this in commit ca23beb346 on Jul 12, 2021
28. PastaPastaPasta referenced this in commit e4060890f6 on Jul 13, 2021
29. DrahtBot locked this on Dec 16, 2021

Contributors

MarcoFalkemzumsandenaumenkogssdaftuar

Review Requested
morcossdaftuar