feat(sequencer): implement `get_pending_nonce` for sequencer API by noot · Pull Request #1073 · astriaorg/astria

noot · 2024-05-14T21:41:36Z

Summary

implement get_pending_nonce for sequencer API, which returns the highest nonce in the mempool (if the address has txs in the mempool), otherwise just returns the nonce in the latest state.

Background

useful for tooling and UX

Changes

implement get_pending_nonce for sequencer API
implement pending_nonce in the mempool, which returns the highest nonce it has for that address if it exists

Testing

unit tests

Related Issues

closes #1070

Fraser999

No blockers - just one point about a potential performance issue.

Fraser999 · 2024-05-20T14:58:17Z

crates/astria-sequencer/src/mempool.rs

+        let inner = self.inner.read().await;
+        let mut nonce = None;
+        for (tx, priority) in inner.iter() {
+            let sender = Address::from_verification_key(tx.verification_key());


I think this could be overly costly in terms of execution/cycles, but I was hoping to memoize the address inside a newtype wrapper for VerificationKey as part of #1079, and that should help.

SuperFluffy

A few comments I would like to see addressed prior to approval.

SuperFluffy · 2024-05-21T16:15:14Z

crates/astria-sequencer/src/grpc/sequencer.rs

+        };
+
+        let address = Address::try_from_raw(&address)
+            .map_err(|e| Status::invalid_argument(format!("invalid address: {e}")))?;


Please emit an event here, logging the entire error source chain.

the error returned from Address::try_from_raw is IncorrectAddressLength so that's the only possible error, can add a trace log

It's the only error at this point. This might or might not change in the future. Assume you have no knowledge about the error.

SuperFluffy · 2024-05-21T16:15:31Z

crates/astria-sequencer/src/grpc/sequencer.rs

