fix(composer)!: update to work with appside mempool #1643

Lilyjjo · 2024-10-08T12:45:23Z

Summary

Updates composer to work with the new appside mempool. New ABCI error codes were added and now nonces can be stacked, so the submission logic should use the GRPC pending_nonce() instead of the RPC get_latest_nonce().

Background

New ABCI error codes were added in PR #1515 which need to be handled in the composer's submission logic.

New Error codes:

NONCE_TAKEN -> try resubmitting with new top-of-pending. If this is happening because the composer is out of funds, that is okay as the composer has it's own fund monitoring logic
PARKED_FULL -> normal error case ok
ACCOUNT_SIZE_LIMIT -> normal error case ok
ALREADY_PRESENT -> normal error case ok, shouldn't be returned

Background

We changed the internals of the mempool which resulted in the users of the mempool to have changes as well.

Testing

Smoke tests

Related Issues

closes #1633

itamarreif

Please also add a black box test for this new error. See existing test for INVALID_NONCE:

https://github.com/astriaorg/astria/blob/main/crates/astria-composer/tests/blackbox/geth_collector.rs#L86

SuperFluffy

The abci error codes are only one part in addressing #1633.

This patch also needs to change how composer fetches the next nonce to use (since right it makes use of cometbft's get_nonce, which is not aware of the mempool).

Lilyjjo · 2024-10-08T15:50:24Z

note: not ready for review yet

itamarreif · 2024-10-09T10:55:38Z

charts/composer/templates/configmap.yaml

@@ -8,7 +8,8 @@ data:
  ASTRIA_COMPOSER_API_LISTEN_ADDR: "0.0.0.0:{{ .Values.ports.healthApi }}"
  ASTRIA_COMPOSER_GRPC_ADDR: "0.0.0.0:{{ .Values.ports.grpc }}"
  ASTRIA_COMPOSER_SEQUENCER_CHAIN_ID: "{{ tpl .Values.config.sequencerChainId . }}"
-  ASTRIA_COMPOSER_SEQUENCER_URL: "{{ tpl .Values.config.sequencerRpc . }}"
+  ASTRIA_COMPOSER_SEQUENCER_HTTP_URL: "{{ tpl .Values.config.sequencerRpc . }}"


we call this COMETBFT_ENDPOINT in other places

updated the name in the code as well

crates/astria-composer/src/executor/mod.rs

itamarreif · 2024-10-09T11:03:58Z

crates/astria-composer/src/executor/mod.rs

    })
    .with_config(retry_config)
    .await
-    .wrap_err("failed getting latest nonce from sequencer after 1024 attempts");
+    .wrap_err("failed getting pending nonce from sequencing after 1024 attempts");


"from sequencer" instead of "from sequencing"?

itamarreif · 2024-10-09T11:08:58Z

crates/astria-composer/tests/blackbox/geth_collector.rs

