fix(wallet): add support for bip-431 rule 2#478
Conversation
- introduces a private method `is_truc` to check if a given tx version is TRUC (e.g `transaction::Version(3)`). - 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.
- updates the previously added to reproduce the issue, to assert for the expected behavior instead of using the workaround.
|
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #478 +/- ##
==========================================
+ Coverage 80.05% 80.64% +0.58%
==========================================
Files 24 24
Lines 5360 5378 +18
Branches 244 247 +3
==========================================
+ Hits 4291 4337 +46
+ Misses 990 962 -28
Partials 79 79
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 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.
| } | ||
|
|
||
| /// Check if the given [`transaction::Version`] is TRUC (Topologically Restricted Until | ||
| /// Confirmation)s. |
There was a problem hiding this comment.
Nit: (Topologically Restricted Until Confirmation)s has a stray s after the closing parenthesis.
| // if building TRUC; filter out all unconfirmed non-TRUC. | ||
| true => { | ||
| is_truc(ancestor_tx.version) | ||
| && local_output.chain_position.is_unconfirmed() |
There was a problem hiding this comment.
Nit: local_output.chain_position.is_unconfirmed() is always true here ?
The is_confirmed() check on line 2020 short-circuits the keep case via ||, so by the time the match runs, local_output is guaranteed unconfirmed.
Maybe same occurs on line 2031.
I think you can drop both checks.
| // only add to optional UTXOs those that follows BIP-431 (TRUC) specification. | ||
| // see https://github.com/bitcoin/bips/blob/master/bip-0431.mediawiki#specification | ||
| .filter(|local_output| { | ||
| local_output.chain_position.is_confirmed() |
There was a problem hiding this comment.
i think this logic can be simplified.
Maybe something like
if local_output.chain_position.is_confirmed() {
return true;
}
let Some(anscestor_tx) = self.tx_graph().get_tx(local_output.outpoint.txid)
else {
return true;
};
is_truc(version) && is_truc(anscestor_tx.version)There was a problem hiding this comment.
Thanks! We can't use the && because we're filtering out different things, though it looks much better like this:
if local_output.chain_position.is_confirmed() {
return true;
}
let Some(ancestor_tx) = self.tx_graph().get_tx(local_output.outpoint.txid)
else {
return true;
};
match is_truc(version) {
// if building TRUC; filter out all unconfirmed non-TRUC.
true => is_truc(ancestor_tx.version),
// if building non-TRUC; filter out all unconfirmed TRUC.
false => !is_truc(ancestor_tx.version),
}
4 participants
partially addresses #477
supersedes #442
Description
I've tried this another approach in order to add support for BIP-431 rule 2. I think this way is clearer and more functional than the previously attempted in #442.
It introduces a new private helper method
is_truc, it's useful as this validation will be necessary for other BIP-431 rules. Also, it adds a new filter intofilter_utxosin order to filter out unconfirmed UTXOs from non-TRUC/TRUC txs.It also introduces the usage of
bdk_testenv, as there's some validation that is checked under node policy level.Notes to the reviewers
Let me know what you think about this one.
Changelog notice
Checklists
All Submissions:
just pbefore pushingNew Features:
Bugfixes: