From 4874550359e30b77ffed1635553533926530c51b Mon Sep 17 00:00:00 2001 From: Nina Barbakadze Date: Tue, 8 Oct 2024 16:36:39 -0400 Subject: [PATCH 1/2] docs: add ADR for nonce aware mempool --- .../adr-010-rbg-mempool.md | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 docs/celestia-architecture/adr-010-rbg-mempool.md diff --git a/docs/celestia-architecture/adr-010-rbg-mempool.md b/docs/celestia-architecture/adr-010-rbg-mempool.md new file mode 100644 index 0000000000..7ef4d3bcb3 --- /dev/null +++ b/docs/celestia-architecture/adr-010-rbg-mempool.md @@ -0,0 +1,77 @@ +# ADR 010: RBF mempool + +## Changelog + +- 2024-08.10.24: Initial draft (@ninabarbakadze) + +## Context + +A key aspect of user experience for any blockchain system, including Celestia, is the smooth and reliable transaction submission. One of the critical components in achieving this is a mempool that not only handles high tx volume gracefully but also manages transaction ordering effectively to minimize unnecessary rejections. The current mempool implementations priorit and cat prioritize transactions solely based on fees without considering nonce order. Lack of nonce awareness in mempools can result in otherwise valid transactions being rejected with nonce errors due to a higher nonce transaction is prioritized while a lower nonce transaction is still pending. + +## Decision + +Extend the current mempool implementation to be nonce aware. + +## Detailed Design + +in order for the mempool to become aware of nonces we'd have to have Transactions partially ordered by both nonce and priority, ensuring high-priority transactions are processed sooner while maintaining nonce order. The internal structure would consist of The internal structure consists of: A priority-ordered skip list for overall transaction sorting +Separate skip lists for each sender, ordered by transaction nonces. + +when a new transaction is added to the mempool ABCI CheckTx method is called where a transaction has to satisfy a number of conditions before it's accepted in node's mempool. CheckTx also returns a number of fields like the sender and priority. We'd need to extend ABCI's `ResponseCheckTx` to include the nonce. + +`AddTransaction` deals with the `CheckTx` response. Here we could already start indexing transactions by their sender and nonce. Essentially this would be a map on `TxMempool` struct. + +All transactions are ordered by priority in `allEntriesSorted`. We'd update this funciton to take nonce into account when ordering. + + +> This section does not need to be filled in at the start of the ADR, but must be completed prior to the merging of the implementation. +> +> Here are some common questions that get answered as part of the detailed design: +> +> - What are the user requirements? +> +> - What systems will be affected? +> +> - What new data structures are needed, what data structures will be changed? +> +> - What new APIs will be needed, what APIs will be changed? +> +> - What are the efficiency considerations (time/space)? +> +> - What are the expected access patterns (load/throughput)? +> +> - Are there any logging, monitoring or observability needs? +> +> - Are there any security considerations? +> +> - Are there any privacy considerations? +> +> - How will the changes be tested? +> +> - If the change is large, how will the changes be broken up for ease of review? +> +> - Will these changes require a breaking (major) release? +> +> - Does this change require coordination with the Celestia fork of the SDK or celestia-app? + +## Status + +Proposed + +## Consequences + +> This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. + +### Positive + +### Negative + +### Neutral + +## References + +> Are there any relevant PR comments, issues that led up to this, or articles referenced for why we made the given design choice? If so link them here! + +TODO: Reference some related issues here. + +- {reference link} From a8634613b690236cb2dc789f3e58b3806e146d7a Mon Sep 17 00:00:00 2001 From: Nina Barbakadze Date: Mon, 14 Oct 2024 16:18:26 -0400 Subject: [PATCH 2/2] docs: implement rbf design --- .../adr-010-rbf-mempool.md | 48 ++++++++++++ .../adr-010-rbg-mempool.md | 77 ------------------- 2 files changed, 48 insertions(+), 77 deletions(-) create mode 100644 docs/celestia-architecture/adr-010-rbf-mempool.md delete mode 100644 docs/celestia-architecture/adr-010-rbg-mempool.md diff --git a/docs/celestia-architecture/adr-010-rbf-mempool.md b/docs/celestia-architecture/adr-010-rbf-mempool.md new file mode 100644 index 0000000000..eee4624473 --- /dev/null +++ b/docs/celestia-architecture/adr-010-rbf-mempool.md @@ -0,0 +1,48 @@ +# ADR 010: RBF mempool + +## Changelog + +- 2024-08.10.24: Initial draft (@ninabarbakadze) + +## Context + +A key aspect of user experience for any blockchain system, including Celestia, is the smooth and reliable transaction submission. One of the critical components in achieving this is a mempool that not only handles high tx volume gracefully but also manages transaction ordering effectively to minimize unnecessary rejections. The current mempool implementations priority and CAT prioritize transactions solely based on fees without considering nonce order. Lack of nonce awareness in mempools can result in otherwise valid transactions being rejected with nonce errors due to a higher nonce transaction being prioritized while a lower nonce transaction is still pending. + +## Decision + +Extend the current mempool implementation to be nonce aware. + +## Detailed Design + +When a new transaction is added to the mempool, the ABCI `CheckTx()` method validates it against a predefined set of conditions in the state machine. To support RBF, we first need to extend the `CheckTx()` return type `ResponseCheckTx` to include the Nonce field alongside the `Sender` and transaction `Priority`. To implement this, we can either extend an existing `AnteHandler` decorator or add a new one that sets both the Sender and Nonce as part of the `CheckTx()` context. After all decorators are executed, `ctx.Nonce()` and `ctx.Sender()` can be set on `ResponseCheckTX` and returned back to the consensus node. + +After a transaction successfully passes the `CheckTx()` validation, it is added to the `txmp.store` via `addNewTransaction()`. Here, it is stored as a `WrappedTx` type, which sets fields like `priority` and `sender` for the transactions in the mempool. By extending the `WrappedTx` type to include the `nonce`, we can start tracking nonces and ensure that they are accounted for during transaction prioritization. Currently, the sender field is not set in the priority mempool, as it prevents a single sender from submitting multiple transactions. However, since we plan on fully switching to the CAT pool, we can bypass this constraint. + +For RBF functionality, when a new transaction is submitted, we will check whether the user has an existing transaction pending with the same nonce. If so, we will compare the priorities of both transactions and the transaction with the lower priority will be discarded, while the higher-priority one will be inserted into the mempool. Additionally, we will ensure that this process reshuffles the global priority of transactions accordingly. + +Currently, all transactions are sorted by priority in the `allEntriesSorted()` function. To support nonce-based prioritization, we will update this function to prioritize nonces when ordering transactions. This way, nonce will take precedence when sorting high-fee transactions. + +These changes should not be breaking for either celestia-app or celestia-core. The necessary changes involve extending `ResponseCheckTX` in core first, then extending the `CheckTx()` response in the state machine to include both the sender and nonce. Once this is complete, we can implement RBF in celestia-core, release the upgrade, and bump celestia-core version in celestia-app. + +Unit tests will be written for both celestia-core and celestia-app to ensure functionality and stability. Additionally, manual testing on internal testnets will be conducted to validate the changes. + +While these updates will result in slightly more disk writing, memory usage and overall efficiency should remain unaffected. + +## Status + +Proposed + +## Consequences + +### Positive + +**Improved Transaction Fee Efficiency:** Users can dynamically adjust fees to prioritize their transactions, ensuring faster processing when needed. + +**Better Nonce Management:** The system enforces proper nonce order, preventing high-fee transactions from being prioritized out of sequence. + +### Negative + +Users can keep increasing the fee and cause excessive transaction resending. + +## References + diff --git a/docs/celestia-architecture/adr-010-rbg-mempool.md b/docs/celestia-architecture/adr-010-rbg-mempool.md deleted file mode 100644 index 7ef4d3bcb3..0000000000 --- a/docs/celestia-architecture/adr-010-rbg-mempool.md +++ /dev/null @@ -1,77 +0,0 @@ -# ADR 010: RBF mempool - -## Changelog - -- 2024-08.10.24: Initial draft (@ninabarbakadze) - -## Context - -A key aspect of user experience for any blockchain system, including Celestia, is the smooth and reliable transaction submission. One of the critical components in achieving this is a mempool that not only handles high tx volume gracefully but also manages transaction ordering effectively to minimize unnecessary rejections. The current mempool implementations priorit and cat prioritize transactions solely based on fees without considering nonce order. Lack of nonce awareness in mempools can result in otherwise valid transactions being rejected with nonce errors due to a higher nonce transaction is prioritized while a lower nonce transaction is still pending. - -## Decision - -Extend the current mempool implementation to be nonce aware. - -## Detailed Design - -in order for the mempool to become aware of nonces we'd have to have Transactions partially ordered by both nonce and priority, ensuring high-priority transactions are processed sooner while maintaining nonce order. The internal structure would consist of The internal structure consists of: A priority-ordered skip list for overall transaction sorting -Separate skip lists for each sender, ordered by transaction nonces. - -when a new transaction is added to the mempool ABCI CheckTx method is called where a transaction has to satisfy a number of conditions before it's accepted in node's mempool. CheckTx also returns a number of fields like the sender and priority. We'd need to extend ABCI's `ResponseCheckTx` to include the nonce. - -`AddTransaction` deals with the `CheckTx` response. Here we could already start indexing transactions by their sender and nonce. Essentially this would be a map on `TxMempool` struct. - -All transactions are ordered by priority in `allEntriesSorted`. We'd update this funciton to take nonce into account when ordering. - - -> This section does not need to be filled in at the start of the ADR, but must be completed prior to the merging of the implementation. -> -> Here are some common questions that get answered as part of the detailed design: -> -> - What are the user requirements? -> -> - What systems will be affected? -> -> - What new data structures are needed, what data structures will be changed? -> -> - What new APIs will be needed, what APIs will be changed? -> -> - What are the efficiency considerations (time/space)? -> -> - What are the expected access patterns (load/throughput)? -> -> - Are there any logging, monitoring or observability needs? -> -> - Are there any security considerations? -> -> - Are there any privacy considerations? -> -> - How will the changes be tested? -> -> - If the change is large, how will the changes be broken up for ease of review? -> -> - Will these changes require a breaking (major) release? -> -> - Does this change require coordination with the Celestia fork of the SDK or celestia-app? - -## Status - -Proposed - -## Consequences - -> This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. - -### Positive - -### Negative - -### Neutral - -## References - -> Are there any relevant PR comments, issues that led up to this, or articles referenced for why we made the given design choice? If so link them here! - -TODO: Reference some related issues here. - -- {reference link}