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

Support wasm #91

Merged
merged 28 commits into from
Mar 14, 2025
Merged

Support wasm #91

merged 28 commits into from
Mar 14, 2025

Conversation

danielgranhao
Copy link
Contributor

@danielgranhao danielgranhao commented Feb 26, 2025

Resolves #90.

This PR:

  • Changes the http client from ureq to async reqwest
  • Switches to a different secp256k1-zkp fork that circumvents wasm compatibility issues
  • Changes the websocket crate from tungstenite to tokio-tungstenite-wasm - as the name indicates, this crate uses tokio-tungstenite for native builds
  • Adds esplora as a supported chain data backend. Electrum is still supported. Both can be enabled using features, allowing clients to switch backends at runtime (which may be useful for redundancy). Only esplora is set as a default so that wasm is supported by default.
  • Allows reusing esplora and electrum clients instead of having to create new ones on every method.

There are still some TODOs, but I would appreciate some initial feedback.

TODO:

  • Testing (adding new tests for esplora + run ignored tests)
    • I've made the testnet and mainnet integration tests run against regtest and added regtest as a submodule. These tests now run in CI both on native and wasm builds.
  • Try to upstream changes to secp256k1-zkp
    • I've rebased sanket's PR (on which we depended already) on top of the secp256k1-zkp main branch (to pull in a WASM fix) and added an additional workaround/fix on top. I won't be trying to upstream the changes because his PR has been dropped in favor of adding musig2 support on the base secp256k1 crate (WIP here). Hopefully soon a fork won't be needed any longer.

