STR-376: create the machinery around polling and execution of bridge duties v2 #293

storopoli · 2024-09-18T15:32:30Z

Description

The machinery around how the bridge client will query for duties from the CL,
delegate them to various handlers for their execution,
as well as the actually execute these duties within those handlers.

TODO:

Implement the RPCs for the bridge client
Implement all the handler traits as defined in STR-83 (done in EXP-83 Create basic scaffolding for bridge core impl #230).

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

Related Issues

STR-376

crates/bridge-exec/src/withdrawal_handler/handler.rs

codecov · 2024-09-18T20:11:43Z

Codecov Report

Attention: Patch coverage is 8.38926% with 273 lines in your changes missing coverage. Please review.

Project coverage is 61.02%. Comparing base (2391bf2) to head (96b2742).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
crates/bridge-exec/src/handler.rs	0.00%	163 Missing ⚠️
crates/primitives/src/relay/types.rs	0.00%	40 Missing ⚠️
crates/primitives/src/l1.rs	3.12%	31 Missing ⚠️
crates/rpc/types/src/types.rs	0.00%	25 Missing ⚠️
crates/primitives/src/buf.rs	0.00%	7 Missing ⚠️
crates/primitives/src/bridge.rs	0.00%	3 Missing ⚠️
sequencer/src/rpc_server.rs	0.00%	3 Missing ⚠️
crates/bridge-exec/src/errors.rs	0.00%	1 Missing ⚠️

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
- Coverage   61.70%   61.02%   -0.69%     
==========================================
  Files         209      207       -2     
  Lines       21449    21703     +254     
==========================================
+ Hits        13235    13244       +9     
- Misses       8214     8459     +245

Files with missing lines	Coverage Δ
crates/primitives/src/relay/util.rs	`96.63% <100.00%> (+0.14%)`	⬆️
crates/rpc/api/src/lib.rs	`0.00% <ø> (ø)`
crates/bridge-exec/src/errors.rs	`0.00% <0.00%> (ø)`
crates/primitives/src/bridge.rs	`86.97% <0.00%> (-0.93%)`	⬇️
sequencer/src/rpc_server.rs	`0.00% <0.00%> (ø)`
crates/primitives/src/buf.rs	`77.45% <0.00%> (-3.27%)`	⬇️
crates/rpc/types/src/types.rs	`0.00% <0.00%> (ø)`
crates/primitives/src/l1.rs	`73.07% <3.12%> (-3.09%)`	⬇️
crates/primitives/src/relay/types.rs	`48.38% <0.00%> (-21.39%)`	⬇️
crates/bridge-exec/src/handler.rs	`0.00% <0.00%> (ø)`

... and 4 files with indirect coverage changes

storopoli · 2024-09-24T14:27:07Z

@Rajil1213 this is ready for the first round of reviews.
I'd like to first a good discussion on the state of things, before proceeding to changing/adding tests.

Rajil1213

I think all the pieces are there and some of the issues I pointed out could have been due to the APIs in bridge-sig-manager and tx-builder not being very clear in their intent.

The deeper refactoring that needs to be done here is to consolidate the deposit and withdrawal handlers together. It no longer makes sense to have them be separate because they do exactly the same thing but on different transactions. The separate transactions are created at a lower-level (in the sig-manager and tx-builder crates). So, the exec part only needs to worry about passing in the right parameters.

The more important role that the exec part plays should be about tracking the duty lifecycle from the point that a duty is received to the point that they are dispatched, calling all the relevant RPCs and maintaining information about itself (keys, index, etc.). It also needs to maintain a checkpoint (the L1 block height) up to which it has already seen duties. This can be used to pull in deposit duties as those are stored off-chain in a database by the rollup node. The implementation for this duty RPC is under way and I should have a draft PR ready in a couple of hours today or by tomorrow at the latest. The other withdrawal duty RPC behaves the same way but does not need a checkpoint because you always get the latest set of duties in the current rollup chain state.

I think there is enough refactoring work to be done here for the duty RPCs to not be a blocker but let me know if you're blocked on anything and we can collaborate some more on how to get both things done in parallel (agreeing on the RPC interface could be one).

crates/bridge-exec/src/deposit_handler/handler.rs

crates/bridge-exec/src/utils.rs

crates/bridge-exec/src/withdrawal_handler/errors.rs

crates/bridge-exec/src/withdrawal_handler/handler.rs

crates/btcio/src/rpc/types.rs

crates/db/src/entities/bridge_tx_state.rs

crates/rpc/types/src/types.rs

delbonis

Structurally seems pretty good. Mostly some comments about the logging though.

crates/bridge-exec/src/withdrawal_handler/handler.rs

crates/bridge-exec/src/utils.rs

crates/bridge-exec/src/deposit_handler/handler.rs

storopoli · 2024-09-25T21:32:17Z

Sorry @delbonis this was into heavy rebasing and overhaul. I had two "pair-coding" sessions with @Rajil1213 to get this in a proper state.

I am afraid that you might have to review again, but it is MUCH more simpler and with less code than when you last reviewed.
Also, the commits are properly ordered and somewhat atomic 😄.

I did a bunch of cleanups and QoL improvements in the bridge message machinery, check if these changes are ok.

storopoli · 2024-09-26T08:47:12Z

I'll fix soon today the logging stuff, check the future-correct version of span.enter() and add the Error variants and improvements mentioned.

storopoli · 2024-09-26T11:45:41Z

I agree that consolidating more of the assembly/signing would be desirable.

Yes, @Zk2u discussed applying the terrors OneOf pattern here for the T, E in the Result.

storopoli · 2024-09-26T11:46:46Z

@delbonis and @Rajil1213 ready for another round.

Rajil1213 · 2024-09-26T17:06:02Z

Looks like @Zk2u is working on doing some refactoring on top of this PR in #324 which has already been approved. Should we wait to review this PR until that's merged @storopoli as I can see a bunch of common files being changed there?

storopoli · 2024-09-26T17:33:42Z

The refactor is only to unify the handlers.
The logic is not touched there, so you can review this PR and the ongoing merge of #324 will not impact the review.

delbonis

If the other comments have been resolved one way or another then this seems good to me.

Rajil1213

A few more nits and then, I think we are good to go

We will need more extensive automated testing for all of this but hopefully we can use the functional-tests when we have the bridge client binary operational.

crates/bridge-exec/src/deposit_handler/handler.rs

crates/primitives/src/relay/types.rs

sequencer/src/rpc_server.rs

storopoli · 2024-09-26T18:24:04Z

@Rajil1213 I've added all your suggestions and will open a ticket for the unimplemented AlpenApi RPC call.

Rajil1213

Left a few more comments after the merge that I missed the last time around.

crates/bridge-exec/src/handler.rs

crates/bridge-sig-manager/src/manager.rs

crates/db/src/entities/bridge_tx_state.rs

crates/primitives/src/relay/types.rs

primitives

crates/bridge-exec/src/handler.rs

Rajil1213 · 2024-09-26T19:15:58Z

Okay. I'll keep an eye on the CI checks and will approve as soon as they pass.

storopoli · 2024-09-26T20:07:34Z

This can be a commit merge

storopoli added the bridge Relates to the core bridge machinery label Sep 18, 2024

storopoli self-assigned this Sep 18, 2024

Rajil1213 reviewed Sep 18, 2024

View reviewed changes

crates/bridge-exec/src/withdrawal_handler/handler.rs Outdated Show resolved Hide resolved

john-light changed the title ~~Exp 183 create the machinery around polling and execution of bridge duties v2~~ STR-376: create the machinery around polling and execution of bridge duties v2 Sep 18, 2024

storopoli mentioned this pull request Sep 19, 2024

STR 361 / EXP 250 devnet cooperative withdrawal signing reimbursement #292

Merged

11 tasks

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch 6 times, most recently from 05a46d9 to e258a8e Compare September 24, 2024 14:25

storopoli marked this pull request as ready for review September 24, 2024 14:26

storopoli requested a review from Rajil1213 September 24, 2024 14:26

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from e258a8e to c090b4d Compare September 24, 2024 14:42

Rajil1213 requested changes Sep 24, 2024

View reviewed changes

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch 4 times, most recently from d4fc7bd to ae9eec2 Compare September 25, 2024 17:05

Rajil1213 mentioned this pull request Sep 25, 2024

STR-406 Implement deposit duty and other RPCs #320

Merged

13 tasks

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from ae9eec2 to 8ca8223 Compare September 25, 2024 20:14

delbonis requested changes Sep 25, 2024

View reviewed changes

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from 8ca8223 to 840d444 Compare September 25, 2024 21:28

storopoli requested review from delbonis and Rajil1213 September 25, 2024 21:30

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from 840d444 to d8ed865 Compare September 26, 2024 08:44

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from d8ed865 to aed88ac Compare September 26, 2024 11:40

storopoli mentioned this pull request Sep 26, 2024

STR-203 Implement withdrawal dispatch #315

Merged

11 tasks

delbonis previously approved these changes Sep 26, 2024

View reviewed changes

feat(primitives): Borsh-friendly BitcoinTxid

47c510a

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from aed88ac to 91aaffa Compare September 26, 2024 18:04

Rajil1213 reviewed Sep 26, 2024

View reviewed changes

storopoli dismissed delbonis’s stale review via 3afb64e September 26, 2024 18:12

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from 3afb64e to 7ebcef5 Compare September 26, 2024 18:22

storopoli requested a review from Rajil1213 September 26, 2024 18:27

Rajil1213 requested changes Sep 26, 2024

View reviewed changes

storopoli added 2 commits September 26, 2024 15:41

feat(primitives)!: improve usability of BridgeMessage

6a974b3

primitives

fix(primitives)!: un-Arc secp use global context

493b21b

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from 7ebcef5 to 4b9472a Compare September 26, 2024 18:43

storopoli requested a review from Rajil1213 September 26, 2024 18:44

Rajil1213 requested changes Sep 26, 2024

View reviewed changes

crates/bridge-exec/src/handler.rs Outdated Show resolved Hide resolved

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from 4b9472a to 0c813d1 Compare September 26, 2024 19:08

Rajil1213 reviewed Sep 26, 2024

View reviewed changes

crates/bridge-exec/src/handler.rs Outdated Show resolved Hide resolved

feat(bridge-exec)!: sign_tx and aggregate_sig

96b2742

storopoli force-pushed the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch from 0c813d1 to 96b2742 Compare September 26, 2024 19:14

storopoli requested a review from Rajil1213 September 26, 2024 20:06

Rajil1213 approved these changes Sep 26, 2024

View reviewed changes

Rajil1213 merged commit 9467a55 into master Sep 26, 2024
16 of 17 checks passed

storopoli deleted the EXP-183-create-the-machinery-around-polling-and-execution-of-bridge-duties-v2 branch September 26, 2024 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STR-376: create the machinery around polling and execution of bridge duties v2 #293

STR-376: create the machinery around polling and execution of bridge duties v2 #293

storopoli commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

storopoli commented Sep 24, 2024

Rajil1213 left a comment •

edited

Loading

delbonis left a comment

storopoli commented Sep 25, 2024

storopoli commented Sep 26, 2024

storopoli commented Sep 26, 2024

storopoli commented Sep 26, 2024 •

edited

Loading

Rajil1213 commented Sep 26, 2024 •

edited

Loading

storopoli commented Sep 26, 2024

delbonis left a comment

Rajil1213 left a comment •

edited

Loading

storopoli commented Sep 26, 2024

Rajil1213 left a comment

Rajil1213 commented Sep 26, 2024

storopoli commented Sep 26, 2024

STR-376: create the machinery around polling and execution of bridge duties v2 #293

STR-376: create the machinery around polling and execution of bridge duties v2 #293

Conversation

storopoli commented Sep 18, 2024 • edited Loading

Description

Type of Change

Checklist

Related Issues

codecov bot commented Sep 18, 2024 • edited Loading

Codecov Report

storopoli commented Sep 24, 2024

Rajil1213 left a comment • edited Loading

Choose a reason for hiding this comment

delbonis left a comment

Choose a reason for hiding this comment

storopoli commented Sep 25, 2024

storopoli commented Sep 26, 2024

storopoli commented Sep 26, 2024

storopoli commented Sep 26, 2024 • edited Loading

Rajil1213 commented Sep 26, 2024 • edited Loading

storopoli commented Sep 26, 2024

delbonis left a comment

Choose a reason for hiding this comment

Rajil1213 left a comment • edited Loading

Choose a reason for hiding this comment

storopoli commented Sep 26, 2024

Rajil1213 left a comment

Choose a reason for hiding this comment

Rajil1213 commented Sep 26, 2024

storopoli commented Sep 26, 2024

storopoli commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

Rajil1213 left a comment •

edited

Loading

storopoli commented Sep 26, 2024 •

edited

Loading

Rajil1213 commented Sep 26, 2024 •

edited

Loading

Rajil1213 left a comment •

edited

Loading