Doc: add a comment referencing past vulnerability next to where it was fixed #30538

1. 
   darosior commented at 9:11 am on July 28, 2024: member

   It is useful when reading code to have context about why it is written or behaves the way it does. Some instances in this PR may seem obvious but i think nonetheless offer important context to anyone willing to change (or review a change to) this code.
2. 
   DrahtBot commented at 9:11 am on July 28, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30538.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ryanofsky
   Stale ACK	tdb3, mjdietzx

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label Docs on Jul 28, 2024
4. tdb3 approved
5. 
   tdb3 commented at 1:30 pm on July 28, 2024: contributor

   ACK 7fea8eeeb9984ff6f3ed661f3970b1aaa68548de Great idea Tested links (worked)
6. eval-exec approved
7. 
   mjdietzx commented at 4:52 pm on July 29, 2024: contributor

   ACK 7fea8eeeb9984ff6f3ed661f3970b1aaa68548de
8. glozow requested review from dergoegge on Jul 31, 2024
9. 
   achow101 commented at 11:15 pm on September 17, 2024: member

   ~0

   I’m not sure that having a comment that references code that no longer exists is all that useful.

10. DrahtBot added the label CI failed on Oct 13, 2024
11. DrahtBot removed the label CI failed on Oct 19, 2024
12. DrahtBot added the label Needs rebase on Oct 29, 2024
13. ryanofsky approved
14. 
    ryanofsky commented at 3:55 pm on February 12, 2025: contributor

    Code review ACK 7fea8eeeb9984ff6f3ed661f3970b1aaa68548de, but needs rebase currently

    I’m not sure that having a comment that references code that no longer exists is all that useful.

    I think I’d agree that some of these comments are more useful than others, but some definitely seem useful, and even the ones that aren’t especially useful provide context and are very interesting to know about and don’t get in the way.

    • d6b46d8d5e77b5fef14658340c316d4d84d95d6e seems useful and important. Without that, the only comment is “Check against checkpoints” which tells you nothing about why the check exists. I think I might move the two comments more closely together, but they also seem ok where they are.

    • 7b44f0741f19471439b367cb608aed1787509b64 seems not especially useful since previous behavior it describes was just an outright bug. But the comment is interesting to know about and fits well with existing comments in the flow of the code. Also it’s good reminder of how this code needs to handle malformed requests as well as well-formed ones.

    • 382f67d39ad55469faf178ba4c41aae8a8bf6265 seems useful but I think it would be helpful if were expanded with another sentence to mention what things ProcessOrphanTx is doing now to avoid previous DoS behavior, since that does not seem clear. In previous cases, the link between the comment and the code was more obvious.

    • f57f62163857128fdc438eead1e07c54e0761650 seems incredibly useful. After a long description of an algorithm there is a link discussing a specific case it was meant to handle, that should be helpful for understanding why it was designed as it is.

    • 70992f59deea591339c6a18d42faf48486b1584f also seems very helpful. Previous comment only described the check, not why it was being done

    • 7fea8eeeb9984ff6f3ed661f3970b1aaa68548de seems helpful because previous comment explained banning and disconnecting but didn’t explain considerations for choosing one over the other.

15. doc: validation: add a reference to historical header spam vulnerability 5e3d9f21df
16. doc: net_proc: reference past defect regarding invalid GETDATA types c02d9f6dd5
17. doc: net_proc: reference past DoS vulnerability in orphan processing 68ac9542c4
18. doc: txrequest: point to past censorship vulnerability in tx re-request handling 4489117c3f
19. doc: net: mention past vulnerability as rationale to limit incoming message size ad616b6c01
20. doc: banman: reference past vuln due to unbounded banlist eb0724f0de
21. darosior force-pushed on Feb 12, 2025
22. 
    darosior commented at 8:10 pm on February 12, 2025: member

    Rebased.
23. DrahtBot removed the label Needs rebase on Feb 12, 2025
24. ryanofsky approved
25. 
    ryanofsky commented at 2:36 pm on March 12, 2025: contributor

    Code review ACK eb0724f0dee307d6d14e47ebd3077b7ffd50f507. No changes since last review other than rebase
26. DrahtBot requested review from tdb3 on Mar 12, 2025

Contributors
darosior DrahtBot tdb3 eval-exec mjdietzx achow101 ryanofsky

Review Requested
dergoegge tdb3

Labels
Docs