Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TxDownloadManager followups #31190

Merged
merged 3 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/node/txdownloadman.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ class TxDownloadManager {
/** Deletes all txrequest announcements and orphans for a given peer. */
void DisconnectedPeer(NodeId nodeid);

/** New inv has been received. May be added as a candidate to txrequest.
/** Consider adding this tx hash to txrequest. Should be called whenever a new inv has been received.
* Also called internally when a transaction is missing parents so that we can request them.
* @param[in] p2p_inv When true, only add this announcement if we don't already have the tx.
* Returns true if this was a dropped inv (p2p_inv=true and we already have the tx), false otherwise. */
bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv);
Expand Down
4 changes: 3 additions & 1 deletion src/node/txdownloadman_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ class TxDownloadManagerImpl {
void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info);
void DisconnectedPeer(NodeId nodeid);

/** New inv has been received. May be added as a candidate to txrequest. */
/** Consider adding this tx hash to txrequest. Should be called whenever a new inv has been received.
* Also called internally when a transaction is missing parents so that we can request them.
*/
bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv);

/** Get getdata requests to send. */
Expand Down
7 changes: 4 additions & 3 deletions src/test/fuzz/txdownloadman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment for the new assert?

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any value in

Suggested change
Assert(txdownload_impl.m_txrequest.Count(peer) <= node::MAX_PEER_TX_ANNOUNCEMENTS);
Assert(txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_ANNOUNCEMENTS);
Assert(txdownload_impl.m_txrequest.Count(peer) <= node::MAX_PEER_TX_ANNOUNCEMENTS);

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean any value in?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would the suggested change potentially give coverage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@marcofleon marcofleon Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
txdownload_impl.m_txrequest.SanityCheck();
Expand Down Expand Up @@ -430,8 +430,9 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize)
}
);

// Jump ahead in time
time += fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS);
if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1;
time += time_skip;
CheckInvariants(txdownload_impl, max_orphan_count);
}
// Disconnect everybody, check that all data structures are empty.
Expand Down
2 changes: 1 addition & 1 deletion src/test/txdownload_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ struct Behaviors {
bool m_ignore_inv_txid;
bool m_ignore_inv_wtxid;

// Constructor. We are passing and casting ints because they are more readable in a table (see all_expected_results).
// Constructor. We are passing and casting ints because they are more readable in a table (see expected_behaviors).
Behaviors(bool txid_rejects, bool wtxid_rejects, bool txid_recon, bool wtxid_recon, bool keep, bool txid_inv, bool wtxid_inv) :
m_txid_in_rejects(txid_rejects),
m_wtxid_in_rejects(wtxid_rejects),
Expand Down
Loading