TxDownloadManager followups #31190

pull glozow wants to merge 3 commits into bitcoin:master from glozow:2024-10-30110-followups changing 4 files +10 −6

glozow commented at 12:46 PM on October 31, 2024: member
Addresses some remaining followups from #30110:
[doc] comment fixups from n30110 917ab810d9
[fuzz] allow negative time jumps in txdownloadman_impl 8351562bec
glozow added the label Docs on Oct 31, 2024
glozow added the label Tests on Oct 31, 2024
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.
DrahtBot added the label CI failed on Oct 31, 2024
DrahtBot removed the label CI failed on Oct 31, 2024
fanquake requested review from theStack on Nov 5, 2024
fanquake requested review from instagibbs on Nov 5, 2024
fanquake requested review from marcofleon on Nov 5, 2024
instagibbs commented at 4:34 PM on November 5, 2024: member

is this ready for undraft?
glozow marked this as ready for review on Nov 5, 2024
naumenkogs commented at 9:22 AM on November 6, 2024: contributor

ACK 8351562bec6081eda2a600bfe4edeb264a9dee0b

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

Type	Reviewers
ACK	instagibbs, theStack, naumenkogs

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:

$ FUZZ=txdownloadman_impl src/test/fuzz/fuzz clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt
src/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

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.
dergoegge commented at 12:02 PM on November 6, 2024: member

is that on master

Sorry I should have clarified, it is on master.
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.

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

instagibbs approved
instagibbs commented at 2:53 PM on November 6, 2024: member

ACK 5dc94d13d419b8d5e543cb50edeb872335c090e7
DrahtBot requested review from naumenkogs on Nov 6, 2024
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

Assert(txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_ANNOUNCEMENTS); 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?
theStack approved
theStack commented at 6:25 AM on November 7, 2024: contributor

ACK 5dc94d13d419b8d5e543cb50edeb872335c090e7
naumenkogs commented at 11:18 AM on November 7, 2024: contributor

ACK https://github.com/bitcoin/bitcoin/commit/5dc94d13d419b8d5e543cb50edeb872335c090e7

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?

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: @.***>
marcofleon commented at 11:54 AM on November 7, 2024: contributor

Assuming we implemented everything correctly yes!

hmm... you make a good point
fanquake merged this on Nov 7, 2024
fanquake closed this on Nov 7, 2024
TheCharlatan referenced this in commit a73b2bd0f0 on Nov 14, 2024
bug-castercv502 referenced this in commit fdcc066ca0 on Sep 28, 2025
bitcoin locked this on Nov 7, 2025

Contributors

Review Requested
naumenkogs

Labels

Docs Tests

Linked (view graph)

#27463 Package Relay Project Tracking #30110 refactor: TxDownloadManager + fuzzing

TxDownloadManager followups #31190

Code Coverage & Benchmarks

Reviews

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