TxDownloadManager followups #31190

pull glozow wants to merge 3 commits into bitcoin:master from glozow:2024-10-30110-followups changing 4 files +10 −6
  1. glozow commented at 12:46 pm on October 31, 2024: member

    Addresses some remaining followups from #30110:

  2. [doc] comment fixups from n30110 917ab810d9
  3. [fuzz] allow negative time jumps in txdownloadman_impl 8351562bec
  4. glozow added the label Docs on Oct 31, 2024
  5. glozow added the label Tests on Oct 31, 2024
  6. DrahtBot commented at 12:46 pm on October 31, 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/31190.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK instagibbs, theStack, naumenkogs

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

  7. DrahtBot added the label CI failed on Oct 31, 2024
  8. DrahtBot removed the label CI failed on Oct 31, 2024
  9. fanquake requested review from theStack on Nov 5, 2024
  10. fanquake requested review from instagibbs on Nov 5, 2024
  11. fanquake requested review from marcofleon on Nov 5, 2024
  12. instagibbs commented at 4:34 pm on November 5, 2024: member
    is this ready for undraft?
  13. glozow marked this as ready for review on Nov 5, 2024
  14. naumenkogs commented at 9:22 am on November 6, 2024: member

    ACK 8351562bec6081eda2a600bfe4edeb264a9dee0b

    I haven’t verified what happens if time goes below 0

  15. dergoegge commented at 10:32 am on November 6, 2024: member

    oss-fuzz found a crash in txdownloadman_impl, perhaps worth including the fix here or in another followup:

    0$ FUZZ=txdownloadman_impl src/test/fuzz/fuzz clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt
1src/test/fuzz/txdownloadman.cpp:290 CheckInvariants: Assertion `txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT' failed.

    clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt

  16. glozow commented at 12:01 pm on November 6, 2024: member
    @dergoegge is that on master, or with the changes in this PR? Would like to see if it could be a negative time thing.
  17. dergoegge commented at 12:02 pm on November 6, 2024: member

    is that on master

    Sorry I should have clarified, it is on master.

  18. glozow commented at 12:08 pm on November 6, 2024: member

    Thanks @dergoegge, I probably also could have inferred from it being oss-fuzz.

    Even though I wrote it, the assertion looks wrong 😅 overloaded inflight is handled with just a delay

    https://github.com/bitcoin/bitcoin/blob/80cb630bd945e7f1777c7bab041c411b6916e651/src/node/txdownloadman_impl.cpp#L199

    I think what I was trying to do was ensure MAX_PEER_TX_ANNOUNCEMENTS isn’t busted. Adding a fix here.

  19. fuzz fix: assert MAX_PEER_TX_ANNOUNCEMENTS is not exceeded 
    Previously this assertion checked MAX_PEER_TX_REQUEST_IN_FLIGHT was not
exceeded. However, this property is not actually enforced; it is just
used to determine when a peer is overloaded.
    5dc94d13d4
  20. instagibbs approved
  21. instagibbs commented at 2:53 pm on November 6, 2024: member
    ACK 5dc94d13d419b8d5e543cb50edeb872335c090e7
  22. DrahtBot requested review from naumenkogs on Nov 6, 2024
  23. in src/test/fuzz/txdownloadman.cpp:290 in 5dc94d13d4 
    286@@ -287,7 +287,7 @@ static void CheckInvariants(const node::TxDownloadManagerImpl& txdownload_impl,
287     // We should never have more than the maximum in-flight requests out for a peer.
288     for (NodeId peer = 0; peer < NUM_PEERS; ++peer) {
289         if (!HasRelayPermissions(peer)) {
290-            Assert(txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT);
291+            Assert(txdownload_impl.m_txrequest.Count(peer) <= node::MAX_PEER_TX_ANNOUNCEMENTS);
    instagibbs commented at 2:55 pm on November 6, 2024:

    any value in

    0            Assert(txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_ANNOUNCEMENTS);
1            Assert(txdownload_impl.m_txrequest.Count(peer) <= node::MAX_PEER_TX_ANNOUNCEMENTS);

    ?

    glozow commented at 4:52 pm on November 6, 2024:
    What do you mean any value in?
    instagibbs commented at 4:56 pm on November 6, 2024:
    would the suggested change potentially give coverage
    naumenkogs commented at 11:16 am on November 7, 2024:
    Perhaps CountInFlight should check against MAX_PEER_TX_REQUEST_IN_FLIGHT and Count against MAX_PEER_TX_ANNOUNCEMENTS, separately? I think that’s what Greg wanted to suggest.
    marcofleon commented at 11:41 am on November 7, 2024:

    Perhaps CountInFlight should check against MAX_PEER_TX_REQUEST_IN_FLIGHT and Count against MAX_PEER_TX_ANNOUNCEMENTS, separately?

    Checking CountInFlight against MAX_PEER_TX_REQUEST_IN_FLIGHT is what the fuzzer crashed on initially.

    Could be misunderstanding here, but wouldn’t m_txrequest.Count(peer) always be greater than (or equal?) to m_txrequest.CountInFlight(peer), so the additional assert would be redundant?

  24. theStack approved
  25. theStack commented at 6:25 am on November 7, 2024: contributor
    ACK 5dc94d13d419b8d5e543cb50edeb872335c090e7
  26. Joj501 approved
  27. Joj501 commented at 6:45 am on November 7, 2024: none
    Connect with my wallet address
  29. in src/test/fuzz/txdownloadman.cpp:287 in 5dc94d13d4 
    286@@ -287,7 +287,7 @@ static void CheckInvariants(const node::TxDownloadManagerImpl& txdownload_impl,
287     // We should never have more than the maximum in-flight requests out for a peer.
    marcofleon commented at 11:42 am on November 7, 2024:
    Update comment for the new assert?
  30. instagibbs commented at 11:43 am on November 7, 2024: member

    Assuming we implemented everything correctly yes!

    On Thu, Nov 7, 2024, 6:42 AM marcofleon @.***> wrote:

    @.**** commented on this pull request.

    In src/test/fuzz/txdownloadman.cpp https://github.com/bitcoin/bitcoin/pull/31190#discussion_r1832534828:

    @@ -287,7 +287,7 @@ static void CheckInvariants(const node::TxDownloadManagerImpl& txdownload_impl, // We should never have more than the maximum in-flight requests out for a peer. for (NodeId peer = 0; peer < NUM_PEERS; ++peer) { if (!HasRelayPermissions(peer)) {

    •        Assert(txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT);

    •        Assert(txdownload_impl.m_txrequest.Count(peer) <= node::MAX_PEER_TX_ANNOUNCEMENTS);

    Could be misunderstanding here, but wouldn’t m_txrequest.Count(peer) always be greater than (or equal?) to m_txrequest.CountInFlight(peer), so the additional assert would be redundant?

    — Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/31190#discussion_r1832534828, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU7JC45F4HYWMEIFEK3Z7NGZXAVCNFSM6AAAAABQ6GY2POVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRQG4ZTEMBWGA . You are receiving this because your review was requested.Message ID: @.***>

  31. marcofleon commented at 11:54 am on November 7, 2024: contributor

    Assuming we implemented everything correctly yes!

    hmm… you make a good point

  32. fanquake merged this on Nov 7, 2024
  33. fanquake closed this on Nov 7, 2024


glozow DrahtBot instagibbs naumenkogs dergoegge marcofleon theStack Joj501


naumenkogs

Labels
Docs Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me