Skip to content

refactor(withdrawer): read from block subscription stream and get events on each block#1207

Merged
noot merged 9 commits intomainfrom
noot/bridge-events
Jun 27, 2024
Merged

refactor(withdrawer): read from block subscription stream and get events on each block#1207
noot merged 9 commits intomainfrom
noot/bridge-events

Conversation

@noot
Copy link
Contributor

@noot noot commented Jun 25, 2024

Summary

refactor the bridge withdrawer to read only from one block subscription stream instead of a block stream + event streams. then, when a new block is received, the events for that block are batched and sent to the submitter.

Background

this fixes race conditions between the block/event streams and ensures proper batching; ie. that one rollup block = one sequencer tx.

Changes

  • refactor the bridge withdrawer to read only from one block subscription stream
  • on startup, gets the events in each block from the last block submitted+1 up until the current block, and batches them by block and sends to submitter
  • on receiving a new block, batches the events and sends them to the submitter

Testing

unit tests

@noot noot requested a review from a team as a code owner June 25, 2024 21:17
@noot noot requested a review from Fraser999 June 25, 2024 21:17
event_tx: mpsc::Sender<(WithdrawalEvent, LogMeta)>,
from_block: u64,
async fn sync_from_next_rollup_block_height(
provider: Arc<Provider<Ws>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like in almost all places we're using Arc<Provider<Ws>> we can get away with just &Provider<Ws>. Looks like the only place we need the provider wrapped in an Arc is in IAstriaWithdrawer::new, and we can just create one inline there.

Copy link
Contributor Author

@noot noot Jun 26, 2024

Choose a reason for hiding this comment

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

unfortunately it needs to be in an Arc due to the loop here:

    loop {
        select! {
            () = shutdown_token.cancelled() => {
                info!("block watcher shutting down");
                return Ok(());
            }
            block = block_rx.next() => {
                if let Some(block) = block {
                    get_and_send_events_at_block(
                        provider.clone(),
                        contract_address,
                        block,
                        &converter,
                        &submitter_handle,
                    )
                    .await
                    .wrap_err("failed to get and send events at block")?;
                }
            }
        }
    }

have to clone it otherwise it gets moved in the previous loop iteration

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to change get_and_send_events_at_block to take provider: &Provider<Ws> too though (and all the other places taking an Arc)?

@noot noot requested a review from Fraser999 June 26, 2024 18:27
Copy link
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

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

Approved, but I still think wrapping Provider in an Arc is not required (except in just the single place indicated in the comment). Not a huge deal though - just a micro-optimization to avoid using atomic ref-counting :)

@noot noot enabled auto-merge June 27, 2024 19:31
@noot noot added this pull request to the merge queue Jun 27, 2024
Merged via the queue into main with commit dc76efc Jun 27, 2024
@noot noot deleted the noot/bridge-events branch June 27, 2024 19:37
steezeburger added a commit that referenced this pull request Jul 11, 2024
* main: (27 commits)
  refactor(sequencer): fix prepare proposal metrics (#1211)
  refactor(bridge-withdrawer): move generated contract bindings to crate (#1237)
  fix(sequencer) fix wrong metric and remove unused metric (#1240)
  feat(sequencer): implement transaction fee query (#1196)
  chore(cli)!: remove unmaintained rollup subcommand (#1235)
  release(sequencer): 0.14.1 patch release (#1233)
  feat(sequencer-utils): generate example genesis state (#1224)
  feat(sequencer): implement abci query for bridge account info (#1189)
  feat(charts): bridge-withdrawer, smoke test, various chart improvements (#1141)
  chore(charts): update for new geth update (#1226)
  chore(chart)!: dusk-8 chart version updates (#1223)
  release(conductor): fix conductor release version (#1222)
  release: dusk-8 versions (#1219)
  fix(core): revert `From` ed25519_consensus types for crypto mod (#1220)
  Refactor(chart, sequencer): restructure sequencer chart, adjust configs (#1193)
  refactor(withdrawer): read from block subscription stream and get events on each block (#1207)
  feat(core): implement with verification key for address builder and crypto improvements (#1218)
  feat(proto, sequencer)!: use full IBC ICS20 denoms instead of IDs (#1209)
  chore(chart): update evm chart for new prefix field (#1214)
  chore: bump penumbra deps (#1216)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants