fix: track full_transactions propagation when packet size limited

[bemevolent]

New to the reth codebase and rust, so feel free to correct any misunderstandings or bad patterns.

In transactions.rs when full_transactions is being built:

reth/crates/net/network/src/transactions.rs

Lines 249 to 254 in f41386d

	for tx in to_propagate.iter() {
	if peer.transactions.insert(tx.hash()) {
	hashes.push(tx);
	full_transactions.push(tx);
	}
	}

hashes and full_transactions can diverge when a .push() to the latter is limited by MAX_FULL_TRANSACTIONS_PACKET_SIZE

reth/crates/net/network/src/transactions.rs

Lines 636 to 640 in f41386d

	fn push(&mut self, transaction: &PropagateTransaction) {
	let new_size = self.total_size + transaction.size;
	if new_size > MAX_FULL_TRANSACTIONS_PACKET_SIZE {
	return
	}

so when PropagatedTransactions is being updated in the snippet below, it sends the limited set full_transactions to peers but then updates the tracking with the full tx hash list of new_pooled_hashes

reth/crates/net/network/src/transactions.rs

Lines 271 to 275 in f41386d

	self.network.send_transactions(*peer_id, full_transactions.build());
	for hash in new_pooled_hashes.into_iter_hashes() {
	propagated.0.entry(hash).or_default().push(PropagateKind::Full(*peer_id));
	}

[onbjerg]

I think perhaps a better way to do this is to have push return a boolean to denote whether or not it was added - so it would return false if it was limited. Then, when inserting hashes, it would be something like:

if full_transactions.push(tx) {
  hashes.push(tx);
}

This current form has some duplicate/semi-dead code, e.g. new_pooled_hashes is only used for the empty check on L258, and in the first branch of the if statement on L260 (which suffers from the same issue you described I think?)

[bemevolent]

That doesn't feel like the right approach? The way I understand it is basically we're splitting up peers and deciding to send either full tx objects or hashes. And within each of those we'd like to send the most tx data up to the defined network limits.

Using your suggestion we would unnecessarily limit hashes based on full_transaction even in the branch where we're just sending hashes to peers. It seems fine for hashes and full_transactions to diverge, as long as propagated properly tracks what was actually sent to peers

[mattsse]

you're absoultely right, they can diverge and this is a very nice find, tysm

only a smol nit

[codecov]

Codecov Report

Merging #3993 (9ca0ea9) into main (f41386d) will decrease coverage by 0.24%.
Report is 15 commits behind head on main.
The diff coverage is 0.00%.

Files Changed	Coverage Δ
crates/net/network/src/transactions.rs	50.78% <0.00%> (-0.45%)	⬇️

... and 50 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	16.23% <0.00%> (-0.11%)	⬇️
unit-tests	64.13% <0.00%> (-0.21%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	25.63% <ø> (-1.05%)	⬇️
blockchain tree	83.04% <ø> (ø)
pipeline	90.03% <ø> (+0.21%)	⬆️
storage (db)	74.30% <ø> (-0.01%)	⬇️
trie	94.70% <ø> (ø)
txpool	45.40% <ø> (-0.61%)	⬇️
networking	77.48% <0.00%> (-0.18%)	⬇️
rpc	58.37% <ø> (-0.10%)	⬇️
consensus	63.51% <ø> (ø)
revm	33.10% <ø> (+0.01%)	⬆️
payload builder	6.58% <ø> (ø)
primitives	88.03% <ø> (+0.12%)	⬆️

unblocking merge