Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Increase maximum size of transaction notifications #7993

Merged
1 commit merged into from
Jan 27, 2021
Merged

Increase maximum size of transaction notifications #7993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 14 additions & 5 deletions client/network/src/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,23 @@ const TICK_TIMEOUT: time::Duration = time::Duration::from_millis(1100);
/// Interval at which we propagate transactions;
const PROPAGATE_TIMEOUT: time::Duration = time::Duration::from_millis(2900);

/// Maximim number of known block hashes to keep for a peer.
/// Maximum number of known block hashes to keep for a peer.
const MAX_KNOWN_BLOCKS: usize = 1024; // ~32kb per peer + LruHashSet overhead
/// Maximim number of known transaction hashes to keep for a peer.
/// Maximum number of known transaction hashes to keep for a peer.
///
/// This should be approx. 2 blocks full of transactions for the network to function properly.
const MAX_KNOWN_TRANSACTIONS: usize = 10240; // ~300kb per peer + overhead.

/// Maximim number of transaction validation request we keep at any moment.
/// Maximum allowed size for a block announce.
const MAX_BLOCK_ANNOUNCE_SIZE: u64 = 1024 * 1024;
Copy link
Member

@bkchr bkchr Jan 27, 2021

Choose a reason for hiding this comment

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

This means a block can be in maximum 1MIB?

Copy link
Contributor

@mxinden mxinden Jan 27, 2021

Choose a reason for hiding this comment

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

If I understand correctly the block announcement can be at most 1 MiB. The block itself will be transferred separately via the block request response protocol.

Copy link
Contributor Author

@tomaka tomaka Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We have this opaque "extra data" field in block announcements that is used in Polkadot, but I have no idea of its purpose nor its size.

Copy link
Member

@bkchr bkchr Jan 27, 2021

Choose a reason for hiding this comment

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

Ahh. That isn't used by Polkadot. This is used by Cumulus. 1MIB should be enough. However, we really should make these values here configurable. Because Substrate != Polkadot.

/// Maximum allowed size for a transactions notification.
const MAX_TRANSACTIONS_SIZE: u64 = 16 * 1024 * 1024;

/// Maximum size used for notifications in the block announce and transaction protocols.
// Must be equal to `max(MAX_BLOCK_ANNOUNCE_SIZE, MAX_TRANSACTIONS_SIZE)`.
pub(crate) const BLOCK_ANNOUNCES_TRANSACTIONS_SUBSTREAM_SIZE: u64 = 16 * 1024 * 1024;
Comment on lines +88 to +89
Copy link
Contributor

@mxinden mxinden Jan 27, 2021

Choose a reason for hiding this comment

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

Suggested change
// Must be equal to `max(MAX_BLOCK_ANNOUNCE_SIZE, MAX_TRANSACTIONS_SIZE)`.
pub(crate) const BLOCK_ANNOUNCES_TRANSACTIONS_SUBSTREAM_SIZE: u64 = 16 * 1024 * 1024;
pub(crate) const BLOCK_ANNOUNCES_TRANSACTIONS_SUBSTREAM_SIZE: u64 = [MAX_BLOCK_ANNOUNCE_SIZE, MAX_TRANSACTIONS_SIZE][(MAX_BLOCK_ANNOUNCE_SIZE < MAX_TRANSACTIONS_SIZE) as usize];

Would make the comment obsolete. Not sure it is worth the complexity. See link below for details:

https://stackoverflow.com/questions/53619695/calculating-maximum-value-of-a-set-of-constant-expressions-at-compile-time

Copy link
Contributor Author

@tomaka tomaka Jan 27, 2021

Choose a reason for hiding this comment

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

Being able to cast a boolean into a usize is completely a hack in my opinion.


/// Maximum number of transaction validation request we keep at any moment.
const MAX_PENDING_TRANSACTIONS: usize = 8192;

/// Current protocol version.
Expand Down Expand Up @@ -483,8 +492,8 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
versions,
build_status_message::<B>(&config, best_number, best_hash, genesis_hash),
peerset,
iter::once((block_announces_protocol, block_announces_handshake, 1024 * 1024))
.chain(iter::once((transactions_protocol, vec![], 1024 * 1024)))
iter::once((block_announces_protocol, block_announces_handshake, MAX_BLOCK_ANNOUNCE_SIZE))
.chain(iter::once((transactions_protocol, vec![], MAX_TRANSACTIONS_SIZE)))
.chain(network_config.extra_sets.iter().map(|s| (
s.notifications_protocol.clone(),
handshake_message.clone(),
Expand Down
10 changes: 8 additions & 2 deletions client/network/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ use sp_runtime::traits::{Block as BlockT, NumberFor};
use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnboundedSender};
use std::{
borrow::Cow,
cmp,
collections::{HashMap, HashSet},
convert::TryFrom as _,
fs,
Expand Down Expand Up @@ -310,8 +311,13 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> {
.map(|cfg| usize::try_from(cfg.max_notification_size).unwrap_or(usize::max_value()));

// A "default" max is added to cover all the other protocols: ping, identify,
// kademlia.
let default_max = 1024 * 1024;
// kademlia, block announces, and transactions.
let default_max = cmp::max(
1024 * 1024,
usize::try_from(protocol::BLOCK_ANNOUNCES_TRANSACTIONS_SUBSTREAM_SIZE)
.unwrap_or(usize::max_value())
);

iter::once(default_max)
.chain(requests_max).chain(responses_max).chain(notifs_max)
.max().expect("iterator known to always yield at least one element; qed")
Expand Down