Account for full transaction encoding when batching messages #2575
Conversation
relayer/src/chain/cosmos/batch.rs
Outdated
| fn test_fixture() -> (TxConfig, KeyEntry, Account) { | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
@soareschen Is there anything in the test framework to provide usable values here? Unfortunately, the unit tests become more complicated.
There was a problem hiding this comment.
There could be a mock instance for TxConfig, if you don't call the API URLs. For KeyEntry you would have to construct a private key pair. For Account the main essential information is the account sequence, which could be set to something like 0 or 999999.
There was a problem hiding this comment.
Good, I'm creating mock values (not pushed yet), but had doubts about all fields of TxConfig.
| config: &TxConfig, | ||
| max_msg_num: MaxMsgNum, | ||
| max_tx_size: MaxTxSize, | ||
| key_entry: &KeyEntry, | ||
| account: &Account, | ||
| tx_memo: &Memo, |
There was a problem hiding this comment.
This parameter lists is used repetitively, maybe there should be a struct to arrange some of them.
There was a problem hiding this comment.
It was originally not repetitive, but you made them repetitive now. The point of explicit parameters is to make it obvious of cascading dependencies and help us avoid that.
Instead, why don't just pass tx_envelope_len as a parameter and call encoded_tx_len from the call site? It also looks like encoded_tx_len can be estimated once and applicable for all transactions, rather than recomputing it every time.
There was a problem hiding this comment.
@soareschen The same parameters are used in other functions in this module, I did not want to break the pattern at this point. As you point out, this flattening was done in an earlier refactoring to reduce unnecessary data dependencies and useless mutability, but I think we should follow it by some creative parameter grouping where it makes sense, to avoid excessive repetition.
It also looks like
encoded_tx_lencan be estimated once and applicable for all transactions
I was not so sure, there may be some variation in how e.g. addresses are encoded, and there's also the memo field. There needs to be some context above the send_batched_messages_and_wait_* API to safely make this assumption.
There was a problem hiding this comment.
The key challenge for using a common struct in this case is that the references are borrowed from different places. So if we define a struct like TxContext we would have to move or clone the values into it. And if we define a struct containing references like TxContext<'a>, then it does not really contribute much other than being a syntactic sugar. It would also risk overgeneralizing functions, especially when catering for those few places that requires a &mut Account.
That said, if we want to refactor the code, it would better to define a proper TxHandler struct that specializes in sending transactions, and decouple it from the ChainHandle implementation, which is the real source of this mess.
The address should all have the same length, and is the same within the same ChainHandle. Same for the memo field, which has the same value configured. The only thing that could potentially change length is the account sequence, unless it has a fixed length encoding like u64.
There was a problem hiding this comment.
t would better to define a proper
TxHandlerstruct that specializes in sending transactions, and decouple it from theChainHandleimplementation, which is the real source of this mess.
I agree, splitting the "god trait" into smaller traits is a good way to refactor.
Same for the memo field, which has the same value configured.
So there might be a case for a struct that contains these less-variable parameters.
The only thing that could potentially change length is the account sequence, unless it has a fixed length encoding like u64.
Yes, there is a number of these varint fields and length delimiters that can grow between one tx to another, so you either have to compute the length on the fully composed transaction as it would be sent, or put in some upper bound for the overhead, neither of which is an ideal approach.
The function evaluates the length of an encoded transaction, to perform more accurate batching.
mzabaluev
force-pushed
the
mikhail/tx-size-fixes
branch
from
03b2fb0 to
4f90ce3
Compare
Move utilities from chain::cosmos::batch to share them with other modules' unit tests.
In batch_messages, the varint encoding of the length of the tx_body field of TxRaw may increase the length of the envelope as the body size grows.
The tx_body member of TxRaw encodes more than the messages, so its total length needs to be taken into account when estimating the total transaction size from the previously computed envelope and the vector of messages.
This reverts commit 6a8e31f.
When the TxBody struct is completely empty (all fields have default values), it is encoded as an empty array and the body_bytes field is then also omitted from the encoding of TxRaw. Add transaction size verification to the unit tests of batch_messages, by creating encoded tx from the batched messages (using the maximum fee to keep unit tests simple) and checking the size corresponds to the `MaxTxSize` parameter.
Test larger numbers of messages, up to 100 (maximum allowed per MaxMsgNum limits), with length increasing in the arithmetic progression. For each round, make sure that the size limit is just equal to the size of an encoded tx with all messages except the last one. Verify that the last message is spilled into the next batch and the resulting tx size for the first batch equals the limit.
…lsystems#2575) * Remove a useless clone * De-clone estimate_tx_fees * cosmos: More de-owning around sign_and_encode_tx * Simplify prepending of ClientUpdate * cosmos: encoded_tx_len function The function evaluates the length of an encoded transaction, to perform more accurate batching. * Account for field encoding in tx batch size * Account for tx envelope in message batching * More credible test fixture for batch_messages * Fix calculations in batch_messages, adjust tests * Initialize KeyEntry data from a seed file * relayer: module chain::cosmos::test_utils Move utilities from chain::cosmos::batch to share them with other modules' unit tests. * Take tx_body varint lenth delimiter into account In batch_messages, the varint encoding of the length of the tx_body field of TxRaw may increase the length of the envelope as the body size grows. * Account for body encoding when batching messages The tx_body member of TxRaw encodes more than the messages, so its total length needs to be taken into account when estimating the total transaction size from the previously computed envelope and the vector of messages. * Document EncodedTxMetrics * Revert "relayer: module chain::cosmos::test_utils" This reverts commit 6a8e31f. * batch: fix size estimation for empty body When the TxBody struct is completely empty (all fields have default values), it is encoded as an empty array and the body_bytes field is then also omitted from the encoding of TxRaw. Add transaction size verification to the unit tests of batch_messages, by creating encoded tx from the batched messages (using the maximum fee to keep unit tests simple) and checking the size corresponds to the `MaxTxSize` parameter. * Improve batch_does_not_exceed_max_tx_size test Test larger numbers of messages, up to 100 (maximum allowed per MaxMsgNum limits), with length increasing in the arithmetic progression. For each round, make sure that the size limit is just equal to the size of an encoded tx with all messages except the last one. Verify that the last message is spilled into the next batch and the resulting tx size for the first batch equals the limit. * Remove an unnecessary branch * Add changelog entry Co-authored-by: Romain Ruetschi <romain@informal.systems>
Simple, Cosmos-specific fix for #2560
Closes: #2560
Description
Use the estimated encoded transaction size to ensure that the transaction created from each message batch does not exceed
max_tx_size.PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.