Skip to content

Replace custom RPC client with bitcoind-async-client#1063

Merged
arminsabouri merged 2 commits into
payjoin:masterfrom
arminsabouri:replace-custom-rpc
Sep 12, 2025
Merged

Replace custom RPC client with bitcoind-async-client#1063
arminsabouri merged 2 commits into
payjoin:masterfrom
arminsabouri:replace-custom-rpc

Conversation

@arminsabouri

Copy link
Copy Markdown
Contributor

Adding new methods to our custom rpc client is cumbersome and error prone. Using an off-the-shelf client allows us to just focus on payjoins.

Currently I am using a fork of bitcoind-async-client because upstream does not support cookie auth.


Unfortunately I am encountering a TLS issue on the directory after I imported the new lib. Need to debug further. Possible due to conflicting reqwest versions?

Error: HTTP request failed

Caused by:
    0: error sending request for url (https://localhost:44481/?v=1&additionalfeeoutputindex=0&maxadditionalfeecontribution=68&minfeerate=1)
    1: client error (SendRequest)
    2: connection error
    3: peer closed connection without sending TLS close_notify: https://docs.rs/rustls/latest/rustls/manual/_03_howto/index.html#unexpected-eof
Pull Request Checklist

Please confirm the following before requesting review:

AI disclosure: used a good bit of cursor autocomplete

@benalleng

Copy link
Copy Markdown
Collaborator

Ok, even with the updated lock files this error still occurs, but you should still update the lock files,

@arminsabouri

Copy link
Copy Markdown
Contributor Author

Ok, even with the updated lock files this error still occurs, but you should still update the lock files,

Done!

@arminsabouri arminsabouri self-assigned this Sep 12, 2025

@benalleng benalleng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are these errors happening perrhaps because of the difference in features enabled for the standalone async client?
https://github.com/arminsabouri/bitcoind-async-client/blob/c49d883285c3085f6d1fe36d41e554d1c0f2b752/Cargo.toml#L27-L33
Our error seems to be complaining in at least one case about parsing the version 2 which I assume in this case means http1 vs http2?

ERROR payjoin_directory: Error serving connection: hyper::Error(Parse(VersionH2))

@benalleng

benalleng commented Sep 12, 2025

Copy link
Copy Markdown
Collaborator

Possible due to conflicting reqwest versions?

I don't think its the versions themselves as bitcoind-async-client is only one minor version lower

@arminsabouri

Copy link
Copy Markdown
Contributor Author

Are these errors happening perrhaps because of the difference in features enabled for the standalone async client? https://github.com/arminsabouri/bitcoind-async-client/blob/c49d883285c3085f6d1fe36d41e554d1c0f2b752/Cargo.toml#L27-L33 Our error seems to be complaining in at least one case about parsing the version 2 which I assume in this case means http1 vs http2?

ERROR payjoin_directory: Error serving connection: hyper::Error(Parse(VersionH2))

I came to the same conclusion. When I remove "http2" feature flag I get past the error above. But run into a new one
"error: Failed to process response The payjoin endpoint is not available for now."

@coveralls

coveralls commented Sep 12, 2025

Copy link
Copy Markdown
Collaborator

Pull Request Test Coverage Report for Build 17681373194

Details

  • 32 of 32 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 85.783%

Totals Coverage Status
Change from base Build 17679832870: -0.2%
Covered Lines: 8055
Relevant Lines: 9390

💛 - Coveralls

Adding new methods to our custom rpc client is cumbersome and error
prone. Using an off-the-shelf client allows us to just focus on
payjoins.

Currently I am using a fork of bitcoind-async-client because upstream
does not support cookie auth.
`process_psbt` can and will sign for inputs the wallet owns.
@arminsabouri
arminsabouri marked this pull request as ready for review September 12, 2025 17:28

@benalleng benalleng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tack on 4b5ffbc

Will we need to try and get your bitcoind-async-client branch merged upstream? i see you have a few commits including removing the http2 feature

@arminsabouri

Copy link
Copy Markdown
Contributor Author

Tack on 4b5ffbc

Will we need to try and get your bitcoind-async-client branch merged upstream? i see you have a few commits including removing the http2 feature

Yes. Each commit will go in its own PR. So it may be a slow-ish process

@arminsabouri
arminsabouri merged commit df3a415 into payjoin:master Sep 12, 2025
10 checks passed
@arminsabouri
arminsabouri deleted the replace-custom-rpc branch September 12, 2025 20:35
Comment thread payjoin-cli/Cargo.toml
[dependencies]
anyhow = "1.0.99"
async-trait = "0.1.89"
bitcoind-async-client = {git = "https://github.com/arminsabouri/bitcoind-async-client", rev = "956ca693e4263c003eaa4fa938d909d24acac5fa"}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Guys you can use the 0.5.0 from crates.io: https://crates.io/crates/bitcoind-async-client

Cc @arminsabouri

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this include the changes Armin made to make this work for us? We will need a crates.io release in order to publish payjoin-cli on crates.io that reflects our payjoin-1.0rc.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, it does.

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.

5 participants