Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STR-885 Fix Bridge Client Bug and add functional Test #564

Merged
merged 18 commits into from
Jan 14, 2025

Conversation

voidash
Copy link
Contributor

@voidash voidash commented Dec 26, 2024

Description

The nature of websocket client is that it requires restart once the connection drops. Due to this bridge-client stops if the connection to strata-client drops which is not the desired and must be handled. This PR fixes the bug by using managed connection pool calleddeadpool.

Note

The functional test deliberately contains duplicated code which can be fixed after STR-734 is merged. STR-734 contains BridgeTestBase class that will be utilized by this functional test after rebase. I opened this PR to discuss the bug fix part

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
  • New or updated tests
  • Dependency Update

Notes to Reviewers

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-885

@voidash voidash requested review from a team as code owners December 26, 2024 15:27
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

The way errors are reasoned about here needs to be improved, it's written assuming they can't really happen.

crates/bridge-exec/src/handler.rs Outdated Show resolved Hide resolved
crates/bridge-exec/src/handler.rs Outdated Show resolved Hide resolved
crates/bridge-exec/src/ws_client.rs Outdated Show resolved Hide resolved
crates/bridge-exec/src/ws_client.rs Outdated Show resolved Hide resolved
functional-tests/fn_bridge_deposit_seq_unreliable.py Outdated Show resolved Hide resolved
crates/bridge-exec/src/handler.rs Outdated Show resolved Hide resolved
bin/bridge-client/src/modes/operator/task_manager.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 2, 2025

Codecov Report

Attention: Patch coverage is 0% with 113 lines in your changes missing coverage. Please review.

Project coverage is 56.55%. Comparing base (0864d5f) to head (083c1e5).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...n/bridge-client/src/modes/operator/task_manager.rs 0.00% 56 Missing ⚠️
crates/bridge-exec/src/ws_client.rs 0.00% 26 Missing ⚠️
bin/bridge-client/src/modes/operator/bootstrap.rs 0.00% 20 Missing ⚠️
crates/bridge-exec/src/handler.rs 0.00% 11 Missing ⚠️
@@            Coverage Diff             @@
##             main     #564      +/-   ##
==========================================
- Coverage   56.78%   56.55%   -0.24%     
==========================================
  Files         308      309       +1     
  Lines       32397    32460      +63     
==========================================
- Hits        18398    18359      -39     
- Misses      13999    14101     +102     
Files with missing lines Coverage Δ
bin/bridge-client/src/args.rs 0.00% <ø> (ø)
crates/bridge-exec/src/handler.rs 0.00% <0.00%> (ø)
bin/bridge-client/src/modes/operator/bootstrap.rs 0.00% <0.00%> (ø)
crates/bridge-exec/src/ws_client.rs 0.00% <0.00%> (ø)
...n/bridge-client/src/modes/operator/task_manager.rs 0.00% <0.00%> (ø)

... and 5 files with indirect coverage changes

bin/bridge-client/src/constants.rs Outdated Show resolved Hide resolved
bin/bridge-client/src/modes/operator/bootstrap.rs Outdated Show resolved Hide resolved
bin/bridge-client/src/modes/operator/task_manager.rs Outdated Show resolved Hide resolved
bin/bridge-client/src/modes/operator/task_manager.rs Outdated Show resolved Hide resolved
crates/bridge-exec/src/errors.rs Outdated Show resolved Hide resolved
functional-tests/fn_bridge_deposit_seq_unreliable.py Outdated Show resolved Hide resolved
functional-tests/fn_bridge_deposit_seq_unreliable.py Outdated Show resolved Hide resolved
bin/bridge-client/src/args.rs Outdated Show resolved Hide resolved
bin/bridge-client/src/errors.rs Outdated Show resolved Hide resolved
bin/bridge-client/src/errors.rs Outdated Show resolved Hide resolved
@voidash voidash force-pushed the STR-730-DEPOSIT-SEQUENCER-UNRELIABLE branch from efe5cd7 to 92849d0 Compare January 5, 2025 14:30
bin/bridge-client/src/args.rs Outdated Show resolved Hide resolved
bin/bridge-client/src/errors.rs Outdated Show resolved Hide resolved
crates/bridge-exec/src/errors.rs Outdated Show resolved Hide resolved
crates/bridge-exec/src/ws_client.rs Show resolved Hide resolved
functional-tests/fn_bridge_deposit_seq_unreliable.py Outdated Show resolved Hide resolved
@voidash voidash force-pushed the STR-730-DEPOSIT-SEQUENCER-UNRELIABLE branch from bfce305 to 91e61df Compare January 6, 2025 18:35
storopoli
storopoli previously approved these changes Jan 6, 2025
Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK 91e61df

@delbonis
Copy link
Contributor

delbonis commented Jan 6, 2025

I think this needs to be rebased and tweaked now that @evgenyzdanovich's PR has been merged?

Copy link
Contributor

github-actions bot commented Jan 6, 2025

Commit: b4f04b2

SP1 Performance Test Results

program cycles success
BTC_BLOCKSPACE 7,239,474
EL_BLOCK 100,495
CL_BLOCK 55,340
L1_BATCH 12,460,065
L2_BATCH 5,448
CHECKPOINT 15,322

@voidash voidash force-pushed the STR-730-DEPOSIT-SEQUENCER-UNRELIABLE branch 2 times, most recently from 9a5c2ee to dd547e5 Compare January 8, 2025 06:11
@john-light john-light changed the title STR-809 Fix Bridge Client Bug and add functional Test STR-887 Fix Bridge Client Bug and add functional Test Jan 8, 2025
@john-light john-light changed the title STR-887 Fix Bridge Client Bug and add functional Test STR-885 Fix Bridge Client Bug and add functional Test Jan 8, 2025
@voidash voidash force-pushed the STR-730-DEPOSIT-SEQUENCER-UNRELIABLE branch from dd547e5 to a7bff9b Compare January 14, 2025 12:33
@voidash voidash requested review from storopoli and delbonis January 14, 2025 15:05
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

I think this is good now.

Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK 083c1e5

@storopoli storopoli added this pull request to the merge queue Jan 14, 2025
Merged via the queue into main with commit f96448d Jan 14, 2025
17 of 18 checks passed
@storopoli storopoli deleted the STR-730-DEPOSIT-SEQUENCER-UNRELIABLE branch January 14, 2025 22:41
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.

3 participants