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

Fix/policy path multisig #286

Merged
merged 2 commits into from
Feb 15, 2021

Conversation

afilini
Copy link
Member

@afilini afilini commented Feb 13, 2021

Description

Opening against the relase branch, as this is a bugfix instead of a new feature. Once the release is done it'll get merged back into master.

[policy] Allow specifying a policy path for Multisig

While technically it's not required since there are no timelocks inside, it's still less confusing for the end user if we allow this instead of failing like we do currently.

[policy] Remove the TooManyItemsSelected error

The TooManyItemsSelected error has been removed, since it's not technically an error but potentailly more of an "over-constraint" over a tx: for instance, given a thresh(3,pk(a),pk(b),older(10),older(20)) descriptor one could create a spending tx with the [0,1,2] items that would only be spendable after 10 blocks, or a tx with the [0,2,3] items that would be spendable after 20.

In this case specifying more items than the threshold would create a tx with the maximum constraint possible, in this case the 20 blocks. This is not necessarily an error, so we should allow it without failing.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • 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
  • I've updated CHANGELOG.md

Bugfixes:

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

The `TooManyItemsSelected` error has been removed, since it's not technically an
error but potentailly more of an "over-constraint" over a tx: for instance,
given a `thresh(3,pk(a),pk(b),older(10),older(20))` descriptor one could create
a spending tx with the `[0,1,2]` items that would only be spendable after `10`
blocks, or a tx with the `[0,2,3]` items that would be spendable after `20`.

In this case specifying more items than the threshold would create a tx with
the maximum constraint possible, in this case the `20` blocks. This is not
necessarily an error, so we should allow it without failing.
@afilini afilini force-pushed the fix/policy-path-multisig branch from 88ebfa4 to 08fa2d9 Compare February 13, 2021 16:10
While technically it's not required since there are no timelocks inside,
it's still less confusing for the end user if we allow this instead of
failing like we do currently.
@afilini afilini force-pushed the fix/policy-path-multisig branch from 08fa2d9 to b61427c Compare February 13, 2021 16:17
@notmandatory
Copy link
Member

ACK b61427c

Tested with bdk-cli and problem resolved.

Test steps:

  1. in local repo bdk-cli master edit Cargo.toml

    ...
    [dependencies] 
    bdk = { git = "https://github.com/afilini/bdk.git", branch = "fix/policy-path-multisig", default-features = false, features = ["all-keys"]}
    bdk-macros = { git = "https://github.com/afilini/bdk.git", branch = "fix/policy-path-multisig" }
    ...
    
  2. run bdk-cli repl command that failed for @thunderbiscuit. It now does not throw an error:

    cargo run --all-features -- repl --descriptor "wsh(multi(2,tprv8ZgxMBicQKsPdxAx3PFTiciVrm1rpnUCXjhbNheQhtsMQUnr8MNb3KWJEJ4Bp6EcTZSSKkvBt5HoRkv3aW7oyowy1BKEo5GcjZXHhyWSHPa/84h/1h/0h/0/*,[6bf3c709/84'/1'/0']tpubDCujUzKmyZPsAxjMyT3cVWCKRAPTtjJbJjez27kn1YZPPaSwbdeXXs9ZGkfE1rXRm4uRzfyXztMhb8ZNDS43mtwJbGbWQL8M4tY6y79hbSq/0/*))"
    
    create_tx --to "tb1qtdhckkuwl55598u6vgm2stvj4xgurzl4f35gck:0" --send_all --external_policy '{"nu9zsur5": [0,1]}'
    {
      "details": {
        "fees": 138,
        "height": null,  
        "received": 0,
        "sent": 6085,
        "timestamp": 1613246811,
        "transaction": null,
        "txid": "462dbb9242329f2826da3b9338802747efd2e90b8b042ec2ce7c1e289b07f0cc"
      },
      "psbt": "cHNidP8BAFIBAAAAAXOmXM5TZ1v2bujTTd3/va3IJcjfeEN+qF+6miF7KIrfAAAAAAD/////ATsXAAAAAAAAFgAUW2+LW479KUKfmmI2qC2SqZHBi/UAAAAAAAEBK8UXAAAAAAAAIgAg35WIXWE2CCtdwFkCbfFFKhKm3Xzx1f6rTq6WJpb1qy8BBUdSIQL9Oynd2xz0dqvB9a0+9c6pLyPO2655NSY59/5MT8WfvSEDmPCPGwmVd+O8bJb5WGhLLRd1PgMhG7Ipu1SXdVDko0ZSriIGAv07Kd3bHPR2q8H1rT71zqkvI87brnk1Jjn3/kxPxZ+9GDrrnyZUAACAAQAAgAAAAIAAAAAAAAAAACIGA5jwjxsJlXfjvGyW+VhoSy0XdT4DIRuyKbtUl3VQ5KNGGGvzxwlUAACAAQAAgAAAAIAAAAAAAAAAAAAA"
    }
    

@afilini afilini merged commit b61427c into bitcoindevkit:release/0.4.0 Feb 15, 2021
@afilini afilini deleted the fix/policy-path-multisig branch February 15, 2021 15:52
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