Skip to content
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

Closed
4 tasks
cmwaters opened this issue Jun 12, 2023 · 3 comments · Fixed by #3196
Closed
4 tasks

Handling multiple blobTxs in a block #1910

cmwaters opened this issue Jun 12, 2023 · 3 comments · Fixed by #3196
Assignees
Labels
enhancement New feature or request proposal item is not yet actionable and is suggesting a change that must first be agreed upon WS: BestTxs Reliable and seamless transaction flow

Comments

@cmwaters
Copy link
Contributor

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

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@cmwaters cmwaters added the enhancement New feature or request label Jun 12, 2023
@evan-forbes evan-forbes added the proposal item is not yet actionable and is suggesting a change that must first be agreed upon label Jun 12, 2023
@evan-forbes evan-forbes added this to the Post-mainnet milestone Jun 12, 2023
@cmwaters
Copy link
Contributor Author

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.

@cmwaters cmwaters mentioned this issue Jun 15, 2023
21 tasks
@nashqueue
Copy link
Member

What are our current options for handling multiple PFBs per Block? What is the Limit for how many blobs fit into a PFB ?

@cmwaters
Copy link
Contributor Author

cmwaters commented Jun 22, 2023

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)

What are our current options for handling multiple PFBs per Block?

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)

@evan-forbes evan-forbes added the WS: BestTxs Reliable and seamless transaction flow label Aug 23, 2023
@evan-forbes evan-forbes removed this from the Post-mainnet milestone Nov 12, 2023
@cmwaters cmwaters self-assigned this Mar 20, 2024
cmwaters added a commit that referenced this issue Mar 26, 2024
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
mergify bot pushed a commit that referenced this issue Mar 26, 2024
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
ninabarbakadze pushed a commit to ninabarbakadze/celestia-app that referenced this issue Apr 2, 2024
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
cmwaters added a commit that referenced this issue Apr 4, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request proposal item is not yet actionable and is suggesting a change that must first be agreed upon WS: BestTxs Reliable and seamless transaction flow
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants