Skip to content

fix(sequencer)!: rewrite check_tx to be more efficient and fix regression #1515

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.

Sign up for GitHub

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

Merged
lobstergrindset merged 17 commits into from
Oct 4, 2024
Merged

fix(sequencer)!: rewrite check_tx to be more efficient and fix regression #1515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9dd9628
rebase work on main
lobstergrindset Sep 26, 2024
83e4718
respond to comments from @noot
lobstergrindset Sep 26, 2024
af24538
respond to comment from @Fraser999
lobstergrindset Sep 28, 2024
ddffdd8
update error logs and rename alert
lobstergrindset Oct 1, 2024
2f3a9c8
Merge branch 'main' into lilyjjo/check-tx-rewrite
lobstergrindset Oct 1, 2024
8812a4e
remove non existant import
lobstergrindset Oct 1, 2024
cc3abb8
fix tests
lobstergrindset Oct 1, 2024
98df89c
update metric name
lobstergrindset Oct 1, 2024
d392c32
Merge branch 'main' into lilyjjo/check-tx-rewrite
lobstergrindset Oct 1, 2024
75708ac
Merge remote-tracking branch 'upstream/main' into lilyjjo/check-tx-re…
Fraser999 Oct 1, 2024
c6f087f
Merge branch 'main' into lilyjjo/check-tx-rewrite
lobstergrindset Oct 1, 2024
2670373
Merge branch 'main' into lilyjjo/check-tx-rewrite
lobstergrindset Oct 2, 2024
021faa2
add comment
lobstergrindset Oct 2, 2024
890acfb
fix error message
lobstergrindset Oct 2, 2024
4501fe4
respond to comments by @SuperFluffy
lobstergrindset Oct 4, 2024
ac66880
make contained_tx code consistent and improve test clarity
lobstergrindset Oct 4, 2024
8d1585d
Merge branch 'main' into lilyjjo/check-tx-rewrite
lobstergrindset Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions crates/astria-core/src/protocol/abci.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ impl AbciErrorCode {
pub const TRANSACTION_INSERTION_FAILED: Self = Self(unsafe { NonZeroU32::new_unchecked(11) });
pub const LOWER_NONCE_INVALIDATED: Self = Self(unsafe { NonZeroU32::new_unchecked(12) });
pub const BAD_REQUEST: Self = Self(unsafe { NonZeroU32::new_unchecked(13) });
pub const ALREADY_PRESENT: Self = Self(unsafe { NonZeroU32::new_unchecked(14) });
pub const NONCE_TAKEN: Self = Self(unsafe { NonZeroU32::new_unchecked(15) });
pub const ACCOUNT_SIZE_LIMIT: Self = Self(unsafe { NonZeroU32::new_unchecked(16) });
}

impl AbciErrorCode {
Expand Down Expand Up @@ -52,6 +55,13 @@ impl AbciErrorCode {
}
Self::LOWER_NONCE_INVALIDATED => "lower nonce was invalidated in mempool".into(),
Self::BAD_REQUEST => "the request payload was malformed".into(),
Self::ALREADY_PRESENT => "the transaction is already present in the mempool".into(),
Self::NONCE_TAKEN => "there is already a transaction with the same nonce for the \
Copy link
Copy Markdown
Contributor

@SuperFluffy SuperFluffy Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if NONCE_TAKEN and LOWER_NONCE_INVALIDATED would have any repercussions on composer? Right now composer acts on INVALID_NONCE, refetching the latest nonce and then resubmitting at the latest nonce + 1.

Copy link
Copy Markdown
Contributor Author

@lobstergrindset lobstergrindset Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The NONCE_TAKEN error case would case the bundle factory to not resend when it should. The ALREADY_PRESENT one isn't an error, it means that the bundle is already inside of the mempool. I should change those to reflect these changes probably

Copy link
Copy Markdown
Contributor Author

@lobstergrindset lobstergrindset Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked issue: #1633

account in the mempool"
.into(),
Self::ACCOUNT_SIZE_LIMIT => {
"the account has reached the maximum number of parked transactions".into()
}
Self(other) => {
format!("invalid error code {other}: should be unreachable (this is a bug)")
}
Expand Down
2 changes: 1 addition & 1 deletion crates/astria-sequencer/src/app/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,8 @@ pub(crate) async fn initialize_app_with_storage(
.await
.expect("failed to create temp storage backing chain state");
let snapshot = storage.latest_snapshot();
let mempool = Mempool::new();
let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap()));
let mempool = Mempool::new(metrics);
let mut app = App::new(snapshot, mempool, metrics).await.unwrap();

let genesis_state = genesis_state.unwrap_or_else(self::genesis_state);
Expand Down
10 changes: 7 additions & 3 deletions crates/astria-sequencer/src/grpc/sequencer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ mod tests {
sequencerblock::v1alpha1::SequencerBlock,
};
use cnidarium::StateDelta;
use telemetry::Metrics;

use super::*;
use crate::{
Expand Down Expand Up @@ -256,7 +257,8 @@ mod tests {
async fn test_get_sequencer_block() {
let block = make_test_sequencer_block(1);
let storage = cnidarium::TempStorage::new().await.unwrap();
let mempool = Mempool::new();
let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap()));
let mempool = Mempool::new(metrics);
let mut state_tx = StateDelta::new(storage.latest_snapshot());
state_tx.put_block_height(1).unwrap();
state_tx.put_sequencer_block(block).unwrap();
Expand All @@ -274,7 +276,8 @@ mod tests {
#[tokio::test]
async fn get_pending_nonce_in_mempool() {
let storage = cnidarium::TempStorage::new().await.unwrap();
let mempool = Mempool::new();
let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap()));
let mempool = Mempool::new(metrics);

let alice = get_alice_signing_key();
let alice_address = astria_address(&alice.address_bytes());
Expand Down Expand Up @@ -324,7 +327,8 @@ mod tests {
use crate::accounts::StateWriteExt as _;

let storage = cnidarium::TempStorage::new().await.unwrap();
let mempool = Mempool::new();
let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap()));
let mempool = Mempool::new(metrics);
let mut state_tx = StateDelta::new(storage.latest_snapshot());
let alice = get_alice_signing_key();
let alice_address = astria_address(&alice.address_bytes());
Expand Down
4 changes: 3 additions & 1 deletion crates/astria-sequencer/src/mempool/benchmarks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use sha2::{
Digest as _,
Sha256,
};
use telemetry::Metrics;

use crate::{
app::test_utils::{
Expand Down Expand Up @@ -103,7 +104,8 @@ fn init_mempool<T: MempoolSize>() -> Mempool {
.enable_all()
.build()
.unwrap();
let mempool = Mempool::new();
let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap()));
let mempool = Mempool::new(metrics);
let account_mock_balance = mock_balances(0, 0);
let tx_mock_cost = mock_tx_cost(0, 0, 0);
runtime.block_on(async {
Expand Down
Loading