feat(sequencer)!: make parked mempool nonces replaceable #1763
Conversation
Lilyjjo
changed the title
| @@ -64,6 +65,9 @@ impl AbciErrorCode { | |||
| "the account has reached the maximum number of parked transactions".into() | |||
| } | |||
| Self::PARKED_FULL => "the mempool is out of space for more parked transactions".into(), | |||
| Self::NONCE_REPLACEMENT => { | |||
| "the transaction was replaced by a different transaction with the same nonce".into() | |||
There was a problem hiding this comment.
What will the flow look like to actually see this abci error code?
If I understand the follow correctly:
- transaction is stuck in mempool
- new transaction is submitted to the mempool, replacing the old one
- check-tx for the new transaction should return a success code (that is abci code 0).
But NONCE_REPLACEMENT is an error code for the transaction that got replaced if I understand correctly. What would I need to do to observe this error code?
There was a problem hiding this comment.
The next task I have is for general transaction observability (#1773), we don't currently have the infra to communicate it now to the users but we should
There was a problem hiding this comment.
I don't think one this is strictly related to the other. The AbciErrorCodes are intended to be returned in response to abci requests and put into the code field of the various responses (in this case CheckTx).
All the ABCI error codes can be observed right now (even if clunky). I don't understand when/how this new code would be observed.
| @@ -434,7 +447,7 @@ pub(super) trait TransactionsForAccount: Default { | |||
| ttx: TimemarkedTransaction, | |||
| current_account_nonce: u32, | |||
| current_account_balances: &HashMap<IbcPrefixed, u128>, | |||
| ) -> Result<(), InsertionError> { | |||
| ) -> Result<Option<[u8; 32]>, InsertionError> { | |||
There was a problem hiding this comment.
This has a bad code smell. The behavior of the trait method can no longer be understood from by just looking at the signature, but I also need to understand the implementation of nonce_replacement_enabled to understand its behavior.
Now that the behavior of the two queues is so fundamentally different, they should be represented by two different types.
|
For new ABCI error codes we should be able to observe the error from the outside. We should add a smoke test that performs the nonce replacement and observes the abci code. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or this PR will be |
|
This PR was closed because it has been stale. |
Summary
This PR enables nonce replacement for all transactions inside of the mempool's parked queue.
It was discovered while writing this PR that nonce replacement for the bottom nonce in parked was already enabled if the new nonce was able to enter the pending queue, when this happened the replaced nonce was being cleared out during the mempool's maintenance loop. This PR enables all of parked to have nonce replacement as well as adds the new notion of 'removed due to nonce replacement' to the information propagated up to cometBFT.
Background
People have started to use the internal rust CLI and have had trouble with transactions getting stuck inside of the mempool's parked queue due to incorrect parameters. This PR enables nonce replacement of parked transactions for a better UX. This feature should be disabled if people start trying to spam the mempool with it.
Changes
Testing
Ran locally and wrote unit tests.
Breaking Changes
Added new ABCI error code
NONCE_REPLACEMENT. This is to signal through cometBFT when a transaction has been removed from the mempool due to being replaced.Metrics
Added counter
MEMPOOL_NONCE_REPLACEMENTto track how many times users have used nonce replacement.Changelogs
Changelogs updated.