feat(sequencer): add transaction status ABCI query #1955

ethanoroshiba · 2025-02-06T21:38:15Z

Summary

Added a transaction status ABCI query to the sequencer and changed the Sequencer Client to poll that instead of CometBFT's tx RPC.

Background

The Sequencer Client's method wait_for_tx_inclusion previously polled CometBFT's tx endpoint, which would return an error no matter whether the transaction was pending, failed, or never submitted in the first place. Because of this lack of distinction, the method would poll indefinitely if a transaction failed for some reason. This is similar to the issue mentioned in #1936.

Changes

Add transaction status ABCI query to the sequencer.
Moved logic of mempool into new MempoolInner, removing the Arc<RwLock<>> around the different caches.
Changed Mempool to be a wrapper for Arc<RwLock<MempoolInner>> so that we don't end up with lock-step issues where, for instance, a transaction status call might fail in between when the transaction is removed from Pending and placed in RemovalCache.
Added get_transaction_status method to mempool, returning either Pending, Parked, RemovalCache, or Unknown.
Added protos and native types for TransactionStatus and TransactionStatusResponse.
Renamed wait_for_tx_inclusion to confirm_tx_inclusion, and changed the logic to continue looping after receiving a Pending or Parked response. If Unknown, it will poll until MAX_WAIT_TIME_UNKNOWN has elapsed, and will then return an error. If it receives RemovalCache, it will try CometBFT's tx RPC for a short time before erroring if there is no response.
Changed reliant bits in the Bridge Withdrawer and Sequencer CLI to use the updated method.

Testing

Added unit tests for mempool's get_transaction_status method.
Added unit tests for Sequencer transaction status ABCI query.
Added unit tests for Sequencer Client's confirm_tx_inclusion method.

Changelogs

Changelod updated

Breaking Changelist

Not breaking, but worth noting this adds to sequencer's public API.

Related Issues

closes #1949

…initely

ethanoroshiba · 2025-02-07T15:01:14Z

crates/astria-sequencer/CHANGELOG.md