let electrum_client = network_config.clone().build_client()?;
pub async fn fetch_utxo<LC: LiquidClient, N: LiquidNetworkConfig<LC>>(
&self,
network_config: &N,

Choose a reason for hiding this comment

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

Passing a config forces this function to call build_client (and create a new client) which prevents from the caller to reuse a client implementation. In electrum we have seen it has a big performance penalty.
I know this was the implementation before this PR but perhaps if we are going for such a big change we can improve it?
I commented on this method as an example but this pattern repeats itself.

Copy link
Contributor Author

@danielgranhao danielgranhao Mar 7, 2025

Choose a reason for hiding this comment

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

Thanks @roeierez. I agree. I've made the methods take a client instead of the config in a6d9d1c

@danielgranhao danielgranhao force-pushed the support-wasm branch 11 times, most recently from 0366f4c to 8ce332e Compare March 6, 2025 17:10
@michael1011 michael1011 requested review from i5hi and roeierez and removed request for roeierez March 10, 2025 12:12
Copy link

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looks good! Added two NITs.

@danielgranhao
Copy link
Contributor Author

There is an issue with the websocket client. tokio-tungstenite-wasm doesn't expose websocket pings because they are not available in the javascript API. IIUC some browsers handle it automatically. This wrapper crate tries to keep the API consistent between native and WASM, so it also hides the ping-pong API for native builds.

I see two main solution paths:

  • Fork the tokio-tungstenite-wasm in order to expose the ping API on native (eventually try to upstream it). Not sure how viable this is. Also, I'm not sure how reliable the auto-keep-alive implementation of the browsers is.
  • Implement a keep-alive ping-pong on the app level. This could be as simple as the text "ping" and "pong" but it requires adapting the boltz backend accordingly cc @michael1011. This is the safest option IMO and the overhead should be insignificant.

@michael1011
Copy link
Collaborator

There is an issue with the websocket client. tokio-tungstenite-wasm doesn't expose websocket pings because they are not available in the javascript API. IIUC some browsers handle it automatically. This wrapper crate tries to keep the API consistent between native and WASM, so it also hides the ping-pong API for native builds.

I see two main solution paths:

  • Fork the tokio-tungstenite-wasm in order to expose the ping API on native (eventually try to upstream it). Not sure how viable this is. Also, I'm not sure how reliable the auto-keep-alive implementation of the browsers is.
  • Implement a keep-alive ping-pong on the app level. This could be as simple as the text "ping" and "pong" but it requires adapting the boltz backend accordingly cc @michael1011. This is the safest option IMO and the overhead should be insignificant.

I'd prefer a ping/pong JSON message in the application layer over having to fork

@danielgranhao
Copy link
Contributor Author

I opened a PR on boltz-backend to add these ping/pong messages.

@danielgranhao
Copy link
Contributor Author

In f48f3d5 I've adjusted the ws message structs to align with https://github.com/BoltzExchange/boltz-backend, including the newly added ping/pong messages. Added also regtest test for the pinging.

This is ready for another review round. cc @michael1011 @i5hi @roeierez

Copy link

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

LGTM

@i5hi
Copy link
Collaborator

i5hi commented Mar 12, 2025

Sorry about the delay in reviewing; closing up another release now and will take a look at this and test it tomorrow morning.

@i5hi
Copy link
Collaborator

i5hi commented Mar 14, 2025

Getting this error when I run the tests:

---- btc_submarine_refund stdout ----

thread 'btc_submarine_refund' panicked at tests/test_framework/mod.rs:46:14:
called `Result::unwrap()` on an `Err` value: JsonRpc(Transport(SocketError(Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" })))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- btc_reverse_claim_relative_fee stdout ----

Similar error for :
test btc_submarine_refund ... FAILED
test btc_reverse_claim_relative_fee ... FAILED
test btc_reverse_claim_size ... FAILED
test btc_reverse_claim ... FAILED
test btc_submarine_refund_size ... FAILED
test btc_submarine_refund_relative_fee ... FAILED

@danielgranhao
Copy link
Contributor Author

Getting this error when I run the tests:

---- btc_submarine_refund stdout ----

thread 'btc_submarine_refund' panicked at tests/test_framework/mod.rs:46:14:
called `Result::unwrap()` on an `Err` value: JsonRpc(Transport(SocketError(Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" })))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- btc_reverse_claim_relative_fee stdout ----

Similar error for : test btc_submarine_refund ... FAILED test btc_reverse_claim_relative_fee ... FAILED test btc_reverse_claim_size ... FAILED test btc_reverse_claim ... FAILED test btc_submarine_refund_size ... FAILED test btc_submarine_refund_relative_fee ... FAILED

@i5hi Could you please try to run the same tests on trunk (cargo test)? Those tests also fail on my machine (I have MacOS, not sure if you do too), but run fine in CI. Because for me, they already fail on trunk, I intended to leave out of this PR any potential fix. My assumption is that there is some issue with the bitcoind crate on MacOS.

I've opened an issue #93 for this.

@i5hi
Copy link
Collaborator

i5hi commented Mar 14, 2025

Getting this error when I run the tests:

---- btc_submarine_refund stdout ----

thread 'btc_submarine_refund' panicked at tests/test_framework/mod.rs:46:14:
called `Result::unwrap()` on an `Err` value: JsonRpc(Transport(SocketError(Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" })))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- btc_reverse_claim_relative_fee stdout ----

Similar error for : test btc_submarine_refund ... FAILED test btc_reverse_claim_relative_fee ... FAILED test btc_reverse_claim_size ... FAILED test btc_reverse_claim ... FAILED test btc_submarine_refund_size ... FAILED test btc_submarine_refund_relative_fee ... FAILED

@i5hi Could you please try to run the same tests on trunk (cargo test)? Those tests also fail on my machine (I have MacOS, not sure if you do too), but run fine in CI. Because for me, they already fail on trunk, I intended to leave out of this PR any potential fix. My assumption is that there is some issue with the bitcoind crate on MacOS.

I've opened an issue #93 for this.

Yes, I had tested it on a mac. Tested on Linux and its all good.

@i5hi
Copy link
Collaborator

i5hi commented Mar 14, 2025

LGTM.

Thanks @danielgranhao !

@i5hi i5hi merged commit f78e159 into SatoshiPortal:trunk Mar 14, 2025
6 checks passed
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.

Support WASM
5 participants