@@ -127,18 +99,57 @@ async fn invalid_nonce_causes_resubmission_under_different_nonce() {

    // wait for 1 sequencer block time to make sure the bundle is preempted
    tokio::time::timeout(
-        Duration::from_millis(test_composer.cfg.block_time_ms),
+        Duration::from_millis(test_composer.cfg.block_time_ms + 1000),


should probably remove the + 1000 here?

itamarreif · 2024-10-09T12:39:53Z

crates/astria-composer/tests/blackbox/helper/mock_grpc_sequencer.rs

+        }
+    }
+
+    pub(crate) async fn mount_pending_nonce_response(


this should probably return a mock guard

It's fine if it doesn't. Ideally you provide the intermediate fetches (maybe with an expect4ed number of requests that match) and only have a mockguard on the actual output of the service (in this case, a transaction that contains a collection of sequence actions).

If you return a mock guard and then wait_until_satisfied it, you end up in a situation where you test the implementation (in which order are requests made?) rather than its behavior (does it produce the expected result?).

itamarreif · 2024-10-09T12:42:01Z

crates/astria-composer/tests/blackbox/geth_collector.rs

    let expected_rollup_ids = vec![RollupId::from_unhashed_bytes("test1")];
    // Expect nonce 1 again so that the resubmitted tx is accepted
    let valid_nonce_guard =
        mount_broadcast_tx_sync_mock(&test_composer.sequencer, expected_rollup_ids, vec![1]).await;

+    // Mount a response of 1 to a nonce query
+    test_composer


should get a mock guard from the mount helper and then wait for it to be satisfied

Noted elsewhere: ideally we don't worry about the choreography of which RPCs are made in which order. It's best to just provide them and then try and observe what comes out of the service. Makes the tests less fragile.

itamarreif

the pending nonce mock should use a mockguard and wait_until_satisfied as the mount is used several times in the test (e.g. for initializing nonce on startup and then on refetch after the invalid/taken nonce errors). besides that, just a couple nits on the error message and config variable names.

SuperFluffy

I'd like to have a few (user facing) names changed, but otherwise this looks great.

SuperFluffy · 2024-10-11T10:22:19Z

crates/astria-composer/tests/blackbox/helper/mock_grpc_sequencer.rs

+        }
+    }
+
+    pub(crate) async fn mount_pending_nonce_response(


It's fine if it doesn't. Ideally you provide the intermediate fetches (maybe with an expect4ed number of requests that match) and only have a mockguard on the actual output of the service (in this case, a transaction that contains a collection of sequence actions).

If you return a mock guard and then wait_until_satisfied it, you end up in a situation where you test the implementation (in which order are requests made?) rather than its behavior (does it produce the expected result?).

SuperFluffy · 2024-10-11T10:25:05Z

crates/astria-composer/src/executor/tests.rs

@itamarreif Can you please go over these and provide feedback if anything that was tested here is not in some way covered in the blackbox tests?

I agree with removing the tests here (it should have been done a long time ago), rather than continuing to fix them.

SuperFluffy · 2024-10-11T10:31:23Z

crates/astria-composer/local.env.example

@@ -25,7 +25,10 @@ NO_COLOR=
 ASTRIA_COMPOSER_API_LISTEN_ADDR="0.0.0.0:0"

 # Address of the RPC server for the sequencer chain
-ASTRIA_COMPOSER_SEQUENCER_URL="http://127.0.0.1:26657"
+ASTRIA_COMPOSER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"


Suggested change

ASTRIA_COMPOSER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"

ASTRIA_COMPOSER_SEQUENCER_ABCI_ENDPOINT="http://127.0.0.1:26657"

SuperFluffy · 2024-10-11T10:32:04Z

crates/astria-composer/src/config.rs

@@ -27,7 +27,10 @@ pub struct Config {
    pub api_listen_addr: SocketAddr,

    /// Address of the RPC server for the sequencer chain
-    pub sequencer_url: String,
+    pub cometbft_endpoint: String,


As for the comment on the env.example - to be consistent we should call this sequencer_abci_endpoint to distinguish between it and the grpc endpoint.

SuperFluffy · 2024-10-11T10:32:36Z

crates/astria-composer/local.env.example

-ASTRIA_COMPOSER_SEQUENCER_URL="http://127.0.0.1:26657"
+ASTRIA_COMPOSER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"
+
+# Address of the RPC server for the sequencer chain


Suggested change

# Address of the RPC server for the sequencer chain

# Address of the gRPC server for the sequencer chain

crates/astria-composer/src/executor/mod.rs

SuperFluffy · 2024-10-11T10:39:06Z

crates/astria-composer/src/executor/mod.rs

@@ -517,7 +536,7 @@ async fn get_latest_nonce(
    err,
 )]
 async fn submit_tx(
-    client: sequencer_client::HttpClient,
+    cometbft_client: sequencer_client::HttpClient,


Very nit: I think you could just keep this as client. In the context of this function there is no confusion.

But if you want to keep it, I think I'd lean toward calling this abci_client.

SuperFluffy · 2024-10-11T10:48:26Z

crates/astria-composer/tests/blackbox/helper/mock_http_sequencer.rs

Let's call this mock_abci_sequencer.rs

SuperFluffy · 2024-10-11T10:54:08Z

crates/astria-composer/tests/blackbox/helper/mock_grpc_sequencer.rs

+    Status,
+};
+
+const GET_PENDING_NONCE_GRPC_NAME: &str = "get_pending_nonce";


Just a side note: the full path that is actually called as part of the gRPC request is "/astria.sequencerblock.v1alpha1.SequencerService/GetPendingNonce"

SuperFluffy · 2024-10-11T10:56:31Z

crates/astria-composer/tests/blackbox/geth_collector.rs

    let expected_rollup_ids = vec![RollupId::from_unhashed_bytes("test1")];
    // Expect nonce 1 again so that the resubmitted tx is accepted
    let valid_nonce_guard =
        mount_broadcast_tx_sync_mock(&test_composer.sequencer, expected_rollup_ids, vec![1]).await;

+    // Mount a response of 1 to a nonce query
+    test_composer


Noted elsewhere: ideally we don't worry about the choreography of which RPCs are made in which order. It's best to just provide them and then try and observe what comes out of the service. Makes the tests less fragile.

itamarreif

Went over the deleted tests and made an issue for adding the coverage back as black box tests: #1652

itamarreif · 2024-10-11T14:30:07Z

crates/astria-composer/src/executor/tests.rs

-/// Test to check that the executor sends a signed transaction to the sequencer as soon as it
-/// receives a `SequenceAction` that fills it beyond its `max_bundle_size`.
-#[tokio::test]
-async fn full_bundle() {


This test does not have an analogous black box test currently. However, there are unit tests on the BundleFactory that test for full bundles being fired off.

itamarreif · 2024-10-11T14:33:53Z

crates/astria-composer/src/executor/tests.rs

-/// Test to check that the executor sends a signed transaction to the sequencer after its
-/// `block_timer` has ticked
-#[tokio::test]
-async fn bundle_triggered_by_block_timer() {


This test also doesn't have an analog in the black box tests. We've tested this internally to the crate, in the executor module instead as this test assumes quite a bit of knowledge about the internal functions of the composer (specifically that if the bundle isn't filled then it will be submitted after the block timer).

itamarreif · 2024-10-11T14:36:59Z

crates/astria-composer/src/executor/tests.rs

-/// Test to check that executor's chain ID check is properly checked against the sequencer's chain
-/// ID
-#[tokio::test]
-async fn chain_id_mismatch_returns_error() {


This test does not currently exist in the blackbox tests as it requires bubbling errors up out from the composer service.

itamarreif · 2024-10-11T14:59:50Z

crates/astria-composer/src/executor/tests.rs

-/// Test to check that the executor sends a signed transaction with two sequence actions to the
-/// sequencer.
-#[tokio::test]
-async fn two_seq_actions_single_bundle() {


This test also does not have an analog blackbox test currently. A test similar to the existing single_rollup_tx_payload_integrity that has two transactions should be added to cover this.

SuperFluffy

Thanks for the fix!

The tests in the executor module are in a weird state and should be part of the blackbox tests instead. As they are testing relatively low impact functionality as per @itamarreif's notes, I am in favor of removing them here and following up in a future patch.

quasystaty1 · 2024-10-12T08:33:33Z

charts/composer/templates/configmap.yaml

@@ -30,6 +29,8 @@ data:
  OTEL_EXPORTER_OTLP_TRACE_HEADERS: "{{ tpl .Values.otel.traceHeaders . }}"
  OTEL_SERVICE_NAME: "{{ tpl .Values.otel.serviceName . }}"
  {{- if not .Values.global.dev }}
+  ASTRIA_COMPOSER_SEQUENCER_ABCI_ENDPOINT: "{{ tpl .Values.config.sequencerRpc . }}"


when .Values.global.dev sets to true it marks a breaking change to configs. Therefore, new env variables should be added to else bracket while values being removed should be added to {{- if not .Values.global.dev }} bracket.

quasystaty1

Charts LGTM, small nit

charts/composer/templates/configmap.yaml

Co-authored-by: quasystaty <ido@astria.org>

* main: feat(sequencer)!: rework all fees (#1647) fix(sequencer): fix app hash in horcrux sentries (#1646) fix(composer)!: update to work with appside mempool (#1643) fix(charts): rollup application monitoring (#1601)

github-actions bot added the composer pertaining to composer label Oct 8, 2024

Lilyjjo marked this pull request as ready for review October 8, 2024 12:49

Lilyjjo requested a review from a team as a code owner October 8, 2024 12:49

Lilyjjo requested review from SuperFluffy and itamarreif October 8, 2024 12:49

itamarreif requested changes Oct 8, 2024

View reviewed changes

SuperFluffy requested changes Oct 8, 2024

View reviewed changes

Lilyjjo requested a review from a team as a code owner October 8, 2024 15:49

Lilyjjo requested a review from quasystaty1 October 8, 2024 15:49

github-actions bot added the cd label Oct 8, 2024

Lilyjjo changed the title ~~fix(composer): handle new abci error codes~~ fix(composer)!: update to work with appside mempool Oct 8, 2024

Lilyjjo force-pushed the lilyjjo/composer_abci_error_fix branch 2 times, most recently from c0b80ae to f6c5a05 Compare October 9, 2024 10:49

Lilyjjo requested a review from itamarreif October 9, 2024 10:51

Lilyjjo self-assigned this Oct 9, 2024

itamarreif reviewed Oct 9, 2024

View reviewed changes

crates/astria-composer/src/executor/mod.rs Show resolved Hide resolved

itamarreif reviewed Oct 9, 2024

View reviewed changes

Lilyjjo force-pushed the lilyjjo/composer_abci_error_fix branch from bd37417 to f0e7f34 Compare October 9, 2024 13:37

Lilyjjo added 5 commits October 9, 2024 15:39

fix composer

ae142a3

remove ALREADY_PRESENT as it should not be returned

3b69c0b

feat: get tests working and cleanup

39d18b3

add test for nonce taken

368715b

delete instead of fix

cfbf479

Lilyjjo force-pushed the lilyjjo/composer_abci_error_fix branch from f0e7f34 to cfbf479 Compare October 9, 2024 13:39

SuperFluffy reviewed Oct 11, 2024

View reviewed changes

Lilyjjo added 4 commits October 11, 2024 14:45

address comments besides mock expectations

9541015

fix charts

bda2d87

fix charts

09af906

chartssss

f517085

Lilyjjo requested review from quasystaty1 and SuperFluffy October 11, 2024 13:08

itamarreif mentioned this pull request Oct 11, 2024

Add black box tests to composer to cover deleted internal tests #1652

Open

itamarreif reviewed Oct 11, 2024

View reviewed changes

SuperFluffy approved these changes Oct 11, 2024

View reviewed changes

quasystaty1 requested changes Oct 12, 2024

View reviewed changes

Lilyjjo and others added 6 commits October 14, 2024 15:49

fix charts

fe86cae

Merge branch 'main' into lilyjjo/composer_abci_error_fix

9d2b75a

fix charts

8ef17cd

fix charts

1d98d64

fix charts

8f6db63

fix charts

5bdfddc

Lilyjjo force-pushed the lilyjjo/composer_abci_error_fix branch from baac1c3 to 5bdfddc Compare October 14, 2024 14:10

Lilyjjo requested a review from quasystaty1 October 14, 2024 14:18

quasystaty1 approved these changes Oct 14, 2024

View reviewed changes

charts/composer/templates/configmap.yaml Show resolved Hide resolved

Lilyjjo and others added 2 commits October 14, 2024 16:25

Update charts/composer/templates/configmap.yaml

703687d

Co-authored-by: quasystaty <ido@astria.org>

fix comment

119e8ae

Lilyjjo enabled auto-merge October 14, 2024 14:37

Lilyjjo requested a review from itamarreif October 14, 2024 14:44

itamarreif approved these changes Oct 14, 2024

View reviewed changes

Lilyjjo added this pull request to the merge queue Oct 14, 2024

Merged via the queue into main with commit acfd370 Oct 14, 2024
44 checks passed

Lilyjjo deleted the lilyjjo/composer_abci_error_fix branch October 14, 2024 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(composer)!: update to work with appside mempool #1643

fix(composer)!: update to work with appside mempool #1643

Lilyjjo commented Oct 8, 2024 •

edited

Loading

itamarreif left a comment •

edited

Loading

SuperFluffy left a comment

Lilyjjo commented Oct 8, 2024

itamarreif Oct 9, 2024

Lilyjjo Oct 9, 2024

itamarreif Oct 9, 2024

itamarreif Oct 9, 2024

itamarreif Oct 9, 2024

SuperFluffy Oct 11, 2024

itamarreif Oct 9, 2024

SuperFluffy Oct 11, 2024

itamarreif left a comment

SuperFluffy left a comment

SuperFluffy Oct 11, 2024

SuperFluffy Oct 11, 2024

SuperFluffy Oct 11, 2024

SuperFluffy Oct 11, 2024

SuperFluffy Oct 11, 2024

SuperFluffy Oct 11, 2024

SuperFluffy Oct 11, 2024

SuperFluffy Oct 11, 2024

SuperFluffy Oct 11, 2024

itamarreif left a comment

itamarreif Oct 11, 2024

itamarreif Oct 11, 2024

itamarreif Oct 11, 2024

itamarreif Oct 11, 2024

SuperFluffy left a comment

quasystaty1 Oct 12, 2024

quasystaty1 left a comment

	ASTRIA_COMPOSER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"
	ASTRIA_COMPOSER_SEQUENCER_ABCI_ENDPOINT="http://127.0.0.1:26657"

	# Address of the RPC server for the sequencer chain
	# Address of the gRPC server for the sequencer chain

fix(composer)!: update to work with appside mempool #1643

fix(composer)!: update to work with appside mempool #1643

Conversation

Lilyjjo commented Oct 8, 2024 • edited Loading

Summary

Background

Background

Testing

Related Issues

itamarreif left a comment • edited Loading

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

Lilyjjo commented Oct 8, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itamarreif left a comment

Choose a reason for hiding this comment

SuperFluffy 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

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

itamarreif 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

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quasystaty1 left a comment

Choose a reason for hiding this comment

Lilyjjo commented Oct 8, 2024 •

edited

Loading

itamarreif left a comment •

edited

Loading