+- Change sequencer client to poll transaction status ABCI query so that it doesn't
+loop indefinitely [#1955](https://github.com/astriaorg/astria/pull/1955).


Unsure if this is the correct place for this? There's no changelog for Sequencer Client.

ethanoroshiba · 2025-02-07T15:14:27Z

proto/protocolapis/astria/protocol/transaction/v1/types.proto

+enum TransactionStatus {
+  TRANSACTION_STATUS_UNSPECIFIED = 0;
+  TRANSACTION_STATUS_PARKED = 1;
+  TRANSACTION_STATUS_PENDING = 2;
+  TRANSACTION_STATUS_REMOVAL_CACHE = 3;
+  TRANSACTION_STATUS_UNKNOWN = 4;
+}


I'm not sure whether this is useful. It's represented as an i32 in Rust anyways, and the domain TransactionStatus conversion is done directly from i32. Could be nice for the API, though.

crates/astria-core/src/protocol/transaction/v1/mod.rs

crates/astria-sequencer-client/src/extension_trait.rs

noot · 2025-02-11T18:24:38Z

crates/astria-sequencer/src/mempool/mod.rs

+            RemovalReason::NonceStale => write!(f, "transaction nonce is lower than current nonce"),
+            RemovalReason::LowerNonceInvalidated => write!(
+                f,
+                "previous transaction was not executed, to this transaction's nonce has become \


Suggested change

"previous transaction was not executed, to this transaction's nonce has become \

"previous transaction was not executed, so this transaction's nonce has become \

Done in cdb152e. Thanks for reviewing!

Looks like this got missed?

Still not handled by the looks of it :D

noot

looks good, only have a few minor suggestions!

Fraser999

As discussed offline, I think we need to make the locking in the mempool more coarse-grained in order to allow the new query to give reliable results. Basically, the four collections comprising the mempool need to be updated atomically.

crates/astria-sequencer/src/mempool/transactions_container.rs

crates/astria-sequencer/CHANGELOG.md

crates/astria-sequencer/src/mempool/mod.rs

Fraser999 · 2025-03-18T14:28:29Z

crates/astria-sequencer/src/mempool/mod.rs

+            RemovalReason::NonceStale => write!(f, "transaction nonce is lower than current nonce"),
+            RemovalReason::LowerNonceInvalidated => write!(
+                f,
+                "previous transaction was not executed, to this transaction's nonce has become \


Looks like this got missed?

crates/astria-sequencer/src/mempool/mod.rs

crates/astria-sequencer/src/mempool/query.rs

Fraser999 · 2025-03-18T15:39:05Z

crates/astria-sequencer/src/mempool/query.rs

+    let height = match storage.latest_snapshot().get_block_height().await {
+        Ok(height) => height,
+        Err(err) => {
+            return response::Query {
+                code: Code::Err(AbciErrorCode::INTERNAL_ERROR.value()),
+                info: "failed getting block height".into(),
+                log: format!("{err:?}"),
+                ..response::Query::default()
+            };
+        }
+    };
+
+    let height = match height.try_into() {
+        Ok(height) => height,
+        Err(err) => {
+            return response::Query {
+                code: Code::Err(AbciErrorCode::INTERNAL_ERROR.value()),
+                info: "failed converting `u64` block height to type `tendermint::block::Height`"
+                    .into(),
+                log: format!("{err:?}"),
+                ..response::Query::default()
+            };
+        }
+    };


I wonder if we need to provide this or if we can just use height 0 or 1 (the default).

I'm pretty non-opinionated on this, but I lean slightly the opposite way just because of the fact that if the height is a part of the message, we should be providing the accurate height for which the response was for, even if unnecessary or unrelated. This might be the wrong mindset, though, so happy to change if you think we should

Co-authored-by: Fraser Hutchison <[email protected]>

ethanoroshiba · 2025-03-20T16:56:35Z

After offline discussion, the decision was made to close this in favor of a timeout approach in #2048. This is because we firstly want to move towards gRPC instead of ABCI queries, and secondly since this approach will pretty much only return correct transaction statuses when querying a validator node.

add tx status ABCI query, prevent sequencer client from looping indef…

21fa040

…initely

github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels Feb 6, 2025

ethanoroshiba temporarily deployed to BUF February 6, 2025 21:38 — with GitHub Actions Inactive

ethanoroshiba added bug Something isn't working sequencer-client labels Feb 6, 2025

ethanoroshiba changed the title ~~fix(sequencer-cli): poll TX status to avoid looping indefinitely~~ fix(sequencer-client): poll TX status to avoid looping indefinitely Feb 6, 2025

fix proto field, update changelog

46063b0

ethanoroshiba temporarily deployed to BUF February 6, 2025 21:50 — with GitHub Actions Inactive

ethanoroshiba changed the title ~~fix(sequencer-client): poll TX status to avoid looping indefinitely~~ fix(sequencer): add transaction status ABCI query Feb 7, 2025

ethanoroshiba changed the title ~~fix(sequencer): add transaction status ABCI query~~ feat(sequencer): add transaction status ABCI query Feb 7, 2025

ethanoroshiba removed the bug Something isn't working label Feb 7, 2025

ethanoroshiba commented Feb 7, 2025

View reviewed changes

use info instead of log, some minor code cleanup

da94795

ethanoroshiba temporarily deployed to BUF February 7, 2025 15:17 — with GitHub Actions Inactive

ethanoroshiba marked this pull request as ready for review February 7, 2025 15:34

ethanoroshiba requested review from a team, SuperFluffy, Fraser999, noot and joroshiba as code owners February 7, 2025 15:34

noot reviewed Feb 11, 2025

View reviewed changes

crates/astria-core/src/protocol/transaction/v1/mod.rs Outdated Show resolved Hide resolved

noot reviewed Feb 11, 2025

View reviewed changes

crates/astria-sequencer-client/src/extension_trait.rs Outdated Show resolved Hide resolved

noot reviewed Feb 11, 2025

View reviewed changes

noot approved these changes Feb 11, 2025

View reviewed changes

ethanoroshiba added 2 commits February 12, 2025 11:47

requested changes

cdb152e

Merge branch 'main' into eoroshiba/fix-wait-for-tx-inclusion

9198a78

ethanoroshiba temporarily deployed to BUF February 12, 2025 17:49 — with GitHub Actions Inactive

Merge branch 'main' into eoroshiba/fix-wait-for-tx-inclusion

b352f8e

ethanoroshiba added 2 commits March 17, 2025 10:09

fixes from merge

440913e

update logging logic to match #1940

582324a

ethanoroshiba temporarily deployed to BUF March 17, 2025 15:49 — with GitHub Actions Inactive

fmt md

0cedd6e

ethanoroshiba temporarily deployed to BUF March 17, 2025 16:39 — with GitHub Actions Inactive

ethanoroshiba added 2 commits March 17, 2025 11:44

move changes to correct changelog section

52e699e

Merge branch 'main' into eoroshiba/fix-wait-for-tx-inclusion

69add10

ethanoroshiba temporarily deployed to BUF March 17, 2025 17:25 — with GitHub Actions Inactive

Fraser999 requested changes Mar 18, 2025

View reviewed changes

ethanoroshiba and others added 3 commits March 19, 2025 10:56

Apply suggestions from code review

882d115

Co-authored-by: Fraser Hutchison <[email protected]>

requested changes

70ba764

Merge branch 'main' into eoroshiba/fix-wait-for-tx-inclusion

a767d5b

ethanoroshiba requested a review from Fraser999 March 19, 2025 17:20

ethanoroshiba temporarily deployed to BUF March 19, 2025 17:20 — with GitHub Actions Inactive

ethanoroshiba closed this Mar 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sequencer): add transaction status ABCI query #1955

feat(sequencer): add transaction status ABCI query #1955

ethanoroshiba commented Feb 6, 2025 •

edited

Loading

ethanoroshiba Feb 7, 2025

ethanoroshiba Feb 7, 2025

noot Feb 11, 2025

ethanoroshiba Feb 12, 2025

Fraser999 Mar 18, 2025

Fraser999 Mar 19, 2025

noot left a comment

Fraser999 left a comment

Fraser999 Mar 18, 2025

Fraser999 Mar 18, 2025

ethanoroshiba Mar 19, 2025

ethanoroshiba commented Mar 20, 2025

		- Change sequencer client to poll transaction status ABCI query so that it doesn't
		loop indefinitely [#1955](https://github.com/astriaorg/astria/pull/1955).

	"previous transaction was not executed, to this transaction's nonce has become \
	"previous transaction was not executed, so this transaction's nonce has become \

feat(sequencer): add transaction status ABCI query #1955

feat(sequencer): add transaction status ABCI query #1955

Conversation

ethanoroshiba commented Feb 6, 2025 • edited Loading

Summary

Background

Changes

Testing

Changelogs

Breaking Changelist

Related Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot left a comment

Choose a reason for hiding this comment

Fraser999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanoroshiba commented Mar 20, 2025

ethanoroshiba commented Feb 6, 2025 •

edited

Loading