strata-client: http client instead of ws for full node

alpenlabs / strata

Rust implementation of the Strata protocol

https://docs.stratabtc.org

Apache License 2.0

23 stars 1 forks source link

strata-client: http client instead of ws for full node #391

Closed voidash closed 1 month ago

voidash commented 1 month ago

Description

Full node bug where if sequencer is down, full node also has to go down is because of ws client needing restart after connection drop. Simple Fix to use http client instead of ws client

Type of Change

[x] 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

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

Related Issues

codecov[bot] commented 1 month ago

Codecov Report

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

Please upload report for BASE (releases/0.1.0@ecc417e). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
bin/strata-client/src/main.rs	0.00%	2 Missing :warning:
bin/strata-client/src/rpc_client.rs	0.00%	2 Missing :warning:

@@                Coverage Diff                @@
##             releases/0.1.0     #391   +/-   ##
=================================================
  Coverage                  ?   57.32%           
=================================================
  Files                     ?      255           
  Lines                     ?    26957           
  Branches                  ?        0           
=================================================
  Hits                      ?    15454           
  Misses                    ?    11503           
  Partials                  ?        0

Files with missing lines	Coverage Δ
bin/strata-client/src/main.rs	`0.00% <0.00%> (ø)`
bin/strata-client/src/rpc_client.rs	`0.00% <0.00%> (ø)`

storopoli commented 1 month ago

Are we planning to have WS again in the future?

delbonis commented 1 month ago

This adds some overhead since we'd have to reestablish the connection on every request. A better solution would be to use a connection pool that checks if the connection is still healthy when it goes to hand it out each time. I've used the deadpool crate before, it's fairly easy to work with. Could abstract out the calls we need to make behind a trait.

delbonis commented 1 month ago

But ehhh maybe this is fine for now.

storopoli commented 1 month ago

This adds some overhead since we'd have to reestablish the connection on every request. A better solution would be to use a connection pool that checks if the connection is still healthy when it goes to hand it out each time. I've used the deadpool crate before, it's fairly easy to work with. Could abstract out the calls we need to make behind a trait.

https://alpenlabs.atlassian.net/browse/STR-518

storopoli commented 1 month ago

Approving because this is only merging into a release branch.

Oh man! Where's Waldo moment. Could have included something about that in the PR description. That was hard to notice.

storopoli commented 1 month ago

rebased