Fix policy condition calculation#932
Merged
notmandatory merged 1 commit intobitcoindevkit:masterfrom Apr 15, 2023
Merged
Conversation
When constructing the `Condition` struct we recursively call `get_condition` on all the items in a threshold and short-circuit if there's an error somewhere (for example, because the policy-path hasn't been provided for a specific threshold). This can cause issues when the user doesn't care about a subtree, because we still try to call `get_condition` on all the items and fail if something is missing, even if the specific subtree isn't selected and won't be used later on. This commit changes the logic so that we first filter only the `selected` items, and then unwrap the error using the question mark. If errors happened somewhere else they will be ignored, as it should.
afilini
force-pushed
the
fix/policy-path-ignored-subtrees
branch
from
539c56b to
ebd6103
Compare
notmandatory
approved these changes
Apr 3, 2023
Member
notmandatory
left a comment
notmandatory
left a comment
There was a problem hiding this comment.
ACK ebd6103
I did a simple manual test with and without the code fix to confirm test_create_tx_policy_path_ignored_subtree_with_csv fails without the fix.
I also confirmed the manual work around passes the same test without the code fix, by adding ("w5ay6ydv".to_string(),vec![0]) to the policy path.
16 tasks
3 tasks
notmandatory
added a commit
that referenced
this pull request
Jun 26, 2023
…ase/0.28 9cffaad Fix build errors (junderw) 3ccdb84 Fix policy condition calculation When constructing the `Condition` struct we recursively call `get_condition` on all the items in a threshold and short-circuit if there's an error somewhere (for example, because the policy-path hasn't been provided for a specific threshold). (Alekos Filini) Pull request description: Fixes #942 ### Description This backports "Fix policy condition calculation" (#932) onto release/0.28 ### Notes to the reviewers Currently kept the Author the same and the committer is myself. Let me know if this is not the desired outcome of the history. (I have made no modifications myself) ### Changelog notice Backported "Fix policy condition calculation" ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: notmandatory: ACK 9cffaad Tree-SHA512: c505bc9c8efd75cfa23db7a0e77f010f1758a1b6725661a1af1bcb40c6fd606a13e50ca005c7752fe28c3b7cd748c182fdb5870693e299adaf230adeef02517a
notmandatory
added a commit
to notmandatory/bdk
that referenced
this pull request
Aug 3, 2023
Summary This patch release backports (from the BDK 1.0 dev branch) a fix for a bug in the policy condition calculation and adds a new taproot single key descriptor template (BIP-86). The policy condition calculation bug can cause issues when a policy subtree fails due to missing info even if it's not selected when creating a new transaction, errors on unused policy paths are now ignored. Fixed - Backported bitcoindevkit#932 fix for policy condition calculation bitcoindevkit#1008 Added - Backported bitcoindevkit#840 taproot descriptor template (BIP-86) bitcoindevkit#1033
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When constructing the
Conditionstruct we recursively callget_conditionon all the items in a threshold and short-circuit if there's an error somewhere (for example, because the policy-path hasn't been provided for a specific threshold).This can cause issues when the user doesn't care about a subtree, because we still try to call
get_conditionon all the items and fail if something is missing, even if the specific subtree isn't selected and won't be used later on.This commit changes the logic so that we first filter only the
selecteditems, and then unwrap the error using the question mark. If errors happened somewhere else they will be ignored, as they should.Notes to the reviewers
I think it makes sense to backport this to
0.27: even though it's not a critical issue (and there's a workaround1 for the bug) it may be a while before the new1.0is released. I wouldn't do a release just for this, but I would just leave it there and maybe in a few weeks if there are other fixes to be backported to pre-1.0 they could all be released.Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
Bugfixes:
Footnotes
The workaround is to simply set the items in the policy tree even if they won't be used. For example, if the item causing troubles is a
thresh(1, ...)just set[0]in the policy path for that id. ↩