-
Notifications
You must be signed in to change notification settings - Fork 331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handling multiple blobTxs in a block #1910
Comments
In addition to the reordering problem caused by using different gas prices, there is also the problem that the proposer will group all the PFBs at the end of the block. Imagine the following scenario: A rollup sends a PFB with nonce 1 and then sends a Send tx with nonce 2. The proposer shuffles the PFB after the Send Tx according to the block validity rules but then drops both of them as being invalid. The user is stuck in a deadlock where no more transactions can be submitted until the user submits two "cancel" transactions with a higher priority. |
What are our current options for handling multiple PFBs per Block? What is the Limit for how many blobs fit into a PFB ? |
There is no current limit (so long as the blobs can fit in a square)
I think the best solution going forward is a combination of reordering prioritised txs such that the sequencer numbers for each account is ordered and using nonce lanes For now, we just need celestia-node to locally increment the sequence number for every tx they sign (as opposed to querying state which won't be updated if there are multiple txs by the same account within a block) |
Closes: #1910 This covers most cases by serializing the actual broadcasts to the consensus node and enabling resubmissions in the case that there is a sequence mismatch. This covers most fail cases with the possible exception of proposal nodes receiving the transactions in the reverse order to the initial nodes that the user broadcasted to There are also some interesting side affects that need to be handled when an existing accepted transaction is later kicked out of the mempool via CheckTx but overall I think this is a huge improvement for the UX of users
Closes: #1910 This covers most cases by serializing the actual broadcasts to the consensus node and enabling resubmissions in the case that there is a sequence mismatch. This covers most fail cases with the possible exception of proposal nodes receiving the transactions in the reverse order to the initial nodes that the user broadcasted to There are also some interesting side affects that need to be handled when an existing accepted transaction is later kicked out of the mempool via CheckTx but overall I think this is a huge improvement for the UX of users (cherry picked from commit deefb54) # Conflicts: # Makefile # app/errors/insufficient_gas_price_test.go # app/errors/nonce_mismatch_test.go # app/test/big_blob_test.go # app/test/priority_test.go # go.work.sum # pkg/user/signer.go # pkg/user/signer_test.go # test/util/blobfactory/payforblob_factory.go # test/util/blobfactory/test_util.go # test/util/direct_tx_gen.go # x/blob/types/blob_tx_test.go
Closes: celestiaorg#1910 This covers most cases by serializing the actual broadcasts to the consensus node and enabling resubmissions in the case that there is a sequence mismatch. This covers most fail cases with the possible exception of proposal nodes receiving the transactions in the reverse order to the initial nodes that the user broadcasted to There are also some interesting side affects that need to be handled when an existing accepted transaction is later kicked out of the mempool via CheckTx but overall I think this is a huge improvement for the UX of users
Closes: #1910 This covers most cases by serializing the actual broadcasts to the consensus node and enabling resubmissions in the case that there is a sequence mismatch. This covers most fail cases with the possible exception of proposal nodes receiving the transactions in the reverse order to the initial nodes that the user broadcasted to There are also some interesting side affects that need to be handled when an existing accepted transaction is later kicked out of the mempool via CheckTx but overall I think this is a huge improvement for the UX of users<hr>This is an automatic backport of pull request #3196 done by [Mergify](https://mergify.com). Co-authored-by: Callum Waters <cmwaters19@gmail.com>
Summary
It will be typical in most cases that a rollup will be producing blocks faster than Celestia network finalizes them (the network will initially be aiming for 15 second block times). This brings about a set of potential UX problems that we should look to address. Perhaps the most important has to do with nonces.
Problem Definition
PFB A is submitted before PFB B but B has a higher fee and hence has greater priority. The block producer naively orders them as PFB B and then PFB A. Both get rejected because their nonce numbers are invalid thus both get dropped
Proposal
There are a few possible solutions:
Group transactions together by signer, using a weighted gas price to indicate the priority of the bundle of txs. The bundle would contains transactions ordered by their nonce. This would have the property that either all transactions would make it or none. We could cap the bundle size before a new bundle was created. It's likely that the gas price will be very similar but if the gas price was different from blobTx to blobTx this could have mixed results.
Taking the list of txs ordered by priority, the proposer would loop through all transaction, popping out any transaction that had an incorrect nonce and tracking the latest nonce of all the signers involved in the proposed block. It would then attempt to push the transaction back into the list after the tx with the correct nonce.
If blobTx order isn't so important to the rollup (it can interpret its own ordering of the transactions) then we could also look to use nonce lanes. If a sequencer submits 10 blobs per 1 Celestia blob, it could use 10 different lanes to divide it's txs across, that way the success of one transaction does not affect the success of others. (See cosmos/cosmos-sdk#13009)
cc @nashqueue
For Admin Use
The text was updated successfully, but these errors were encountered: