Set transaction size limit #4107
Conversation
Longarithm
changed the title
core/primitives/src/transaction.rs
Outdated
| @@ -72,6 +72,14 @@ impl Action { | |||
| _ => 0, | |||
| } | |||
| } | |||
| #[cfg(feature = "protocol_feature_tx_size_limit")] | |||
There was a problem hiding this comment.
I suggest for the transaction size to use the size of the transaction serialized as borsh. This will remove potential tricky angles of attack, e.g. it is still possible to create a heavy transaction that adds a very large number of function access keys with long method names. Also, that removes ambiguity in interpreting what transaction size is, so e.g. we can say that block size is sum of the transaction sizes plus header data.
Ideally, we would add a trait to Borsh called BorshSize that implements borsh_size() -> u64 method which we can use similarly to BorshSerialize and BorshDeserialize, like this: #[derive(BorshSize)]. AFAIR there might some places in our code where we measure the size of the objects by first serializing them in Borsh and then counting bytes, so it might be useful there too. It shouldn't be difficult to add to borsh and will probably be largely copy-paste of https://github.com/near/borsh-rs/blob/master/borsh/src/ser/mod.rs
There was a problem hiding this comment.
Yeah agree with what Max said here
There was a problem hiding this comment.
Thank you!
Changed the way of size computation and added a separate issue for adding new trait.
| @@ -57,6 +57,19 @@ pub fn validate_transaction( | |||
| return Err(InvalidTxError::InvalidSignature.into()); | |||
| } | |||
|
|
|||
| #[cfg(feature = "protocol_feature_tx_size_limit")] | |||
There was a problem hiding this comment.
This check should only be done once. There is no reason to check transactions for which you already checked. For example, inside prepare_transactions it doesn't make sense to check again because this check should have been done when the transaction is received. To further reduce the overhead, you could change NetworkClientMessage::Transaction to include the transaction size. It probably even makes sense to implement custom borsh deserializer that would fail when the a larger-than-allowed transaction arrives on the network (this can potentially be done in a separate PR)
There was a problem hiding this comment.
It seems that at least one serialization is already happening inside SignedTransaction::new, to save Transaction hash. So I saved size there in the same fashion and started to use it, which shouldn't give significant overhead.
There was a problem hiding this comment.
Yeah that makes sense
| @@ -57,6 +57,19 @@ pub fn validate_transaction( | |||
| return Err(InvalidTxError::InvalidSignature.into()); | |||
| } | |||
|
|
|||
| #[cfg(feature = "protocol_feature_tx_size_limit")] | |||
There was a problem hiding this comment.
Yeah that makes sense
|
Please run nayduck |
|
@Longarithm is this PR ready for review? |
|
Not yet, I got some Nayduck tests failing: http://nayduck.eastus.cloudapp.azure.com:3000/#/run/1401 |
|
http://nayduck.eastus.cloudapp.azure.com:3000/#/run/1414 - runs for |
Longarithm
requested review from
evgenykuzyakov,
frol and
SkidanovAlex
as code owners
No description provided.