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

(WIP!) Enable BIP-127 support #34

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Ademan
Copy link
Contributor

@Ademan Ademan commented Dec 29, 2023

DRAFT PR

This change set enables key parts of BIP-127 such that I believe it can be described as properly implementing most of BIP-127. Notably absent is the protobuf based multi-proof serialization. This change set adds:

  • The BIP-127 PSBT_IN_POR_COMMITMENT key is added to proof PSBTs. This enables BIP-127 supporting hardware signers to operate properly.
  • Use single SHA-256 to hash the reserves commitment as described in BIP-127 (breaks compatibility with proofs generated before this change set)
  • Move verification to the Transaction structure, PSBT metadata is not required to verify a BIP-127 proof. Add a compatibility layer to PSBTs which retains API compatibility

This change set also includes changes from my cleanup branch which are not really relevant to discussion.

I'm creating this draft PR now to discuss (and hopefully concept ACK) switching to single SHA-256, and moving validation onto Transaction

In addition I'm seeking feedback on the TxOutSet trait I added. I think it's a useful abstraction, and it can facilitate "point in time" proofs, such as proving ownership of coins which are spent now, as well as out-of-wallet proofs in a seamless way. TxOutSet in particular added quite a few lines of code however.

@ulrichard
Copy link
Collaborator

ulrichard commented Jan 5, 2024

sorry for the late reply. I had a first quick look. But before diving in deeper, I want to get the CI working again.

Is "point in time" proof validation already working? This is a highly desired feature. When I looked at this initially, I had only Electrum as a backend, and I couldn't see a way to support it with the Electrum server API. What kind of backend is required for this to work?

ulrichard added a commit to ulrichard/bdk-cli that referenced this pull request Jan 5, 2024
Significant refactor to support these tests.

* Add a `TxOutSet` trait for retrieving live Utxos
* Implement `TxOutSet` for `&bdk::Wallet`
* Implement `TxOutSet` for `&BTreeMap<OutPoint, TxOut>`
* Add `WalletAtHeight` struct to support `max_block_height` use case
  previously supported
* Implement `TxOutSet` for `WalletAtHeight`
* Eliminate `ProofError::NonSpendableInput` and use
  `ProofError::OutpointNotFound` instead since `NonSpendableInput`
  interprets the actual fact that the outpoint is not found
* Eliminate dead `ProofError::NeitherWitnessNorLegacy` variant
* Add `ProofError::OutpointLookupError` variant for cases where the
  underlying `TxOutSet` generated an error. (Such as an RPC error)
* Refactor other code to use these new primitives
* Remove free `verify_proof` function
@Ademan
Copy link
Contributor Author

Ademan commented Jan 6, 2024

097c7a9 supports point in time validation using the esplora backend. I added some rudimentary tests and I think it's mostly correct but there's a couple of edge cases I need to think a bit about.

Steve also said he wants bdk tests to use electrsd, which this doesn't (I'm just using the public mempool.space signet endpoint with esplora_client and a few txids I hand picked). Not sure if that's a blocker for merging, but this all should still be considered a WIP at the moment anyway.

I want to add foreign utxo support using electrum and bitcoind rpc as well, I don't believe they can support full point-in-time though. I believe electrum can at least support a maximum block height (by which a user can establish a minimum number of confirmations)

@Ademan
Copy link
Contributor Author

Ademan commented Jan 7, 2024

RE: using a public esplora instance in the tests, I actually had a test failure due to this, so the unreliability actually matters.

ulrichard added a commit to ulrichard/bdk-cli that referenced this pull request Jan 8, 2024
@ulrichard
Copy link
Collaborator

There are already some regtest tests with electrsd in bdk-reserves. If I remember correctly, we would just have to add the esplora_a33e97e1 feature to be able to test using an esplora backend.

@ulrichard
Copy link
Collaborator

ulrichard commented Jan 8, 2024

I added a point in time test with electrsd / esplora but it fails.
Curious to learn if I misunderstood something, implemented it wrong, or there is a problem left with the implementation.

@Ademan
Copy link
Contributor Author

Ademan commented Jan 8, 2024

