fix(bridge-withdrawer)!: fix nonce handling#1215
Merged
SuperFluffy merged 40 commits intomainfrom Jul 15, 2024
Merged
Conversation
…idge-withdrawer-errorscleanup
…idge-withdrawer-errorscleanup
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
Contributor
Author
|
This required calling the sequencer's gRPC service, which broke the tests. I am working on a separate PR that would add the black box tests (#1227) instead of directly fixing this. |
…idge-withdrawer/nonce-fix
…idge-withdrawer/nonce-fix
itamarreif
changed the title
This was referenced Jul 15, 2024
SuperFluffy
approved these changes
Jul 15, 2024
…amarreif/bridge-withdrawer/nonce-fix
github-actions
bot
added
the
ci
steezeburger
added a commit
that referenced
this pull request
Jul 19, 2024
* main: (24 commits) chore: update `bytes` and `ics23` crates (#1279) fix(sequencer): improve and fix instrumentation (#1255) feature(charts): hermes chart fixes, bech32 updates, ibc bridge test (#1130) chore(cli): remove unused rollup cli code (#1275) chore(test): use a temporary file to not pollute the workspace (#1269) chore(sequencer): add mempool benchmarks (#1238) fix(bridge-withdrawer)!: fix nonce handling (#1215) feat(cli, bridge-withdrawer)!: share code between cli and service (#1270) feat(cli): add cmd to collect withdrawal events and submit as actions (#1261) fix(core, bridge, sequencer)!: dismabiguate return addresses (#1266) fix(withdrawer): support withdrawer address that differs from bridge address (#1262) (core, sequencer)!: generate serde traits impls for all protocol protobufs (#1260) fix(charts): add resources for sequencer/cometbft (#1254) chore(sequencer)!: add metrics (#1248) fix(sequencer-utils): fixes issue in `parse_blob` tests (#1243) feat(core, proto)!: make bridge unlock memo string (#1244) fix(conductor): don't panic during panic (#1252) feat(core)!: lowerCamelCase for protobuf json mapping (#1250) refactor(bridge-withdrawer)!: refactor startup to a separate subtask and remove balance check from startup (#1190) fix: rollup archive node configurations (#1249) ...
bharath-123
pushed a commit
that referenced
this pull request
Jul 25, 2024
## Summary Adds a check that waits for an empty mempool on startup and then uses the pending nonce instead of latest finalized nonce for transaction submission. ## Background Before these changes we used the latest nonce, which could lead to data races if there are batches awaiting inclusion in the mempool. See #1228 for more context. ## Changes - Add `ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_GRPC_ENDPOINT` config - Wait for mempool to be empty as part of startup - Submitter connects to the grpc service as part of its startup - Submitter uses pending nonces for batch submission instead of latest nonce ## Testing Removed most testing; reworked in #1232 ## Metrics - Punted: #1272 ## Breaking Changelist - Adds a required env var `ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_GRPC_ENDPOINT` ## Related Issues closes #1228 --------- Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 29, 2024
) ## Summary changes made in #1215 required adding a mock gRPC server. Moving to black box tests was the next step planned so this will replace the invalidated unit tests with valid black box tests. ## Background Brief background on why these changes were made, ie "why?" ## Changes * Add black box tests for native and erc20 sequencer and ics20 withdrawals * test helpers for the mock cometbft server * test helpers for the mock sequencer gRPC server * test helpers for managing anvil & ethereum node * remove the "unit tests" in `submitter/tests.rs` * some logs i found useful to add while putting this together ## Testing * native withdrawal to the sequencer works * native withdrawal with ics20 works * erc20 to the sequencer works * erc20 with ics20 works ## Related Issues Link any issues that are related, prefer full github links. closes #1227 --------- Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 30, 2024
## Summary Deduplicated and added metrics to `get_pending_nonce()`, fixed too many arguments in `process_batch()`. ## Background Following #1215, `get_pending_nonce()` was present in both the bridge-withdrawer and submitter, in which the submitter's `get_pending_nonce()` also did not implement metrics. The submitter's `process_batch()` function also had a Clippy exception for too many arguments. ## Changes - Deleted `bridge_withdrawer/startup.rs: get_pending_nonce()` and its helper function `make_sequencer_grpc_retry_config`. - Instead imported `submitter/mod.rs: get_pending_nonce()` and formatted accordingly. - Added optional metrics argument to submitter `get_pending_nonce()` to allow for it to be used in the bridge-withdrawer startup. - Added conditional metrics to `get_pending_nonce()`, as well as mutators within `Metrics`. - Moved `process_batch()` inside `Submitter` to allow for sharing of members between the two, cutting down arguments and removing Clippy exception. ## Testing Passes blackbox tests. ## Metrics - Added `nonce_fetch_count`. - Added `nonce_fetch_failure_count`. - Added `nonce_fetch_latency`. ## Related Issues Closes #1272 Closes #1273 Closes #1274 --------- Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a check that waits for an empty mempool on startup and then uses the pending nonce instead of latest finalized nonce for transaction submission.
Background
Before these changes we used the latest nonce, which could lead to data races if there are batches awaiting inclusion in the mempool. See #1228 for more context.
Changes
ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_GRPC_ENDPOINTconfigTesting
Removed most testing; reworked in #1232
Metrics
Breaking Changelist
ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_GRPC_ENDPOINTRelated Issues
closes #1228