+
+        // nonce wasn't in mempool, so just look it up from storage
+        let snapshot = self.storage.latest_snapshot();
+        let nonce = snapshot.get_account_nonce(address).await.map_err(|e| {


Event logging the error source chain.

SuperFluffy · 2024-05-21T16:17:31Z

proto/sequencerblockapis/astria/sequencerblock/v1alpha1/service.proto

+  astria.primitive.v1.Address address = 1 [(google.api.field_behavior) = REQUIRED];
+}
+
+message PendingNonce {


Suggested change

message PendingNonce {

message GetPendingNonceResponse

SuperFluffy · 2024-05-21T16:19:03Z

crates/astria-sequencer/src/grpc/sequencer.rs

+
+        let request = request.into_inner();
+        let Some(address) = request.address else {
+            return Err(Status::invalid_argument(


Add an event

SuperFluffy · 2024-05-21T16:20:29Z

crates/astria-sequencer/src/mempool.rs

    /// required so that `BasicMempool::iter_mut()` can be called.
-    pub(crate) async fn inner(&self) -> tokio::sync::MutexGuard<'_, BasicMempool> {
-        self.inner.lock().await
+    pub(crate) async fn inner(&self) -> tokio::sync::RwLockWriteGuard<'_, BasicMempool> {


Suggested change

pub(crate) async fn inner(&self) -> tokio::sync::RwLockWriteGuard<'_, BasicMempool> {

pub(crate) async fn write(&self) -> tokio::sync::RwLockWriteGuard<'_, BasicMempool> {

SuperFluffy · 2024-05-21T16:21:45Z

crates/astria-sequencer/src/mempool.rs

+
+    /// returns the pending nonce for the given address,
+    /// if it exists in the mempool.
+    pub(crate) async fn pending_nonce(&self, address: &Address) -> Option<u32> {


insturment this method, injecting the address as a span attribute

…nonce

… noot/pending-nonce

SuperFluffy

Please change the span events to levels that we will actually observe during normal operation. It is very interesting to know why a particular request was rejected. As it stands, we will never know that.

SuperFluffy · 2024-05-22T16:13:51Z

crates/astria-sequencer/src/grpc/sequencer.rs

+
+        let request = request.into_inner();
+        let Some(address) = request.address else {
+            trace!("required field address was not set",);


info! level. We want to know what happened when a request is rejected.

that seems really high considering these endpoints could potentially be hit a lot, was concerned about spamming the logs, can change tho

Don't be. The job of a service is to give a good idea about what's going on during its execution. Obviously within reason - you don't want to emit events in a hot loop.

It's the job of the observability platform to make sense of the traces/events that it collects by either dropping them or displaying them such that they are easy to understand.

In this case here I am not at all concerned with these logs.

okay, updated!

SuperFluffy · 2024-05-22T16:14:08Z

crates/astria-sequencer/src/grpc/sequencer.rs

+        };
+
+        let address = Address::try_from_raw(&address).map_err(|e| {
+            trace!(


Suggested change

trace!(

info!(

SuperFluffy · 2024-05-22T16:14:54Z

crates/astria-sequencer/src/grpc/sequencer.rs

+        // nonce wasn't in mempool, so just look it up from storage
+        let snapshot = self.storage.latest_snapshot();
+        let nonce = snapshot.get_account_nonce(address).await.map_err(|e| {
+            trace!(


This strikes me as a critical error that we definitely want to notify on.

Suggested change

trace!(

error!(

…nonce

## Summary The mempool was refactored to reduce the visibility, count and scope of acquired lock guards, and to be implemented in terms of a single collection rather than two which needed synchronized. ## Background The mempool has been implemented to be functionally correct, but can benefit from a refactor to improve safety, performance and complexity (by avoiding synchronizing two collections). **NOTE**: This PR is based on top of #1073. Only the final two commits comprise the new work in this PR. ## Changes - Having reduced the `BasicMempool` to only holding a single collection, there seemed little point to retaining it, so the `Mempool` now just holds the naked priority queue. - Changed from using a `DoublePriorityQueue` to a `PriorityQueue` since we don't need the extra functionality, and the latter is slightly more performant. - All mempool methods now take `&self` rather than `&mut self`. - The `RwLock` is no longer locked and unlocked in a tight loop in the `insert_all` and `remove_all` methods - `TransactionPriority` has been updated to only hold the nonce diff. There was a subtle bug in the previous impl since the derived `PartialEq` and `Eq` didn't correspond to the hand-rolled impls of `PartialOrd` and `Ord`. This has been fixed and further tests added. - `SignedTransaction` is internally held in an `Arc` now and passed in an `Arc` when executing by the app to reduce the number of clones. We could potentially look to extend this across more of the sequencer code for the same reason (not in this PR) - In [the final commit](fd4402b), I changed how `update_mempool_after_finalization` was implemented, so that the mempool no longer has to expose a lock guard. I think it's generally much safer to not expose the lock or a guard on the lock in a non-private API. As well as moving the implementation inside mempool, I added a temporary in-mem cache of current account nonces, since it seems that there could be many txs all under the same account. As already noted, this method could be problematic due to holding the mempool lock for an extended duration, so this might reduce the risk. ## Testing Unit tests extended/added. ## Related Issues Closes #1083. --------- Co-authored-by: elizabeth <elizabethjbinks@gmail.com> Co-authored-by: noot <36753753+noot@users.noreply.github.com>

noot added 2 commits May 14, 2024 17:36

implement get_pending_nonce for sequencer api and in mempool

85f24df

fix tests

e540615

noot requested review from a team as code owners May 14, 2024 21:41

noot requested review from Fraser999 and SuperFluffy May 14, 2024 21:41

github-actions bot added conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate labels May 14, 2024

noot temporarily deployed to BUF May 14, 2024 21:41 — with GitHub Actions Inactive

use RwLock instead of Mutex for mempool

7965fca

noot temporarily deployed to BUF May 15, 2024 20:56 — with GitHub Actions Inactive

Fraser999 approved these changes May 20, 2024

View reviewed changes

Merge branch 'main' into noot/pending-nonce

bd39672

noot enabled auto-merge May 20, 2024 15:07

noot had a problem deploying to BUF May 20, 2024 15:07 — with GitHub Actions Failure

noot had a problem deploying to BUF May 20, 2024 15:41 — with GitHub Actions Failure

noot temporarily deployed to BUF May 20, 2024 15:42 — with GitHub Actions Inactive

Fraser999 mentioned this pull request May 21, 2024

chore(sequencer): refactor mempool #1092

Merged

SuperFluffy reviewed May 21, 2024

View reviewed changes

noot added 2 commits May 21, 2024 13:28

Merge branch 'main' of github.com:astriaorg/astria into noot/pending-…

7c73525

…nonce

address comments, add trace logs

fc365c2

noot requested a review from SuperFluffy May 21, 2024 18:24

Merge branch 'noot/pending-nonce' of github.com:astriaorg/astria into…

41b8676

… noot/pending-nonce

noot temporarily deployed to BUF May 21, 2024 18:25 — with GitHub Actions Inactive

SuperFluffy reviewed May 22, 2024

View reviewed changes

noot added 2 commits May 22, 2024 13:06

update log levels

8d72736

Merge branch 'main' of github.com:astriaorg/astria into noot/pending-…

5c7bfdd

…nonce

noot temporarily deployed to BUF May 22, 2024 17:06 — with GitHub Actions Inactive

SuperFluffy approved these changes May 22, 2024

View reviewed changes

noot added this pull request to the merge queue May 22, 2024

Merged via the queue into main with commit 23c4d9a May 22, 2024

noot deleted the noot/pending-nonce branch May 22, 2024 17:17

	pub(crate) async fn inner(&self) -> tokio::sync::RwLockWriteGuard<'_, BasicMempool> {
	pub(crate) async fn write(&self) -> tokio::sync::RwLockWriteGuard<'_, BasicMempool> {

Conversation

noot commented May 14, 2024

Summary

Background

Changes

Testing

Related Issues

Uh oh!

Fraser999 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SuperFluffy May 22, 2024 •

edited

Loading