yeah WalletAtHeight cant support point in time regardless of backend (it's limited by the bdk database).

You need the txout_set::PointInTimeTxOutSet trait which is implemented for the esplora blocking client, take a look at txout_set::test::test_spent_at_later_height it can be adapted to use electrsd.

i was actually looking at it this morning too (but I need to get to my day job shortly)

@Ademan
Copy link
Contributor Author

Ademan commented Jan 8, 2024

As far as I know this is what's possible:

bdk::Wallet electrum blocking esplora blocking bitcoin rpc
Current UTXO Set 🟢 🟢 🟢 🟢
UTXOs currently unspent, confirmed before a certain height 🟢 🟢 🟢 🟢
Point in time 🔴 🔴 🟢 🔴

While we're talking about it, I'm trying to come up with a concise term for the second case "UTXOs currently unspent, but confirmed before a certain height"

@Ademan
Copy link
Contributor Author

Ademan commented Jan 8, 2024

Just to be precise, we have 2 (really 3) sets of transaction outputs

  1. utxo set at tip utxos_tip
  2. utxo set at X block height utxos_X
  3. utxo set in the future if all utxos currently in the mempool (but nothing else) are confirmed utxos_mempool
relevant set bdk::Wallet electrum blocking esplora blocking bitcoin rpc
Current UTXO Set utxos_tip 🟢 🟢 🟢 🟢
UTXOs currently unspent, confirmed before a certain height utxos_tip ∩ utxos_X 🟢 🟢 🟢 🟢
Point in time utxos_X 🔴 🔴 🟢 🔴
UTXOS currently unspent in the tip block AND the mempool, confirmed before a certain height utxos_tip ∩ utxos_X ∩ utxos_mempool 🔴 ❔ 🟢 ❔ 🟢 ❔ 🟢 ❔

I believe some (all?) backends can support utxos_tip ∩ utxos_X ∩ utxos_mempool if that's desirable

@ulrichard
Copy link
Collaborator

ulrichard commented Jan 9, 2024

Very cool! I wanted to be able to verify proofs containing UTXOs that were spent after generating the proof since forever. Super glad that this can now be done.
From the table above, for me only the second and third row are really relevant. But it's good to have all the options.

@Ademan
Copy link
Contributor Author

Ademan commented Jan 13, 2024

I added support for electrum utxos_tip ∩ utxos_X queries as well as a test. I actually was told that it might be possible to do full point-in-time for electrum, too, but I think I'm going to add bitcoin rpc support first, and probably wait on point-in-time on electrum. I kind of want to rework the API here, but I also don't want to hold up the PR for that.

@Ademan
Copy link
Contributor Author

Ademan commented Jan 14, 2024

I just added very WIP bitcoincore_rpc support. Notably, the confirmed_by test fails, but I actually suspect there's an issue in either bitcoincore_rpc or bdk's usage of it, you can see that old_balance.confirmed (which comes from bdk::wallet::Wallet) is consistent between esplora and electrum backends, but is conspicuously different with the bitcoincore_rpc backend. I will have to investigate further another day...

Once that's resolved, I think that will be everything I want to do with this PR, and I'll change from a draft to a "real" PR. (I imagine review will reveal some issues though)

$ cargo test -F rpc,use-esplora-blocking,electrum --test txout_set electrum -- --nocapture
    Finished test [unoptimized + debuginfo] target(s) in 0.06s
warning: the following packages contain code that will be rejected by a future version of Rust: rstest v0.11.0, traitobject v0.1.0
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 1`
     Running tests/txout_set.rs (target/debug/deps/txout_set-b53b110cc918c350)

running 1 test
[tests/txout_set.rs:158] old_balance.confirmed = 4999999718
test test_electrum_confirmed_by ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out; finished in 21.65s

$ cargo test -F rpc,use-esplora-blocking,electrum --test txout_set esplora -- --nocapture
    Finished test [unoptimized + debuginfo] target(s) in 0.06s
warning: the following packages contain code that will be rejected by a future version of Rust: rstest v0.11.0, traitobject v0.1.0
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 1`
     Running tests/txout_set.rs (target/debug/deps/txout_set-b53b110cc918c350)

running 1 test
[tests/txout_set.rs:83] old_balance.confirmed = 4999999718
test test_esplora_point_in_time ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out; finished in 21.51s

$ cargo test -F rpc,use-esplora-blocking,electrum --test txout_set bitcoincore -- --nocapture
    Finished test [unoptimized + debuginfo] target(s) in 0.06s
warning: the following packages contain code that will be rejected by a future version of Rust: rstest v0.11.0, traitobject v0.1.0
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 1`
     Running tests/txout_set.rs (target/debug/deps/txout_set-b53b110cc918c350)

running 1 test
[tests/txout_set.rs:158] old_balance.confirmed = 49999999718
thread 'test_bitcoincore_rpc_confirmed_by' panicked at 'assertion failed: `(left == right)`
  left: `4999999718`,
 right: `49999999718`', tests/txout_set.rs:158:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test test_bitcoincore_rpc_confirmed_by ... FAILED

failures:

failures:
    test_bitcoincore_rpc_confirmed_by

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2 filtered out; finished in 16.54s

error: test failed, to rerun pass `--test txout_set`

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.

2 participants