Skip to content

Upgrade or replace bitcoind in integration tests #369

Closed
Closed
Upgrade or replace bitcoind in integration tests#369
Assignees
spacebear21
Labels
good first issueDoes NOT mean "an LLM can do this." Please discuss the constraints and intended approach.tech debt
Milestone
payjoin-1.0

Description

@spacebear21
spacebear21
opened on Oct 16, 2024

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

Activity

  1. DanGould commented on Oct 16, 2024

    @DanGould
    DanGould
    on Oct 16, 2024
    Member

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

  2. spacebear21
    added
    tech debt
    on Nov 20, 2024
  3. spacebear21
    added this to Payjoin Roadmap 📝on Nov 20, 2024

  4. DanGould commented on Dec 2, 2024

    @DanGould
    DanGould
    on Dec 2, 2024
    Member

    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
  5. DanGould
    added this to the payjoin-1.0 milestone on Jan 3, 2025

  6. spacebear21 commented on Jan 7, 2025

    @spacebear21
    spacebear21
    on Jan 7, 2025
    CollaboratorAuthor

    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.

  7. DanGould
    added
    blocked
    on Jan 13, 2025

  8. spacebear21 commented on Jan 13, 2025

    @spacebear21
    spacebear21
    on Jan 13, 2025
    CollaboratorAuthor

    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.

  9. spacebear21 commented on Jan 13, 2025

    @spacebear21
    spacebear21
    on Jan 13, 2025
    CollaboratorAuthor

    @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.

  10. arminsabouri commented on Feb 2, 2025

    @arminsabouri
    arminsabouri
    on Feb 2, 2025
    Contributor

    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

  11. spacebear21 commented on Feb 4, 2025

    @spacebear21
    spacebear21
    on Feb 4, 2025
    CollaboratorAuthor

    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.

  12. arminsabouri commented on Feb 5, 2025

    @arminsabouri
    arminsabouri
    on Feb 5, 2025
    Contributor

    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

  13. spacebear21 commented on Feb 5, 2025

    @spacebear21
    spacebear21
    on Feb 5, 2025
    CollaboratorAuthor

    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.

  14. DanGould commented on Feb 12, 2025

    @DanGould
    DanGould
    on Feb 12, 2025
    Member

    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

  15. arminsabouri commented on Feb 12, 2025

    @arminsabouri
    arminsabouri
    on Feb 12, 2025
    Contributor

    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.

    I can put it up but there isn't much code changed. To reproduce you just need to run latest bitcoin core locally and set BITCOIN_EXE envar

  16. spacebear21 commented on Feb 13, 2025

    @spacebear21
    spacebear21
    on Feb 13, 2025
    CollaboratorAuthor

    I replicated the taproot error with a local bitcoind_exe.

    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

    This is strange... the payjoin input calculations actually match the expected values. The reason it's off-by-one is because the original PSBT is paying 1 sat more than we should expect 🤔 I added some debug prints:

                let psbt = build_original_psbt(&sender, &uri)?;
            dbg!(psbt.clone().extract_tx_unchecked_fee_rate().weight());
            dbg!(psbt.fee().unwrap());
    [payjoin/tests/integration.rs:90:13] psbt.clone().extract_tx_unchecked_fee_rate().weight() = Weight(
    520,
)
[payjoin/tests/integration.rs:91:13] psbt.fee().unwrap() = 131 SAT

    520 WU is exactly 130 vB, and we explicitly specify a feerate of DEFAULT_MIN_RELAY_TX_FEE which is 1 sat/vB, so I would expect the original PSBT fee to be 130 SAT...

  17. spacebear21 commented on Feb 13, 2025

    @spacebear21
    spacebear21
    on Feb 13, 2025
    CollaboratorAuthor

    I replicated this with bitcoin-cli alone:

    // specify a witness_v1_taproot input for funding
❯ bitcoin-cli walletcreatefundedpsbt "[{\"txid\":\"11c016e8dafa55e6e4bd7d63305adda039ef73dec4ab3372901460fb06e1c15b\",\"vout\":0}]" "[{\"bcrt1qh23xwxs0wgf8en5a3ggjlka7j3hxgf5k9zzf07\":\"0.1\"}]" 0 "{\"fee_rate\":1}
"
{
  "psbt": "cHNidP8BAHECAAAAAVvB4Qb7YBSQcjOrxN5z7zmg3VowY3295OZV+troFsARAAAAAAD9////Ap51rCQAAAAAFgAU//MIH/Ov52GhitF3oYbKui2v7RKAlpgAAAAAABYAFLqiZxoPchJ8zp2KES/bvpRuZCaWAAAAAAABASuhDEUlAAAAACJRIF5iZ+CNJF90vVfeeezsUqj
+EWhIgTK8DjZ+OiNqWfarIRYHrlQVC0lAYEyy++eZf61dFqrLsqZYWXN7A5mQiZxn6hkAFUTY31YAAIABAACAAAAAgAAAAAAAAAAAARcgB65UFQtJQGBMsvvnmX+tXRaqy7KmWFlzewOZkImcZ+oAIgICx3VhryJH4wp2oCvFqdYUNSPMsZwXh7rQBwGT3gLHjzMYFUTY31QAAIABAACAA
AAAgAEAAAANAAAAACICAmH/qonCobPMcPU2yRgPlEgzpXraunEKV3h1w6EgLBM1GBVE2N9UAACAAQAAgAAAAIAAAAAAAgAAAAA=",
  "fee": 0.00000131,
  "changepos": 0
}

❯ bitcoin-cli walletprocesspsbt "cHNidP8BAHECAAAAAVvB4Qb7YBSQcjOrxN5z7zmg3VowY3295OZV+troFsARAAAAAAD9////Ap51rCQAAAAAFgAU//MIH/Ov52GhitF3oYbKui2v7RKAlpgAAAAAABYAFLqiZxoPchJ8zp2KES/bvpRuZCaWAAAAAAABASuhDEUlAAAAACJRI
F5iZ+CNJF90vVfeeezsUqj+EWhIgTK8DjZ+OiNqWfarIRYHrlQVC0lAYEyy++eZf61dFqrLsqZYWXN7A5mQiZxn6hkAFUTY31YAAIABAACAAAAAgAAAAAAAAAAAARcgB65UFQtJQGBMsvvnmX+tXRaqy7KmWFlzewOZkImcZ+oAIgICx3VhryJH4wp2oCvFqdYUNSPMsZwXh7rQBwGT3gL
HjzMYFUTY31QAAIABAACAAAAAgAEAAAANAAAAACICAmH/qonCobPMcPU2yRgPlEgzpXraunEKV3h1w6EgLBM1GBVE2N9UAACAAQAAgAAAAIAAAAAAAgAAAAA="
{
  "psbt": "cHNidP8BAHECAAAAAVvB4Qb7YBSQcjOrxN5z7zmg3VowY3295OZV+troFsARAAAAAAD9////Ap51rCQAAAAAFgAU//MIH/Ov52GhitF3oYbKui2v7RKAlpgAAAAAABYAFLqiZxoPchJ8zp2KES/bvpRuZCaWAAAAAAABASuhDEUlAAAAACJRIF5iZ+CNJF90vVfeeezsUqj
+EWhIgTK8DjZ+OiNqWfarAQhCAUD1af502czUuJZe3+pZgUblsvoRUW6pr43UZORmKGc0JExGaFjRLR6YuH+lAH0/WEiLUM3/XJ4cACYQ4g2xcoOkACICAsd1Ya8iR+MKdqArxanWFDUjzLGcF4e60AcBk94Cx48zGBVE2N9UAACAAQAAgAAAAIABAAAADQAAAAAiAgJh/6qJwqGzzHD1N
skYD5RIM6V62rpxCld4dcOhICwTNRgVRNjfVAAAgAEAAIAAAACAAAAAAAIAAAAA",
  "complete": true,
  "hex": "020000000001015bc1e106fb6014907233abc4de73ef39a0dd5a30637dbde4e655fadae816c0110000000000fdffffff029e75ac2400000000160014fff3081ff3afe761a18ad177a186caba2dafed128096980000000000160014baa2671a0f72127cce9d8a
112fdbbe946e6426960140f569fe74d9ccd4b8965edfea598146e5b2fa11516ea9af8dd464e466286734244c466858d12d1e98b87fa5007d3f58488b50cdff5c9e1c002610e20db17283a400000000"
}

