feat(bridge-withdrawer): PoC astria-bridge-withdrawer implementation by itamarreif · Pull Request #984 · astriaorg/astria

itamarreif · 2024-04-19T19:59:58Z

Summary

implement ethereum and submitter modules for astria-bridge-withdrawer.

Background

required to implement rollup bridge withdrawals.

Changes

ethereum:

implement AstriaWithdrawer.sol contract, put inside ethereum/ dir as a foundry project
implement ethereum module which contains a Watcher that watches the contract for Withdrawal events
implement event to action conversion
the events are sent to a Batcher which batches all events by block number and sends a batch of actions to a sequencer handler

sequencer:

submit txs to sequencer when a batch of actions is received

Testing

unit tests

manual testing:

run sequencer+cometbft as normal (just run + just run-cometbft in astria-sequencer)
run anvil as normal (anvil in a terminal)
init the bridge account:

export SEQUENCER_PRIVATE_KEY=2bd806c97f0e00af1a1fc3328fa763a9269723c8db8fac4f93af71db186d6e90
./target/debug/astria-cli sequencer init-bridge-account --sequencer-url=http://localhost:26657 --rollup-name=astria --sequencer.chain-id=astria

just copy-env in astria-bridge-withdrawer, put the private key from above into a file, update .env to use that
go to astria-bridge-withdrawer/ethereum, cp local.env.example .env
deploy the withdrawer contract:

source .env
forge create --rpc-url $RPC_URL --private-key $PRIVATE_KEY src/AstriaWithdrawer.sol:AstriaWithdrawer

run this twice, makes 2 withdrawals (forces anvil to make 2 blocks) note: the withdrawer will only submit the withdraw for the first tx, as it's waiting for the next block before submitting the second and anvil doesn't auto-build blocks by default

forge script script/AstriaWithdrawer.s.sol:AstriaWithdrawerScript \
   --rpc-url $RPC_URL --broadcast --sig "withdrawToSequencer()" -vvvv

in astria-bridge-withdrawer, update ASTRIA_BRIDGE_WITHDRAWER_ETHEREUM_CONTRACT_ADDRESS to be the contract address deployed in step 6.
just run and you should see a withdrawal being submitted:

2024-05-24T19:36:22.952210Z DEBUG astria_bridge_withdrawer::withdrawer::submitter: fetched latest nonce nonce=1
2024-05-24T19:36:23.003380Z DEBUG astria_bridge_withdrawer::withdrawer::submitter: signed transaction tx_hash=faad567d4f71ba20bdd9a1f679c4c3875c4b172c96d236155d52eb7ca02cd389
2024-05-24T19:36:23.003742Z DEBUG submit_tx: astria_bridge_withdrawer::withdrawer::submitter: submitting signed transaction to sequencer nonce=1 transaction.hash=faad567d4f71ba20bdd9a1f679c4c3875c4b172c96d236155d52eb7ca02cd389
2024-05-24T19:36:24.049943Z  INFO astria_bridge_withdrawer::withdrawer::submitter: withdraw batch successfully executed. sequencer.block=881 sequencer.tx_hash=FAAD567D4F71BA20BDD9A1F679C4C3875C4B172C96D236155D52EB7CA02CD389 rollup.height=2

Related Issues

#913

closes #1109

## Summary implement `ethereum` module for astria-bridge-withdrawer. ## Background required to read from the astria withdrawal evm contract. ## Changes - implement `AstriaWithdrawer.sol` contract, put inside `ethereum/` dir as a foundry project - implement `ethereum` module which contains a `Watcher` that watches the contract for `Withdrawal` events - the events are sent to a `Batcher` which batches all events by block number and sends a batch of events to a sequencer handler (to be implemented later) ## Testing unit tests ## Related Issues #913

…idge-scaffolding

