doc: add comment explaining recentRejects-DoS behavior #14436

1. 
   jamesob commented at 2:32 AM on October 9, 2018: member

   When we receive invalid txs for the first time, we mark the sender as misbehaving. If we receive the same tx before a new block is seen, we don't punish the second sender (in the same way we do the original sender). It wasn't initially clear to me that this is intentional, so add a clarifying comment.

2. fanquake added the label Docs on Oct 9, 2018
3. jamesob force-pushed on Oct 9, 2018
4. fanquake requested review from sdaftuar on Oct 10, 2018
5. in src/net_processing.cpp:2376 in b8dc405d85 outdated

   2367 | +        // we have to account for recentRejects showing false positives.
   2368 | +        //
   2369 | +        // Note that recentRejects doesn't just record DoSy or invalid
   2370 | +        // transactions, but any tx not accepted by the mempool; this may be
   2371 | +        // due to node policy.
   2372 | +
   

   ---

   ---

   

   conscott commented at 3:24 PM on October 11, 2018:

   I'm probably being thick, but from this comment it's not entirely clear to me if subsequent peers relaying a previously non-accepted tx aren't punished because:

   1. The possibility of false positive inherent to bloom filters
   2. The transactions can be added to recentRejects without penalizing the peer (due to something like node policy), so just matching the filter doesn't give us enough information to deduct the reason (penalized vs non-penalized case), and therefore we can't penalize the peer.
   3. both 1 and 2

   ---

   

   jamesob commented at 5:06 PM on October 16, 2018:

   Thanks for the look, that's good feedback. I think it's (3) and I've updated the comment to reflect your question. Let me know if you think there's a better articulation.

   ---

   

   conscott commented at 1:47 AM on October 17, 2018:

   Looks good to me! Thanks for updating.

6. 
   conscott commented at 3:26 PM on October 11, 2018: contributor

   Concept ACK

7. doc: add comment explaining recentRejects-DoS behavior

   When we receive invalid txs for the first time, we mark the sender as
   misbehaving. If we receive the same tx before a new block is seen, we *don't*
   punish the second sender (in the same way we do the original sender). It wasn't
   initially clear to me that this is intentional, so add a clarifying comment.

   b191c7dfb7
8. jamesob force-pushed on Oct 16, 2018
9. 
   conscott commented at 1:47 AM on October 17, 2018: contributor

   Re-ACK b191c7dfb7ede3f74edb3a32b8ac6fa2f4d6b78a

10. 
    sipa commented at 7:05 AM on October 17, 2018: member

    ACK

11. in src/net_processing.cpp:2369 in b191c7dfb7

    2364 | +        //
    2365 | +        // This means we won't penalize any peer subsequently relaying a DoSy
    2366 | +        // tx (even if we penalized the first peer who gave it to us) because
    2367 | +        // we have to account for recentRejects showing false positives. In
    2368 | +        // other words, we shouldn't penalize a peer if we aren't *sure* they
    2369 | +        // submitted a DoSy tx.
    

    ---

    ---

    

    MarcoFalke commented at 4:49 PM on November 7, 2018:

    It sounds a bit like we want to penalize subsequent peers if we could. Though, I think this is not true, since we have not experienced any further damage by this "malicious" action (compared to lets say a "honest" relay of a transaction that we already have).

    So if we hit recentRejects, we are not interested in the DoS score because it would be too expensive to calculate for almost no gain (could detect rare false positives).

12. 
    MarcoFalke commented at 8:00 PM on November 7, 2018: member

    ACK b191c7d

13. ken2812221 referenced this in commit 11e1ac3ae0 on Nov 7, 2018
14. MarcoFalke merged this on Nov 7, 2018
15. MarcoFalke closed this on Nov 7, 2018

    ---

16. jamesob deleted the branch on Nov 7, 2018
17. deadalnix referenced this in commit 063c90dc91 on Jun 9, 2020
18. ftrader referenced this in commit e84f3ec5b5 on Aug 17, 2020
19. Munkybooty referenced this in commit 6fb718dc96 on Jul 29, 2021
20. 5tefan referenced this in commit ec490050bf on Aug 12, 2021
21. MarcoFalke locked this on Sep 8, 2021

Contributors

jamesobconscottsipaMarcoFalke

Review Requested
sdaftuar