Increase maximum size of transaction notifications#7993
Conversation
tomaka
added
A0-please_review
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Change looks good to me. I don't have an opinion on the constant 16 MiB.
| // Must be equal to `max(MAX_BLOCK_ANNOUNCE_SIZE, MAX_TRANSACTIONS_SIZE)`. | ||
| pub(crate) const BLOCK_ANNOUNCES_TRANSACTIONS_SUBSTREAM_SIZE: u64 = 16 * 1024 * 1024; |
There was a problem hiding this comment.
| // 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:
There was a problem hiding this comment.
Being able to cast a boolean into a usize is completely a hack in my opinion.
|
|
||
| /// 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; |
There was a problem hiding this comment.
This means a block can be in maximum 1MIB?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
bot merge |
|
Trying merge. |
ghost
merged commit 
3 participants
#7925 reduced the maximum notification sizes for transactions to 1MiB.
I didn't realize that, since we're sending runtime upgrades through transactions, 1MiB isn't enough.
This PR bumps this to 16MiB.