frost/bdk
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge bitcoindevkit/bdk#932: Fix policy condition calculation

Browse Source 
ebd6103e6597e0a66470541d2fcd25709ea77866 Fix policy condition calculation (Alekos Filini)

Pull request description:

  ### Description

  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 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 workaround[^1] for the bug) it may be a while before the new `1.0` is 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

  - Fixed a bug in the 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

  #### New Features:

  * [ ] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

  [^1]: 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.

ACKs for top commit:
  notmandatory:
    ACK ebd6103e6597e0a66470541d2fcd25709ea77866

Tree-SHA512: 1e0c6140e47ead3a518656cab660932b40f9e67425ce144f072267c30fd5cae5923ba943d9682af050c8f314756c522260abb68b104b9eeaae597319612984db
This commit is contained in:
Steve Myers 2023-04-14 22:46:02 -05:00
commit 5e026cfd03
No known key found for this signature in database
GPG Key ID: 8105A46B22C2D051
2 changed files with 42 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
crates/bdk/src/descriptor/policy.rs
View File

@ -662,11 +662,11 @@ impl Policy {
(0..*threshold).collect()
}
SatisfiableItem::Multisig { keys, .. } => (0..keys.len()).collect(),
_ => vec![],
_ => HashSet::new(),
};
let selected = match path.get(&self.id) {
Some(arr) => arr,
_ => &default,
let selected: HashSet<_> = match path.get(&self.id) {
Some(arr) => arr.iter().copied().collect(),
_ => default,
};
match &self.item {
@ -674,14 +674,24 @@ impl Policy {
let mapped_req = items
.iter()
.map(|i| i.get_condition(path))
.collect::<Result<Vec<_>, _>>()?;
.collect::<Vec<_>>();
// if all the requirements are null we don't care about `selected` because there
// are no requirements
if mapped_req.iter().all(Condition::is_null) {
if mapped_req
.iter()
.all(|cond| matches!(cond, Ok(c) if c.is_null()))
{
return Ok(Condition::default());
}
// make sure all the indexes in the `selected` list are within range
for index in &selected {
if *index >= items.len() {
return Err(PolicyError::IndexOutOfRange(*index));
}
}
// if we have something, make sure we have enough items. note that the user can set
// an empty value for this step in case of n-of-n, because `selected` is set to all
// the elements above
@ -690,23 +700,18 @@ impl Policy {
}
// check the selected items, see if there are conflicting requirements
let mut requirements = Condition::default();
for item_index in selected {
requirements = requirements.merge(
mapped_req
.get(*item_index)
.ok_or(PolicyError::IndexOutOfRange(*item_index))?,
)?;
}
Ok(requirements)
mapped_req
.into_iter()
.enumerate()
.filter(|(index, _)| selected.contains(index))
.try_fold(Condition::default(), |acc, (_, cond)| acc.merge(&cond?))
}
SatisfiableItem::Multisig { keys, threshold } => {
if selected.len() < *threshold {
return Err(PolicyError::NotEnoughItemsSelected(self.id.clone()));
}
if let Some(item) = selected.iter().find(|i| **i >= keys.len()) {
return Err(PolicyError::IndexOutOfRange(*item));
if let Some(item) = selected.into_iter().find(|&i| i >= keys.len()) {
return Err(PolicyError::IndexOutOfRange(item));
}
Ok(Condition::default())

19
crates/bdk/tests/wallet.rs
View File

@ -925,6 +925,25 @@ fn test_create_tx_policy_path_use_csv() {
assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(144));
}
#[test]
fn test_create_tx_policy_path_ignored_subtree_with_csv() {
let (mut wallet, _) = get_funded_wallet("wsh(or_d(pk(cRjo6jqfVNP33HhSS76UhXETZsGTZYx8FMFvR9kpbtCSV1PmdZdu),or_i(and_v(v:pkh(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW),older(30)),and_v(v:pkh(cMnkdebixpXMPfkcNEjjGin7s94hiehAH4mLbYkZoh9KSiNNmqC8),older(90)))))");
let external_policy = wallet.policies(KeychainKind::External).unwrap().unwrap();
let root_id = external_policy.id;
// child #0 is pk(cRjo6jqfVNP33HhSS76UhXETZsGTZYx8FMFvR9kpbtCSV1PmdZdu)
let path = vec![(root_id, vec![0])].into_iter().collect();
let addr = Address::from_str("2N1Ffz3WaNzbeLFBb51xyFMHYSEUXcbiSoX").unwrap();
let mut builder = wallet.build_tx();
builder
.add_recipient(addr.script_pubkey(), 30_000)
.policy_path(path, KeychainKind::External);
let (psbt, _) = builder.finish().unwrap();
assert_eq!(psbt.unsigned_tx.input[0].sequence, Sequence(0xFFFFFFFE));
}
#[test]
fn test_create_tx_global_xpubs_with_origin() {
use bitcoin::hashes::hex::FromHex;