Nothing Special   »   [go: up one dir, main page]

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

[tm2] Improved Mempool Logic #1830

Open
zivkovicmilos opened this issue Mar 25, 2024 · 7 comments
Open

[tm2] Improved Mempool Logic #1830

zivkovicmilos opened this issue Mar 25, 2024 · 7 comments
Assignees
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related

Comments

@zivkovicmilos
Copy link
Member

Description

This effort covers expanding the capabilities of the Mempool module to be more in line with how stable blockchains should work.

The major problems currently present in the Mempool implementation of Gno are:

Transactions are unordered by account sequence

The current implementation of the mempool module does not take into account the account sequence ordering when returning “ready” transactions for block building.

The mempool currently returns transactions only ordered by their gas, and not by account sequence first, and gas second.

This leads to a scenario where the mempool can return transactions for block building that are not in the correct order of sending for an account. For example:

  1. TX 1 sends a balance to account X, initializing it in the process (the account was uninitialized)
  2. TX 2, by account X, deploys a Realm
  3. These 2 transactions are prepared offline, and sent to the node for processing
  4. The mempool is asked to return ready transactions in the block building process of the node, returns [TX2, TX1], since they are ordered by gas
  5. TX 2 fails because the account is uninitialized
  6. TX 1 succeeds
  7. The user now has to send TX2 again, in order to get the desired state

Realm deploys cost more gas, so TX2 will be executed before TX1, since the mempool returns this transaction first (orders by gas). TX 2 will fail, because the account is uninitialized. Account balance transfers cost less gas, so they will be executed first in the mempool implementation.

Block building does not take into account actual gas used by a TX

The current implementation of the block building process does not take into account the actual gas used by a transaction when trying to figure out if it can fit into a block. Instead, the block building process simply reads the (user defined) gas limit, and sees if the block gas enough space to fit the transaction, even though the transaction actually uses less gas than what was specified.

The optimal flow for the block building process would be:

  • get ready transactions from the mempool
  • execute each transaction to see how much gas it would utilize (and whether the tx is valid at all)
    • if the tx is valid, but cannot fit into the block, return it to the mempool

This way, the mempool always has ready transactions that are executable, given the previous point about transaction ordering.

If this is not patched, there is a DDoS vector where a user can spam valid high gas limit transactions and take up all the block space (ex. with a single transaction), leaving no room for the transactions of other users.

Successful outcome of this effort:

  • transactions are ordered in the mempool by [account sequence, gas]
  • block generation takes into account the actual gas used, not the user defined gas used when deciding if a transaction can fit into a block
@ltzmaxwell
Copy link
Contributor

in this case, the first transaction would be processed priorly because it has a lower account sequence, even it costs less gas? is this reasonable?

Account_0 send tokens to another account; (less gas cost)
Account_1 deploys realm; (more gas cost)

@zivkovicmilos
Copy link
Member Author
zivkovicmilos commented Oct 25, 2024

in this case, the first transaction would be processed priorly because it has a lower account sequence, even it costs less gas? is this reasonable?

Account_0 send tokens to another account; (less gas cost)
Account_1 deploys realm; (more gas cost)

Yes, transactions should be ordered by 2 sort params:

  • account sequence (nonce), ascending
  • gas price, descending

@thehowl
Copy link
Member
thehowl commented Oct 25, 2024

The optimal flow for the block building process would be:

* get ready transactions from the mempool

* execute each transaction to see how much gas it would utilize (and whether the tx is valid at all)
  
  * if the tx is valid, but cannot fit into the block, return it to the mempool

Is this what's done on cometbft / ethereum?

@zivkovicmilos
Copy link
Member Author

The optimal flow for the block building process would be:

* get ready transactions from the mempool

* execute each transaction to see how much gas it would utilize (and whether the tx is valid at all)
  
  * if the tx is valid, but cannot fit into the block, return it to the mempool

Is this what's done on cometbft / ethereum?

Yes. This is how most mempool implementations work

@zivkovicmilos
Copy link
Member Author

@Kouteki
I'm taking this up after #2852

@zivkovicmilos zivkovicmilos self-assigned this Oct 27, 2024
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Oct 29, 2024
@Kouteki Kouteki removed the in focus Core team is prioritizing this work label Nov 22, 2024
@Kouteki
Copy link
Contributor
Kouteki commented Nov 22, 2024

After researching the issue, we determined:

  • The issue is much more complex than we initially thought
  • It's not a breaking change
  • It exists in Tendermint

That's why I'm moving it to the post-launch milestone.

@zivkovicmilos
Copy link
Member Author

To give clarity, we can't fix these issues without changing the canonical CListMempool implementation heavily.

We're leaving it to be an optimization and effort after the mainnet launch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Development

No branches or pull requests

5 participants