Fix wasteful bandwidth usage by transaction gossip

[nazar-pc]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Motivation

Bandwidth usage is wasteful on node is synced due to size of transactions. Right now transactions seem to be sent in full:

polkadot-sdk/substrate/client/network/transactions/src/lib.rs

Lines 470 to 509 in 26c11fc

	fn do_propagate_transactions(
	&mut self,
	transactions: &[(H, B::Extrinsic)],
	) -> HashMap<H, Vec<String>> {
	let mut propagated_to = HashMap::<_, Vec<_>>::new();
	let mut propagated_transactions = 0;
	for (who, peer) in self.peers.iter_mut() {
	// never send transactions to the light node
	if matches!(peer.role, ObservedRole::Light) {
	continue
	}
	let (hashes, to_send): (Vec<_>, Vec<_>) = transactions
	.iter()
	.filter(|(hash, _)| peer.known_transactions.insert(hash.clone()))
	.cloned()
	.unzip();
	propagated_transactions += hashes.len();
	if !to_send.is_empty() {
	for hash in hashes {
	propagated_to.entry(hash).or_default().push(who.to_base58());
	}
	trace!(target: "sync", "Sending {} transactions to {}", to_send.len(), who);
	// Historically, the format of a notification of the transactions protocol
	// consisted in a (SCALE-encoded) `Vec<Transaction>`.
	// After RFC 56, the format was modified in a backwards-compatible way to be
	// a (SCALE-encoded) tuple `(Compact(1), Transaction)`, which is the same encoding
	// as a `Vec` of length one. This is no coincidence, as the change was
	// intentionally done in a backwards-compatible way.
	// In other words, the `Vec` that is sent below **must** always have only a single
	// element in it.
	// See <https://github.com/polkadot-fellows/RFCs/blob/main/text/0056-one-transaction-per-notification.md>
	for to_send in to_send {
	let _ = self
	.notification_service
	.send_sync_notification(who, vec![to_send].encode());
	}

I understand why sending a batch of transactions that fits into TCP packet would makes sense from efficiency point of view, but sending full transactions instead of hashes in all cases is very wasteful, especially when they are large in size. The only benefit of doing this is saving one roundtrip for the price of substantial bandwidth increase.

In fact RFC-0056: Enforce only one transaction per notification is potentially a step backwards in this area IMO.

By offloading decision to combine multiple notifications to the lower level that knows nothing about transactions, we have lost the ability to decide whether a batch of transactions fits into TCP packet to optimize delivery.

I say "potentially" because as can be seen from the code above in case a batch of transactions come in they are send separately in a loop, so it is theoretically possible to roughly estimate how many individual notifications will fit into a packet and decide whether to send them in full or send just hashes.

Request

We already implemented a variant of BIP152 Compact Blocks at Subspace for blocks using #1524, now something similar needs to be done for transactions.

Transaction gossip protocol should be improved to not send transactions in full unconditionally, instead it should have more intelligent logic that decides between sending full transactions or just transaction hashes based on how many transactions there is and how many of them fit into TCP packet.

Solution

I propose the following:

• when a batch of transactions is about to be gossiped, send them as transaction hashes by default
• [optional] for latency optimization if after concatenating transaction hashes there is still space in the payload that fits into IPv6 TCP packet, try to expand as many transactions as possible from hashes into full transaction
• when unknown transaction is received from a peer, request it explicitly

This will ensure that large transactions are not sent multiple times, which is especially bad when there are slower peer connections that will result in the same transaction being in-flight multiple times.

The cost is increased latency for additional roundtrip in case peer doesn't have the transaction locally.

This will likely require RFC or a few, but would be great to get at least some initial feedback first.

Are you willing to help with this request?

Maybe (please elaborate above)