…er and ics20 withdrawals (#1101) ## Summary as title says ## Changes - update AstriaWithdrawer contract to have separate methods for sequencer and ics20 withdrawals `withdrawToSequencer` or `withdrawToOriginChain` - these methods now take different parameters and emit different events ## Testing unit tests

) ## Summary This adds the `Submitter` and merges in the `Watcher` related logic into a single `WithdrawService`. `Submitter` receives batches of rollup transactions that have been converted into sequencer `Action`s from the `Batcher` and does the following: 1. fetch the current nonce and create the `UnsignedTransaction` 2. sign the transaction 3. `broadcast_tx_commit` If submission to the sequencer fails for any reason (either in `CheckTx` or `DeliverTx`), the `Submitter` will stop, instead relying on the service's recovery process to reconstruct the batch from rollup transaction and resubmit. ## Background Withdrawal batches need to be submitted to the sequencer. ## Changes - Add `Submitter` - Add logic for converting the events reaped from rollup to `Action`s - a lot of random cleanup ## Testing How are these changes tested? ## Metrics - Some metrics we use in composer already since i copied over a good amount of submission logic: - NONCE_FETCH_COUNT - NONCE_FETCH_FAILURE_COUNT - NONCE_FETCH_LATENCY - CURRENT_NONCE - SEQUENCER_SUBMISSION_FAILURE_COUNT - SEQUENCER_SUBMISSION_LATENCY ## Related Issues Link any issues that are related, prefer full github links. closes  --------- Co-authored-by: elizabeth <elizabethjbinks@gmail.com>

…idge-scaffolding

SuperFluffy

I have skimmed the PR and left a few early comments.

Please provide blackbox tests for this service.

SuperFluffy · 2024-05-27T13:39:28Z

crates/astria-core/src/protocol/transaction/v1alpha1/action.rs

 /// It also contains a `return_address` field which may or may not be the same as the signer
 /// of the packet. The funds will be returned to the `return_address` in the case of a timeout.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]


All fields should be private.

all the other actions have pub fields - we should probably make constructors for them instead? but a bit out of scope for here

can we make a follow up issue here? seems a general code quality thing

Gonna let it pass here because the other actions also have their fields exposed. But yeah, in general I prefer to not leak that stuff. We should address this in a follow, prior to publishing this crate.

SuperFluffy · 2024-05-27T13:40:39Z

crates/astria-bridge-withdrawer/src/withdrawer/submitter/signer.rs

