Upgrade or replace `bitcoind` in integration tests

[spacebear21]

Moved from #367 (comment)

Currently bitcoind is on 0.21 which doesn't support bech32m addresses and is preventing us from using taproot in integration tests.

My attempts to upgrade it have failed so far (bitcoind 0_22 fails because it creates legacy wallets by default, which don't support bech32m. bitcoind 0_23 and above fail because of some deadlock between tests running in parallel).

It looks like bitcoind might be on its way out so if upgrading doesn't work perhaps we should consider replacing this dependency with something else.

What closing this entails

• Update to use corepc-node so we can run a more recent version of bitcoin core
• Consider an upgrade to a new client like alpen's in order to support this new version
• test bech32m addresses in integration tests

[DanGould]

bitcoind-json-rpc-regtest appears to be the favored upgrade path in the rust-bitcoin issue linked in OP

[DanGould]

some functions we use from Client are missing. These are the ones we used and what's already implemented in bitcoind-json-rpc-regtest.

• generate_to_address
• get_new_address
• get_balances
• get_address_info
• list_unspent
• send_raw_transaction
• wallet_process_psbt
• finalize_psbt
• wallet_create_funded_psbt

[spacebear21]

bitcoind-json-rpc-regtest has been archived and development is now happening at https://github.com/rust-bitcoin/corepc. The corresponding crate is corepc-node.

[spacebear21]

I made a local fork of corepc that attempts to implement the missing functions for the v28 client version. I had partial success in writing those and getting it to compile and pushed my progress here (resorted to many hacks and shortcuts so it's not pretty...). I also have a working branch that modifies rust-payjoin to use this local corepc instead of bitcoind

However, some problems remain:

• When running all tests they just fail immediately due to a timeout error. Debug prints seem to indicate that tests hang on the "generating coins" initialization step and minreq kills the hung requests.
  • I ran into a similar issue when I attempted to upgrade bitcoind and mentioned it in the top comment. Perhaps they are related:

    bitcoind 0_23 and above fail because of some deadlock between tests running in parallel

• I can run tests one at a time e.g. cargo test v1_to_v1 but there are various incompatibilities that I "fixed" in my corepc branch with ugly hacks or simply commenting out unneeded fields.
  • walletcreatefundedpsbt expects amounts expressed in BTC but corepc passes satoshis.
  • Serialization errors like PSBTs not converted to string when calling PSBT RPCs.
  • Deserialization errors from RPC results like a required is_compressed field that is in fact optional in the RPC response.

Overall it seems corepc is not yet mature enough for us to make the switch at this time.

[spacebear21]

@nothingmuch mentioned a potential workaround for upgrading bitcoind which would involve using a Nix flake to initialize bitcoind and skipping the auto-download, calling the Nix binary directly.

[arminsabouri]

Currently bitcoind is on 0.21 which doesn't support bech32m addresses and is preventing us from using taproot in integration tests.

I set the BITCOIND_EXE to a locally compiled bitcoind bin. Generating bech32m addresses worked fine however the v1-v1 taproot test failed for a different reason.

thread 'integration::v1::v1_to_v1_taproot' panicked at payjoin/tests/integration.rs:126:13:
assertion `left == right` failed
  left: 4899999811 SAT
 right: 4899999812 SAT

Side note: I believe the bitcoind crate will skip autodownload if it detects bitcoind in the PATH.
Instead of ignoring certain tests we could enable them based on the version of bitcoind

[spacebear21]

Generating bech32m addresses worked fine however the v1-v1 taproot test failed for a different reason.

thread 'integration::v1::v1_to_v1_taproot' panicked at payjoin/tests/integration.rs:126:13:
assertion `left == right` failed
  left: 4899999811 SAT
 right: 4899999812 SAT

I wrote the v1-v1 taproot test but I guess I never confirmed that it actually works. Exact fee calculations are tricky to get right in tests. My first guess is that the value returned by predicted_tx_weight() in the test doesn't match the one returned by InputPair::expected_input_weight for taproot inputs.

[arminsabouri]

first guess is that the value returned by predicted_tx_weight() in the test doesn't match the one returned by InputPair::expected_input_weight for taproot inputs

Correct. When I pulled network_fees from the psbt directly the test passed

[spacebear21]

Makes sense, ideally we'd just use network_fees and drop the manual fee calculations in tests. IIRC that doesn't work for the non-taproot tests because ECSDA signatures lengths vary. expected_input_weight returns the worst-case weight so we can make conservative fee estimations.

If you push a working branch with your bitcoind changes I can take a look at why predicted_tx_weight isn't matching the expected weight.

[DanGould]

Noting this isn't really so much tech debt as it is maintenance. When the dependency was taken on it was state of the art, and the state of the art changed.

Not really tech debt to pay back, but regular maintenance