feat(sequencer, bridge-withdrawer)!: enforce withdrawals consumed by joroshiba · Pull Request #1391 · astriaorg/astria

joroshiba · 2024-08-22T01:24:06Z

Summary

Adds state for bridge events and consumes the events as they occur. Updates proto for bridge unlock to no longer use memo for rollup information and instead include on main action. Renames rollup_transaction_hash to rollup_withdrawal_event_id.

Background

There was only implicit stop against reusing a withdrawal event, this adds consumption of withdrawal events. While this is not strictly security enhancing, consuming the event adds in protocol protection against accidental double spend by the bridge operator.

Changes

Proto updates for BridgeUnlockAction + ICS20WithdrawalMemo
Add stateless checks on bridging unlock and withdrawals of correct information filled out
Added state to enforce rollup event cannot be consumed twice.
Enforce that Ics20WithdrawalAction must have bridge_address set if making a bridge withdrawal

Testing

smoke test + minor test updates

Breaking Changelist

New stateful information about rollup withdrawal events
Adds new fields to BridgeUnlockAction

Related Issues

closes #1430

Fraser999

Mostly nitpicks, but one issue in bridge/state_ext.rs causing the rollup block height to not be parsed correctly. Not blocking the PR though since the actual value isn't critical to the flow, only whether it exists in storage or not.

crates/astria-bridge-contracts/src/lib.rs

crates/astria-sequencer/src/bridge/bridge_unlock_action.rs

Fraser999 · 2024-08-28T16:43:37Z

crates/astria-sequencer/src/ibc/ics20_withdrawal.rs

        ensure!(self.timeout_time() != 0, "timeout time must be non-zero",);