+    /// Construct a `SequencerSigner` from a file.
+    ///
+    /// The file should contain a hex-encoded ed25519 secret key.
+    pub(super) fn from_path<P: AsRef<Path>>(path: P) -> eyre::Result<Self> {


Suggested change

pub(super) fn from_path<P: AsRef<Path>>(path: P) -> eyre::Result<Self> {

pub(super) fn try_from_path<P: AsRef<Path>>(path: P) -> eyre::Result<Self> {

SuperFluffy · 2024-05-27T13:42:02Z

crates/astria-bridge-withdrawer/src/withdrawer/submitter/signer.rs

+};
+use sequencer_client::Address;
+
+pub(super) struct SequencerSigner {


This type does not in fact sign anything as far as I can see? I would expect there to be a method SequencerSigner::sign.

SuperFluffy · 2024-05-27T13:43:51Z

crates/astria-bridge-withdrawer/src/withdrawer/submitter/mod.rs

+mod signer;
+
+pub(super) struct Handle {
+    pub(super) batches_tx: mpsc::Sender<Batch>,


Make this private. No point having a handle if it just reveals its contents. Also, please don't overload abbreviations. In other parts of this crate _tx is used as transaction, whereas here it's used as sending

SuperFluffy · 2024-05-27T13:47:08Z

crates/astria-bridge-withdrawer/src/withdrawer/submitter/builder.rs

+
+impl Builder {
+    /// Instantiates an `Submitter`.
+    pub(crate) async fn build(self) -> eyre::Result<(super::Submitter, super::Handle)> {


There are multiple errors in here that don't have context

This constructor should ideally not be async

SuperFluffy · 2024-05-27T13:48:16Z

crates/astria-bridge-withdrawer/src/withdrawer/submitter/builder.rs

+        let sequencer_cometbft_client = sequencer_client::HttpClient::new(&*cometbft_endpoint)
+            .context("failed constructing cometbft http client")?;
+
+        let actual_chain_id = get_sequencer_chain_id(sequencer_cometbft_client.clone()).await?;


This could probably be moved into a "runtime intializer" that runs before the select! loop

This would remove the need to make this method async.

…idge-scaffolding

…divisor in code

joroshiba

Some general comments on source of withdrawal contract code, only change I really want to see on PR though is refactoring the github ci test logic. I want to be able to isolate the tests using all the foundry stuff in case it fails on us again.

.github/workflows/test.yml

joroshiba · 2024-05-29T20:17:15Z

crates/astria-bridge-withdrawer/ethereum/out/AstriaWithdrawer.sol/AstriaWithdrawer.json

is this a generated file? Should it be excluded from the repo in .gitignore? Or added to the generated files list?

this is generated by foundry - however we need it in the astria_withdrawer.rs module to do abigen for the contract, so i committed it

joroshiba · 2024-05-29T20:19:22Z

.gitmodules

@@ -0,0 +1,3 @@
+[submodule "crates/astria-bridge-withdrawer/ethereum/lib/forge-std"]


should we have the contracts outside of crates in the monorepo perhaps?

joroshiba · 2024-05-31T16:31:52Z

crates/astria-bridge-withdrawer/ethereum/local.env.example

@@ -0,0 +1,20 @@
+# anvil account 0
+PRIVATE_KEY=0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80


How are these env vars used with contract deployment? Is this foundry env var? Generally we want to put private keys in files.

I agree. We should never leak private keys in plain text.

this is anvil account 0 so it's a known private key. i can remove tho

joroshiba · 2024-05-31T16:33:51Z

crates/astria-bridge-withdrawer/local.env.example

+# The socket address at which the bridge service will server healthz, readyz, and status calls.
+ASTRIA_BRIDGE_WITHDRAWER_API_ADDR=127.0.0.1:2450
+
+# Set to true to enable prometheus metrics.


Suggested change

# Set to true to enable prometheus metrics.

# Set to true to disable prometheus metrics.

joroshiba · 2024-05-31T16:37:53Z

crates/astria-bridge-withdrawer/src/withdrawer/batch.rs

+use ethers::types::{
+    TxHash,
+    U64,
+};


I think it would be a bit more clear further down if these types weren't "used" and instead fully specify the type path below. I was confused about why u64 was capitalized further down and it's good to have clarity on what type of txHash we are talking about (it's an EVM tx hash not a cometbft one)

moved to the ethereum crate as this logic is eth-specific

joroshiba · 2024-05-31T16:43:13Z

crates/astria-core/src/protocol/transaction/v1alpha1/action.rs

 /// It also contains a `return_address` field which may or may not be the same as the signer
 /// of the packet. The funds will be returned to the `return_address` in the case of a timeout.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]


can we make a follow up issue here? seems a general code quality thing

…idge-scaffolding

joroshiba · 2024-05-31T17:21:54Z

.github/workflows/test.yml

          solc-select install 0.8.21
          solc-select use 0.8.21
-          cargo nextest run --archive-file=archive.tar.zst -- --include-ignored
+          cargo nextest run --package astria-bridge-withdrawer -- --include-ignored 


Suggested change

cargo nextest run --package astria-bridge-withdrawer -- --include-ignored

cargo nextest run --package astria-bridge-withdrawer -- --include-ignored

SuperFluffy · 2024-05-31T17:20:45Z

crates/astria-bridge-withdrawer/src/main.rs

+            .metrics_addr(&cfg.metrics_http_listener_addr)
+            .service_name(env!("CARGO_PKG_NAME"));
+    }
+    metrics_init::register();


Please register metrics the same way as all other services

SuperFluffy · 2024-05-31T17:22:10Z

crates/astria-bridge-withdrawer/ethereum/local.env.example

Where is this file used?

this is used to deploy the ethereum contract and make calls to it - see the readme in this crate

SuperFluffy · 2024-05-31T17:22:33Z

crates/astria-bridge-withdrawer/ethereum/local.env.example

@@ -0,0 +1,20 @@
+# anvil account 0
+PRIVATE_KEY=0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80


I agree. We should never leak private keys in plain text.

SuperFluffy · 2024-05-31T17:23:47Z

crates/astria-bridge-withdrawer/local.env.example

+# Address of cometbft/tendermint to request new block heights.
+# 127.0.0.1:26657 is the default socket address at which cometbft
+# serves RPCs.
+ASTRIA_BRIDGE_WITHDRAWER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"


Let's be clearer about this. This reads like it's the withdrawer's endpoint.

Suggested change

ASTRIA_BRIDGE_WITHDRAWER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"

ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"

SuperFluffy · 2024-05-31T17:25:22Z

crates/astria-bridge-withdrawer/src/withdrawer/mod.rs

+mod state;
+mod submitter;
+
+pub struct Service {


Call this WithdrawerBridge following all our other services.

that cause a clippy name repetitions error since this module is already called withdrawer and this isn't a bridge, so i'd prefer not to name it that

SuperFluffy · 2024-05-31T17:30:30Z

crates/astria-bridge-withdrawer/src/withdrawer/ethereum/watcher.rs

+        if let Ok((event, meta)) = item {
+            event_tx
+                .send((WithdrawalEvent::Ics20(event), meta))
+                .await
+                .wrap_err("failed to send ics20 withdrawal event; receiver dropped?")?;
+        } else if let Err(e) = item {
+            return Err(e).wrap_err("failed to read from event stream; event stream closed?");
+        }


As above, please simplify this.

SuperFluffy · 2024-05-31T17:33:38Z

crates/astria-bridge-withdrawer/src/withdrawer/ethereum/watcher.rs

+                    info!("batcher shutting down");
+                    break;
+                }
+                item = self.event_rx.recv() => {


This select! statement should break if self.event_rx.recv() == None because that means the channel is closed.

SuperFluffy · 2024-05-31T17:34:06Z

crates/astria-bridge-withdrawer/src/withdrawer/ethereum/watcher.rs

+    let bytes = hex::decode(s).wrap_err("failed to parse ethereum address as hex")?;
+    let address: [u8; 20] = bytes
+        .try_into()
+        .map_err(|_| eyre!("invalid length for ethereum address, must be 20 bytes"))?;


report the actual number of bytes received

SuperFluffy · 2024-05-31T17:36:00Z

crates/astria-core/src/protocol/transaction/v1alpha1/action.rs

 /// It also contains a `return_address` field which may or may not be the same as the signer
 /// of the packet. The funds will be returned to the `return_address` in the case of a timeout.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]


Gonna let it pass here because the other actions also have their fields exposed. But yeah, in general I prefer to not leak that stuff. We should address this in a follow, prior to publishing this crate.

SuperFluffy · 2024-05-31T17:36:52Z

crates/astria-bridge-withdrawer/src/withdrawer/submitter/mod.rs

+}
+
+/// Queries the sequencer for the latest nonce with an exponential backoff
+#[instrument(name = "get_latest_nonce", skip_all, fields(%address))]


name matches the function name. You can remove that attribute.

SuperFluffy · 2024-05-31T17:40:27Z

I would like a simple blackbox test here that shows that we can spin up the service, receive an event, and see it come out on the other side.

noot · 2024-05-31T21:19:57Z

@SuperFluffy comments are addressed, blackbox test will be done in a PR shortly

joroshiba

Re-approving noting this is blocking further development and blackbox tests are actively being built, we have a tight timeline can't delay on the tests.

blackbox tests non-blocking for greenfield work, in progress but this PR is blocking other work

binary scaffolding

3d98bc4

itamarreif self-assigned this Apr 19, 2024

itamarreif and others added 7 commits April 19, 2024 16:04

cleanup config vars

312ac95

cleanup names

dc13bb6

merge w main and make bridge compile and no warnings

b36a152

rename astria-bridge to astria-bridge-withdrawer

badadd1

lint toml

743f57d

clippy

687f371

lint

fade698

noot changed the title ~~bridge binary scaffolding~~ feat(bridge-withdrawer): begin astria-bridge-withdrawer implementation May 21, 2024

github-actions bot added the ci issues that are related to ci and github workflows label May 22, 2024

Merge branch 'main' of github.com:astriaorg/astria into itamarreif/br…

9e860e2

…idge-scaffolding

itamarreif assigned noot May 22, 2024

itamarreif added the bridging label May 22, 2024

noot and others added 2 commits May 23, 2024 16:55

noot changed the title ~~feat(bridge-withdrawer): begin astria-bridge-withdrawer implementation~~ feat(bridge-withdrawer): PoC astria-bridge-withdrawer implementation May 24, 2024

github-actions bot added conductor pertaining to the astria-conductor crate sequencer-relayer pertaining to the astria-sequencer-relayer crate composer pertaining to composer labels May 24, 2024

noot added 2 commits May 23, 2024 21:31

Merge branch 'main' of github.com:astriaorg/astria into itamarreif/br…

16ebcf6

…idge-scaffolding

cleanup

397ddbb

noot marked this pull request as ready for review May 24, 2024 01:34

noot requested review from a team, SuperFluffy, joroshiba and noot as code owners May 24, 2024 01:34

noot added 2 commits May 24, 2024 15:52

put solc back

1b3c1f7

don't cache pip

0e115f7

SuperFluffy reviewed May 27, 2024

View reviewed changes

noot added 3 commits May 28, 2024 12:00

Merge branch 'main' of github.com:astriaorg/astria into itamarreif/br…

10ad97a

…idge-scaffolding

address comments

4ee6fd1

Merge branch 'main' of github.com:astriaorg/astria into itamarreif/br…

6ce8d8e

…idge-scaffolding

noot requested a review from SuperFluffy May 28, 2024 16:30

noot added 3 commits May 30, 2024 14:06

Merge branch 'main' of github.com:astriaorg/astria into itamarreif/br…

f914103

…idge-scaffolding

implement ASSET_WITHDRAWAL_DECIMALS in contract and asset_withdrawal_…

bee7fb3

…divisor in code

clippy

4a914f8

itamarreif mentioned this pull request May 30, 2024

add abci query for allowed fee asset ids and check against it on startup #1123

Closed

joroshiba requested changes May 31, 2024

View reviewed changes

noot added 3 commits May 31, 2024 13:03

run tests using foundry into their own job

2dfd7c4

Merge branch 'main' of github.com:astriaorg/astria into itamarreif/br…

7271f75

…idge-scaffolding

fix nextest

7aba534

joroshiba approved these changes May 31, 2024

View reviewed changes

SuperFluffy previously requested changes May 31, 2024

View reviewed changes

address comments

9029d02

noot requested a review from SuperFluffy May 31, 2024 21:20

noot enabled auto-merge May 31, 2024 21:20

clippy

6968898

joroshiba approved these changes May 31, 2024

View reviewed changes

noot added this pull request to the merge queue Jun 1, 2024

Merged via the queue into main with commit afe4901 Jun 1, 2024

noot deleted the itamarreif/bridge-scaffolding branch June 1, 2024 00:10

noot mentioned this pull request Jun 5, 2024

sequencer: bridge withdrawal handler #913

Closed

	pub(super) fn from_path<P: AsRef<Path>>(path: P) -> eyre::Result<Self> {
	pub(super) fn try_from_path<P: AsRef<Path>>(path: P) -> eyre::Result<Self> {

		@@ -0,0 +1,3 @@
		[submodule "crates/astria-bridge-withdrawer/ethereum/lib/forge-std"]

		@@ -0,0 +1,20 @@
		# anvil account 0
		PRIVATE_KEY=0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80

	# Set to true to enable prometheus metrics.
	# Set to true to disable prometheus metrics.

	cargo nextest run --package astria-bridge-withdrawer -- --include-ignored
	cargo nextest run --package astria-bridge-withdrawer -- --include-ignored

	ASTRIA_BRIDGE_WITHDRAWER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"
	ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_COMETBFT_ENDPOINT="http://127.0.0.1:26657"

Comments

Conversation

itamarreif commented Apr 19, 2024 • edited by noot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Changes

Testing

Related Issues

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!

Choose a reason for hiding this comment

Uh oh!

joroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

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

itamarreif commented Apr 19, 2024 •

edited by noot

Loading