bdk icon indicating copy to clipboard operation
bdk copied to clipboard

deps(bdk_testenv): bump `electrsd` to `0.31.0`

Open oleonardolima opened this issue 11 months ago • 15 comments

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

  • [ ] I've signed all my commits
  • [ ] I followed the contribution guidelines
  • [ ] I ran cargo fmt and cargo clippy before committing

New Features:

  • [ ] I've added tests for the new feature
  • [ ] I've added docs for the new feature

Bugfixes:

  • [ ] This pull request breaks the existing API
  • [ ] I've added tests to reproduce the issue which are now passing
  • [ ] I'm linking the issue being fixed by this PR

oleonardolima avatar Feb 06 '25 20:02 oleonardolima

It looks like I'll need to open a PR on corepc-client to add get-block-template RPC, then I'll get back to this one :(

I'll do the same in rust-electrum-client for the tests, but I'll do it to 0.30.x instead to prevent this issue from blocking the PR there.

oleonardolima avatar Feb 07 '25 14:02 oleonardolima

@oleonardolima I noticed that the getblocktemplate RPC is implemented in corepc-client.

ValuedMammal avatar Jul 21 '25 23:07 ValuedMammal

@oleonardolima I noticed that the getblocktemplate RPC is implemented in corepc-client.

Yes, it was added a few releases after I started this PR, but IIRC some other methods were missing too. I'll check that again.

oleonardolima avatar Jul 22 '25 13:07 oleonardolima

Yes, it was added a few releases after I started this PR, but IIRC some other methods were missing too. I'll check that again.

Seems to be lacking good support for get_raw_mempool_verbose, at least for versions of Bitcoin Core later than 0.17.

ValuedMammal avatar Jul 23 '25 21:07 ValuedMammal

Once #1988 lands, we won't be relying on get_raw_mempool_verbose; instead, we will use get_raw_mempool. It should unblock this one.

oleonardolima avatar Jul 24 '25 15:07 oleonardolima

Once #1988 lands, we won't be relying on get_raw_mempool_verbose; instead, we will use get_raw_mempool. It should unblock this one.

It appears that other methods are missing in the corepc-client at electrsd 0.34. They're released on corepc-node 0.8, which will be included in the next release of electrsd. Will check if there are alternative methods for those.

oleonardolima avatar Aug 05 '25 02:08 oleonardolima

It appears that other methods are missing in the corepc-client at electrsd 0.34. They're released on corepc-node 0.8, which will be included in the next release of electrsd. Will check if there are alternative methods for those.

As of the latest v0.35 release, it appears that all the necessary methods are implemented in corepc-{node|client}. I went ahead and updated the bdk-testenv and bdk-electrum/esplora crates.

It requires a more significant refactor and changes for the bdk_bitcoind_rpc, as the Emitter relies on the bitcoincore-rpc ' RpcApi' trait. I can cover it all in a single PR, but would like to know what you guys think as well.

cc @ValuedMammal @LagginTimes

oleonardolima avatar Aug 08 '25 20:08 oleonardolima

Replaying a comment from discord

I also noticed that updating electrsd means potentially changing the Emitter to use corepc Client because of how it relates to testenv, etc. We have to make sure switching clients doesn't negatively impact behavior, performance and stuff

ValuedMammal avatar Aug 12 '25 14:08 ValuedMammal

We may be able to unblock this by having a way to create a new RPC client using the node credentials of a TestEnv instance but independent of the actual test client that is used by electrsd. oleonardolima/bdk#3

ValuedMammal avatar Nov 22 '25 04:11 ValuedMammal

We may be able to unblock this by having a way to create a new RPC client using the node credentials of a TestEnv instance but independent of the actual test client that is used by electrsd. oleonardolima#3

Cool, I'll take a look at your PR this morning.

oleonardolima avatar Nov 22 '25 04:11 oleonardolima

We may be able to unblock this by having a way to create a new RPC client using the node credentials of a TestEnv instance but independent of the actual test client that is used by electrsd. oleonardolima#3

Alright, but if I got it right, the idea would to still rely on bitcoincore_rpc API for bdk_bitcoind_rpc tests (which I think makes sense), right ? And when we land bdk_bitcoind_client that wouldn't an issue anymore, right

oleonardolima avatar Nov 24 '25 18:11 oleonardolima

re-posting from https://github.com/oleonardolima/bdk/pull/3#issuecomment-3572074616

Thanks for the suggestion, I'll cherry-pick this one and update the remaining tests.

oleonardolima avatar Nov 24 '25 18:11 oleonardolima

Also, it looks like the latest one is v0.36.1, I'll give it a try on that one too.

oleonardolima avatar Nov 24 '25 23:11 oleonardolima

Alright, but if I got it right, the idea would to still rely on bitcoincore_rpc API for bdk_bitcoind_rpc tests (which I think makes sense), right ? And when we land bdk_bitcoind_client that wouldn't an issue anymore, right

The approach I had in mind is to bump electrsd to the latest version in bdk_testenv which might entail updating some methods in order to benefit from all new corepc types. In bdk_bitcoind_rpc we can start using the TestEnv client for tests, and this ClientExt trait or whatever we're calling it can be used to create a client just for the Emitter with no other code changes needed. When bdk_bitcoind_client is available we'll update the Emitter to use that and remove bitcoincore_rpc. Don't know if I've fully answered the question.

ValuedMammal avatar Nov 25 '25 01:11 ValuedMammal