fix(wallet): add support for bip-431 rule 2 - #478
Conversation
|
cc @yan-pi as he already reviewed the other PR and has a reproducible test using bdk-cli too. |
|
I noticed in CI I need to fix some bdk_testenv/electrs/esplora related issues. However, the fix in bc05f88 is ready for review/discussion. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #478 +/- ##
==========================================
+ Coverage 81.15% 81.22% +0.06%
==========================================
Files 24 24
Lines 5552 5565 +13
Branches 247 248 +1
==========================================
+ Hits 4506 4520 +14
+ Misses 970 969 -1
Partials 76 76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
tACK 1d32117.
IMO The is_truc helper is more functional and reads cleaner than the XNOR in #442, and definitely is a good foundation for the rest of #477.
Tested it locally by checking out test/unconfirmed-truc-txs and rebuilding the rust-repro binaries from https://github.com/yan-pi/bdk-pr442-repro against the PR via [dependencies] bdk_wallet = { path = "../../bdk_wallet" }:
The inverse direction of Rule 2: the non-v3 unconfirmed UTXO was filtered correclty.
One concern not flagged inline: the integration test is great, but maybe the unit-level coverage from #442 is also worth keeping?
Plus two minor nits and typos.
b8d9ae1 to
85143c2
Compare
47bba04 to
d6fca63
Compare
d6fca63 to
6145afc
Compare
ValuedMammal
left a comment
There was a problem hiding this comment.
#442 added docs explaining the manual selection caveat #442 (comment), but that seems to be missing from this branch.
- add new step in `filter_utxos` to validate BIP-431 rule 2, which validates the proper usage of unconfirmed TRUC/non-TRUC ancestor in a TRUC/non-TRUC tx. - update docs explaining the manual selection caveat when building non-TRUC/TRUC txs.
- introduce `test_create_and_spend_from_tx` to exercise BIP-431 rule-2 filtering in-memory, not relying on `bdk_testenv`/`bdk_electrum`. NOTE: I asked Claude to remove the `bdk_testenv` dependency, keeping the test behavior in-memory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6145afc to
7982b6e
Compare
@ValuedMammal Thanks! I updated the code following some of the suggestions, let me know if there's anything else to address. |
ValuedMammal
left a comment
There was a problem hiding this comment.
ACK 7982b6e; ran the tests locally
There was a problem hiding this comment.
ACK 7982b6e
This solution is inline with the current approach for UTXO filtering, so I agree with it, however, the number of constraints we impose on the local utxo seems to be always growing.
I usually come back to the idea that this filtering should be done in a detachable pipeline, with customizable filters rather than manually adding new ones each time we need.
In that case I would prefer to create a wrapper around txouts injecting all the information we need to filter them later, or providing an oracle that can be called with custom made closure to apply these filters.
Has anyone have any ideas about this?
| assert!( | ||
| tx_c.input | ||
| .iter() | ||
| .all(|txin| txin.previous_output.txid.ne(&txid_b)), | ||
| "SHOULD NOT try to spend an unconfirmed non-TRUC output in a TRUC tx!" | ||
| ); |
There was a problem hiding this comment.
Why not checking we are funding the TRUC tx with another TRUC tx?
There was a problem hiding this comment.
Yes, that would also work.
In the new create_psbt and PsbtParams flow, you could have a custom filter instead of this always-growing one, doesn't that solve this issue ? Let me know if I'm missing your point here. |
partially addresses #477
supersedes #442
Description
Adds support for BIP-431 (TRUC) rule 2 in filter_utxos. When building a transaction, unconfirmed UTXOs are filtered based on version compatibility with the transaction being constructed:
The filter is implemented inline in filter_utxos, which now takes a
versionparameter passed down from the transaction builder. The version compatibility check uses an equality comparison on the== Version(3)predicate, ensuring the unconfirmed output's transaction version matches the version class (TRUC or non-TRUC) of the transaction being built.Note that manually selected UTXOs bypass this filter — callers are responsible for ensuring any manually selected unconfirmed UTXO is compatible with the transaction version being built.
Changelog notice
Checklists
All Submissions:
just pbefore pushingNew Features:
Bugfixes: