fix(bridge-withdrawer, cli, sequencer-client): migrate from broadcast_tx_commit to broadcast_tx_sync
#1376
Conversation
ethanoroshiba
force-pushed
the
ENG-20/use_tx_sync
branch
from
76d5149 to
44319e5
Compare
github-actions
bot
added
the
ci
ethanoroshiba
force-pushed
the
ENG-20/use_tx_sync
branch
from
44319e5 to
327f093
Compare
ethanoroshiba
added
sequencer-client
cli
| /// - If the transaction proof is missing. | ||
| #[allow(clippy::blocks_in_conditions)] // Allow: erroneous clippy warning. Should be fixed in Rust 1.81 | ||
| #[instrument(skip_all, err)] | ||
| async fn wait_for_tx_inclusion( |
There was a problem hiding this comment.
It might be worth considering following a similar approach to that taken in the sequencer-relayer for confirm_submission which is essentially doing the same thing, but targeting Celestia rather than a sequencer.
The main differences are:
- it tries forever, hence the function doesn't return a
Result - when the server sends a success response, but indicates the tx hash wasn't found, the retry interval stays at the minimum
- because we can keep retrying fairly frequently, there's a logging strategy to avoid spamming the logs
If you decide to go that way, then bear in mind that the timing consts were chosen for a Celestia block time of 12 seconds, whereas the sequencer block times are closer to 2 seconds I believe.
There was a problem hiding this comment.
Thanks for the tip! I changed the logic to reflect the function you sent over. The only differences are:
- used millis instead of secs since the timeframe of this is much shorter
- kept the result return value. If we get a response which has a non-ok error code or doesn't include proof of the tx's inclusion in the block, I think it would probably still be good to throw an error.
There was a problem hiding this comment.
It looks like we're always returning Ok, so maybe the return type can be changed to just tendermint_rpc::endpoint::tx::Response?
There was a problem hiding this comment.
Should we also have a wrapper function with a timeout, similar to the one you sent? Seems like it may be problematic to introduce code which will try forever unchecked instead of erroring out at some point.
Co-authored-by: noot <36753753+noot@users.noreply.github.com>
* main: feat(sequencer): make mempool balance aware (#1408) chore(sequencer): change test addresses to versions with known private keys (#1487) chore(chart): update geth tag (#1485) feat(sequencer): report deposit events (#1447) feat(proto, core, sequencer)!: add traceability to rollup deposits (#1410) fix(bridge-withdrawer, cli, sequencer-client): migrate from `broadcast_tx_commit` to `broadcast_tx_sync` (#1376) fix(sequencer): add `end_block` to `app_execute_transaction_with_every_action_snapshot` (#1455) release: end of iteration release cuts (#1456) chore(charts): rollupName templates (#1458) chore(sequencer-relayer): Add instrumentation (#1375) feat(proto, core, sequencer)!: permit bech32 compatible addresses (#1425) chore: memoize `address_bytes` of verification key (#1444) chore(ci): include `ibc-bridge-test` in `docker` CI target (#1438) chore(charts): bump celestia versions (#1431)
Summary
Deleted all usage of
broadcast_tx_commitand instead implementedbroadcast_tx_sync.Background
Previously, we relied on
broadcast_tx_committo submit transactions to the sequencer. CometBFT RPC docs warn against this and instead favor usingbroadcast_tx_sync.Changes
broadcast_tx_commit, replacing withbroadcast_tx_sync.wait_for_tx_inclusion()to probe the sequencer client for a new transaction of the given hash with backoff.Testing
sequencer-clientto ensurewait_for_tx_inclusion()works properlyRelated Issues
closes #1283