+        ensure!(self.amount() > 0, "amount must be greater than zero",);
+        if self.bridge_address.is_some() {
+            let parsed_bridge_memo: Ics20WithdrawalFromRollupMemo =


We're JSON-encoding a PB version of this type, whereas here we're decoding straight into our domain type. The serde try_from = "raw::Ics20WithdrawalFromRollup" on the domain type handles that ok, but I still keep feeling uneasy when I see this.

I could either remove the domain type or modify the serialization on the other end, it uses the pb::name property when serializing for errors which is why it uses the raw type

It's certainly not a blocker, and maybe I'm being overly pedantic. But I guess I'd prefer if the domain type didn't derive serde traits, forcing us to always use the pb type for encoding to/decoding from JSON, and we use the domain type in the business logic.

No probs if you prefer to leave as is - I think we're doing similar things elsewhere. We can always try and get more strict on the separation of pb and domain types in the future.

I went ahead and went back to how the memos were defined in domain type as reexporting, less code changes here a bit cleaner I think.

IMO this check_stateless (and the other deseriailzation in check_and_execute) shows that using validated domain types that establish invariants is preferable over doing the validation here.

I have created an issue here for follow up

crates/astria-sequencer/src/ibc/ics20_withdrawal.rs

Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com>

SuperFluffy

Everything in this PR seems fine. I am still blocking this because I am uneasy about the keys used to put the events into state (see the comment there).

proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto

proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto

crates/astria-sequencer/src/ibc/ics20_transfer.rs

SuperFluffy · 2024-08-29T15:59:21Z

crates/astria-sequencer/src/bridge/bridge_unlock_action.rs

 #[async_trait::async_trait]
 impl ActionHandler for BridgeUnlockAction {
    async fn check_stateless(&self) -> Result<()> {
+        ensure!(self.amount > 0, "amount must be greater than zero",);


What would be so bad if it was 0?

It's just kinda pointless? I would prefer not wasting compute time on pointless actions :)

crates/astria-sequencer/src/ibc/ics20_withdrawal.rs

SuperFluffy · 2024-08-29T16:16:14Z

crates/astria-sequencer/src/ibc/ics20_withdrawal.rs

        ensure!(self.timeout_time() != 0, "timeout time must be non-zero",);
+        ensure!(self.amount() > 0, "amount must be greater than zero",);
+        if self.bridge_address.is_some() {
+            let parsed_bridge_memo: Ics20WithdrawalFromRollupMemo =


IMO this check_stateless (and the other deseriailzation in check_and_execute) shows that using validated domain types that establish invariants is preferable over doing the validation here.

crates/astria-sequencer/src/ibc/ics20_withdrawal.rs

crates/astria-sequencer/src/bridge/state_ext.rs

SuperFluffy

Discussed offline: BridgeUnlockAction::check_and_execute contains a check that no event with a given ID was previously processed, so a second action would not be able to overwrite the first. That addresses my issue.

Approving.

crates/astria-sequencer/src/bridge/bridge_unlock_action.rs

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

… method

crates/astria-sequencer/src/bridge/state_ext.rs

crates/astria-sequencer/src/ibc/ics20_withdrawal.rs

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

* main: chore: ibc e2e smoke test (#1284) chore(metrics): restrict `metrics` crate usage to `astria-telemetry` (#1192) fix(charts)!: sequencer-relayer chart correct startup env var (#1437) chore(bridge-withdrawer): Add instrumentation (#1324) chore(conductor): Add instrumentation (#1330) fix(cli, bridge-withdrawer): dont fail entire block due to bad withdraw event (#1409) feat(sequencer, bridge-withdrawer)!: enforce withdrawals consumed (#1391)

) ## Summary Adds state for bridge events and consumes the events as they occur. Updates proto for bridge unlock to no longer use memo for rollup information and instead include on main action. Renames `rollup_transaction_hash` to `rollup_withdrawal_event_id`. ## Background There was only implicit stop against reusing a withdrawal event, this adds consumption of withdrawal events. While this is not strictly security enhancing, consuming the event adds in protocol protection against accidental double spend by the bridge operator. ## Changes - Proto updates for `BridgeUnlockAction` + `ICS20WithdrawalMemo` - Add stateless checks on bridging unlock and withdrawals of correct information filled out - Added state to enforce rollup event cannot be consumed twice. - Enforce that `Ics20WithdrawalAction` must have `bridge_address` set if making a bridge withdrawal ## Testing smoke test + minor test updates ## Breaking Changelist - New stateful information about rollup withdrawal events - Adds new fields to `BridgeUnlockAction` ## Related Issues closes #1430 --------- Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com> Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels Aug 22, 2024

joroshiba temporarily deployed to BUF August 22, 2024 01:24 — with GitHub Actions Inactive

joroshiba temporarily deployed to BUF August 27, 2024 21:15 — with GitHub Actions Inactive

joroshiba temporarily deployed to BUF August 27, 2024 21:38 — with GitHub Actions Inactive

joroshiba temporarily deployed to BUF August 27, 2024 22:04 — with GitHub Actions Inactive

joroshiba added 6 commits August 27, 2024 15:07

feat(sequencer, bridge-withdrawer)!: enforce withdrawals consumed

f4bc8db

updated

97a93c2

fix cargo check, add memo back in

458529d

some formatting improvements

91ffc80

clippy

dbffdbd

snapshots

64bcc9a

joroshiba force-pushed the joroshiba/start-event-consumption branch from 7a1c32c to 64bcc9a Compare August 27, 2024 22:32

joroshiba temporarily deployed to BUF August 27, 2024 22:32 — with GitHub Actions Inactive

tests

cf71063

joroshiba temporarily deployed to BUF August 28, 2024 00:07 — with GitHub Actions Inactive

fmt

3057605

joroshiba marked this pull request as ready for review August 28, 2024 00:14

joroshiba requested review from a team, SuperFluffy and noot as code owners August 28, 2024 00:14

joroshiba temporarily deployed to BUF August 28, 2024 00:14 — with GitHub Actions Inactive

joroshiba requested a review from Fraser999 August 28, 2024 15:16

Fraser999 reviewed Aug 28, 2024

View reviewed changes

review updates

b90b504

joroshiba temporarily deployed to BUF August 28, 2024 21:28 — with GitHub Actions Inactive

format + state_ext fix

cc54c1e

joroshiba temporarily deployed to BUF August 28, 2024 21:33 — with GitHub Actions Inactive

review suggestion

a1076a6

Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com>

SuperFluffy suggested changes Aug 29, 2024

View reviewed changes

SuperFluffy approved these changes Aug 29, 2024

View reviewed changes

crates/astria-sequencer/src/bridge/bridge_unlock_action.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

1f2400f

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

joroshiba temporarily deployed to BUF August 29, 2024 17:30 — with GitHub Actions Inactive

joroshiba mentioned this pull request Aug 29, 2024

refactor stateless checks to be done on types with withdrawals #1430

Closed

update ics logic for stateless to work, make consumption single state…

8216576

… method

joroshiba temporarily deployed to BUF August 29, 2024 19:33 — with GitHub Actions Inactive

SuperFluffy reviewed Aug 29, 2024

View reviewed changes

crates/astria-sequencer/src/bridge/state_ext.rs Outdated Show resolved Hide resolved

joroshiba added 2 commits August 29, 2024 12:40

generate proto comments

28bab18

proto comment

268139d

joroshiba temporarily deployed to BUF August 29, 2024 19:42 — with GitHub Actions Inactive

SuperFluffy reviewed Aug 29, 2024

View reviewed changes

crates/astria-sequencer/src/ibc/ics20_withdrawal.rs Outdated Show resolved Hide resolved

minors

b4eaef0

joroshiba temporarily deployed to BUF August 29, 2024 19:48 — with GitHub Actions Inactive

Update proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto

cfa684b

Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>

joroshiba temporarily deployed to BUF August 29, 2024 19:49 — with GitHub Actions Inactive

proto comment regen

f7a6df3

joroshiba temporarily deployed to BUF August 29, 2024 19:50 — with GitHub Actions Inactive

replace alias

6d1a000

joroshiba temporarily deployed to BUF August 29, 2024 19:52 — with GitHub Actions Inactive

format

93ab08e

joroshiba temporarily deployed to BUF August 29, 2024 19:53 — with GitHub Actions Inactive

joroshiba enabled auto-merge August 29, 2024 19:56

joroshiba added this pull request to the merge queue Aug 29, 2024

Merged via the queue into main with commit 9f61870 Aug 29, 2024

joroshiba deleted the joroshiba/start-event-consumption branch August 29, 2024 20:05

Conversation

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

Uh oh!

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

Uh oh!

Fraser999 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joroshiba commented Aug 22, 2024 •

edited

Loading