❯ bitcoin-cli decoderawtransaction "020000000001015bc1e106fb6014907233abc4de73ef39a0dd5a30637dbde4e655fadae816c0110000000000fdffffff029e75ac2400000000160014fff3081ff3afe761a18ad177a186caba2dafed128096980000000000160014baa2671a0f72127cce9d8a112fdbbe946e6426960140f569fe74d9ccd4b8965edfea598146e5b2fa11516ea9af8dd464e466286734244c466858d12d1e98b87fa5007d3f58488b50cdff5c9e1c002610e20db17283a400000000"
{
  "txid": "dc09a5b7cdffd2307157e0685d54b47b5b4006a826689fe9cf47f15c3f0b4400",
  "hash": "3bee98c006f7be5e804a35d755e575672bec14356213dc5f13b1c2e397f5560c",
  "version": 2,
  "size": 181,
  "vsize": 130,
  "weight": 520,
  "locktime": 0,
  "vin": [
    {
      "txid": "11c016e8dafa55e6e4bd7d63305adda039ef73dec4ab3372901460fb06e1c15b",
      "vout": 0,
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "txinwitness": [
        "f569fe74d9ccd4b8965edfea598146e5b2fa11516ea9af8dd464e466286734244c466858d12d1e98b87fa5007d3f58488b50cdff5c9e1c002610e20db17283a4"
      ],
      "sequence": 4294967293
    }
  ],
  "vout": [
    {
      "value": 6.15282078,
      "n": 0,
      "scriptPubKey": {
        "asm": "0 fff3081ff3afe761a18ad177a186caba2dafed12",
        "desc": "addr(bcrt1qlless8ln4lnkrgv269m6rpk2hgk6lmgjvsfzpn)#hx6tljgx",
        "hex": "0014fff3081ff3afe761a18ad177a186caba2dafed12",
        "address": "bcrt1qlless8ln4lnkrgv269m6rpk2hgk6lmgjvsfzpn",
        "type": "witness_v0_keyhash"
      }
    },
    {
      "value": 0.10000000,
      "n": 1,
      "scriptPubKey": {
        "asm": "0 baa2671a0f72127cce9d8a112fdbbe946e642696",
        "desc": "addr(bcrt1qh23xwxs0wgf8en5a3ggjlka7j3hxgf5k9zzf07)#smy3ehgq",
        "hex": "0014baa2671a0f72127cce9d8a112fdbbe946e642696",
        "address": "bcrt1qh23xwxs0wgf8en5a3ggjlka7j3hxgf5k9zzf07",
        "type": "witness_v0_keyhash"
      }
    }
  ]
}

    The fee is 131 sats despite the vsize being 130vB and the fee_rate of 1sat/vB.

  18. DanGould commented on Aug 26, 2025

    @DanGould
    DanGould
    on Aug 26, 2025
    Member

    Seems like corepc-node and bitcoind-async-client would let us close this upgrade, test taproot, and close this issue for 1.0 stability.

    This is not really an API issue but it does plug a small hole in our tests.

  19. DanGould
    added
    good first issueDoes NOT mean "an LLM can do this." Please discuss the constraints and intended approach.
    and removed
    blocked
    on Aug 26, 2025
  20. DanGould
    assigned
    spacebear21
    on Sep 5, 2025

  21. arminsabouri commented on Sep 5, 2025

    @arminsabouri
    arminsabouri
    on Sep 5, 2025
    Contributor

    closed by #1041

  22. arminsabouri
    closed this as completedon Sep 5, 2025
  23. github-project-automation
    moved this to Done in Payjoin Roadmap 📝on Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

good first issueDoes NOT mean "an LLM can do this." Please discuss the constraints and intended approach.tech debt

Type

No type

Projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions