tests: add functional test for miniscript decaying multisig #29156

mjdietzx commented at 9:27 PM on December 29, 2023: contributor

This is very closely based on test/functional/wallet_multisig_descriptor_psbt.py both in code and concept. It should serve as some integration testing for Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4 and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the real world aligning this to halvenings would be nice).

DrahtBot commented at 9:27 PM on December 29, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29156.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, hodlinator, achow101
Concept ACK	michaelfolkson, epiccurious, RandyMcMillan
Stale ACK	kevkevinpal, Eunovo

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Dec 29, 2023

mjdietzx force-pushed on Dec 29, 2023

DrahtBot added the label CI failed on Dec 29, 2023

DrahtBot removed the label CI failed on Dec 29, 2023

mjdietzx force-pushed on Dec 30, 2023

kevkevinpal commented at 4:36 PM on January 6, 2024: contributor

PR description link is routes to 404 can you update to use this https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_multisig_descriptor_psbt.py instead?

kevkevinpal commented at 4:05 AM on January 8, 2024: contributor

Concept ACK 2422b90

I've done a little code review and so far it is looking good, I think it would be cool if instead of always using the pubkeys in the same order when decaying we could randomly use different signers, I just quickly tested it worked the other way around with this diff (not sure how you feel about adding this randomness)

             for m in range(self.M):
-                signers_multisig = participants["multisigs"][m]
+                self.log.info(self.N - 1 - m)
+                signers_multisig = participants["multisigs"][self.N - 1 - m]
                 self._check_psbt(psbt["psbt"], to, value, signers_multisig)
-                signing_wallet = participants["signers"][m]
+                signing_wallet = participants["signers"][self.N - 1 - m]
                 psbt = signing_wallet.walletprocesspsbt(psbt["psbt"])

That way we can assert that any of the keys can create the 4/4 3/4 2/4 and 1/4

Also, one thing I am unsure of is if we should include this test in test/functional/wallet_multisig_descriptor_psbt.py since they are very similar and maybe we can use common functions but I am not sure what will be the cleanest

mjdietzx commented at 5:24 PM on January 8, 2024: contributor

Thank you for the review, and good idea @kevkevinpal!

I think it would be cool if instead of always using the pubkeys in the same order when decaying we could randomly use different signers

I added a 2nd commit to this PR with your suggestion. In a similar spirit I improved #29154 (also adding a 2nd commit with your suggestion, and in that case we can also assert that order of xpubs in the multisig descriptors don't matter since it is sorted).

Also, one thing I am unsure of is if we should include this test in test/functional/wallet_multisig_descriptor_psbt.py since they are very similar and maybe we can use common functions but I am not sure what will be the cleanest

I don't think it would be too much trouble to combine them, but I didn't go that route because my opinion is that some code duplication is preferable to trying to generalize/combine these tests. So I went the route of keeping these as two simple independent integration tests. I don't have a strong opinion on this, but usually for test code like this I lean towards approach I took where I duplicate some code to favor simple and easy to explain/delete tests. If more people feel strongly one way or the other I'm happy to change though

kevkevinpal commented at 2:42 AM on January 9, 2024: contributor

ACK 7f0be89

michaelfolkson commented at 10:16 AM on January 9, 2024: contributor

Concept ACK

A couple of things on first skim read.

I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in wallet_miniscript.py?
Miniscript has always been described by its authors/contributors as an extension of descriptors. I personally wouldn't be against adding some Miniscript explanation, examples in descriptors.md but I'd want a short explanation of what Miniscript is making it clear that this decaying multisig example is enabled by Miniscript and not just added to the list of descriptor examples. Maybe longer term there should be a miniscript.md but for now that seems fine to me.

Eunovo commented at 11:53 AM on January 12, 2024: none

ACK 7f0be89

Adding an example for this to descriptors.md is really helpful but I think users who are unfamiliar with miniscript and descriptors might benefit from the addition of the fragments, thresh and after to the Reference section. Seeing these fragments used in the examples but not included the reference can introduce confusion.

Eunovo commented at 5:02 PM on January 12, 2024: none

I tested this locally and I've observed that the test doesn't fail if you modify the external wallet's timelocks, see below: external = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({f'),s:pk('.join(external_xpubs)}),sln:after(128)))") The reason is that the test only sends from the external wallet once using all keys and the transactions that use the locktimes send from the internal wallet because that's where the funds are. Hence, changing the timelocks for the external descriptor doesn't break the test.

I'm not sure if we care about this or not but fixing this is pretty simple, we just need to create a transaction that uses 3-of-4 keys and the first timelock and sends from the external wallet. The easiest way to do this is to modify the first transaction in the test. See sample code below:

self.generate(self.nodes[0], self.locktimes[0])
self.sync_blocks()
self.log.info("First, make a sending transaction, created using `walletcreatefundedpsbt` (anyone can initiate this)...")
psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, feeRate=0.00010, locktime=self.locktimes[0])

psbts = []
self.log.info("Now at least M users check the psbt with decodepsbt and (if OK) signs it with walletprocesspsbt...")
for m in random.sample(range(self.M), self.M - 1):
    signers_multisig = participants["multisigs"][m]
    self._check_psbt(psbt["psbt"], to, value, signers_multisig)
    signing_wallet = participants["signers"][m]
    partially_signed_psbt = signing_wallet.walletprocesspsbt(psbt["psbt"])
    psbts.append(partially_signed_psbt["psbt"])

self.log.info("Finally, collect the signed PSBTs with combinepsbt, finalizepsbt, then broadcast the resulting transaction...")
combined = coordinator_wallet.combinepsbt(psbts)
finalized = coordinator_wallet.finalizepsbt(combined)
coordinator_wallet.sendrawtransaction(finalized["hex"])

mjdietzx commented at 6:49 PM on January 12, 2024: contributor

Thank you @michaelfolkson and @Eunovo for the reviews! I agree with both of your feedback re improving descriptors.md docs to accompany this test. I will have a commit addressing your feedback in the coming days.

I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in wallet_miniscript.py

My intention with this test is to provide a "reference example" with the same motivations and flow/architecture as #22067 (and the original issue #21278 it resolved). But taking this one step further with a "decaying" multisig that highlights "Miniscript as an extension of descriptors" and showing the same multisig architecture and signing flows "just work" when we utilize Miniscript.

To your point - the additional raw test coverage this provides is limited, and if anything could be accomplished in wallet_miniscript.py. So if a "reference example" that tests end-to-end functionality of a wallet like this at a high-level (and more from a user perspective) isn't valuable then this is overkill and is not needed.

I think once I follow up with docs this intention will be clear. Then we can see if this additional test is useful or overkill.

modify(ing) the external wallet's timelocks @Eunovo interesting observation! We could also break up the initial deposit into multiple utxos so that we aren't only spending change after the first transaction. I played around with it locally after thinking through your writeup, but I'm leaning towards not modifying the test to try to assert this failure. Partly because modifying the external descriptor to differ from internal would be a different wallet conceptually than what I have here. Ideally #22838 would be merged and I'd be using those semantics in this test and then it's even more clear that external and internal wallets should only differ in derivation path

Regardless, it's cool you thought of this. For a similar wallet that uses relative time locks instead of absolute, this would be even weirder. Because the same concept could lead to the change of a spendable utxo being unspendable for a time.

mjdietzx force-pushed on Jan 13, 2024

epiccurious commented at 2:49 AM on February 11, 2024: contributor

Concept ACK 5023b47e85161a0b35839ce03c7b5d55ff1cd4c1.

achow101 commented at 3:48 PM on April 9, 2024: member

This is very closely based on test/functional/wallet_multisig_descriptor_psbt.py both in code and concept.

Can you articulate what meaningful difference there is?

mjdietzx commented at 4:58 PM on April 9, 2024: contributor

Can you articulate what meaningful difference there is?

Tests a Miniscript descriptor and asserts that it behaves as expected at different block heights as the multisig's thresh of required signers decreases.

I can't judge how meaningful that is or if it deserves a standalone test. As someone who checks in occasionally I needed to convince myself I can really throw Miniscript into a descriptor and the wallet will work/behave like I'm used to

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:154 in 5023b47e85 outdated

 149 | +        for locktime in self.locktimes:
 150 | +            self.M -= 1
 151 | +            self.log.info(f"At block height >= {locktime} this multisig is {self.M}-of-{self.N}")
 152 | +            current_height = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['height']
 153 | +
 154 | +            psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, feeRate=0.00010, locktime=locktime)

mjdietzx commented at 5:08 PM on April 9, 2024:

An interesting thing I learned with this test was psbt behavior related to locktime. If I created a psbt at a blockheight of 800,000 when multisig was 4-of-4 (maybe I want to pre-sign it with a cold wallet and save it for later), and then I wanted to spend it when the multisig decayed to 3-of-4 at blockheight 840,000, then wallet would not process that psbt as complete with 3 valid signatures like I expected (bc I guess it's using locktime of when psbt was created to know how to interpret the Miniscript and not current blockheight)

I didn't investigate further or learn why that is.. or if there is a fundamental reason for this behavior or if walletprocesspsbt can/should be improved

rkrux commented at 9:04 AM on April 13, 2024:

then wallet would not process that psbt as complete with 3 valid signatures like I expected

Do you mean the 4-signature transaction was rejected?

mjdietzx commented at 1:12 AM on April 14, 2024:

I just got confused here. CLTV opcode checks against spending transaction nLockTime. So behavior is exactly as expected

Do you mean the 4-signature transaction was rejected?

No I didn't mean this. 4-signature transaction would never be rejected. But I explained this confusingly so you can ignore my comment above

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:170 in 2422b90978 outdated

 163 | +
 164 | +            self.generate(self.nodes[0], locktime)
 165 | +            self.sync_blocks()
 166 | +            coordinator_wallet.sendrawtransaction(psbt["hex"])
 167 | +
 168 | +            self.log.info("Check that balances are correct after the transaction has been included in a block.")

rkrux commented at 9:02 AM on April 13, 2024:

2024-04-13T08:31:05.463000Z TestFramework (INFO): At block height >= 128 this multisig is 3-of-4
2024-04-13T08:31:05.473000Z TestFramework (INFO): Check that the time-locked transaction is too immature to spend...
2024-04-13T08:31:05.614000Z TestFramework (INFO): Check that balances are correct after the transaction has been included in a block.

The first time I ran this test, I felt there should be another log message in between that displays something like "Generating ABC blocks to cover for the timelock". Reading the 3rd message immediately after the 2nd one made me think this new message might make what the test is doing more explicit.

mjdietzx commented at 11:07 PM on April 13, 2024:

Good point! I added the message like you suggest. Logs now read:

2024-04-13T23:05:41.654000Z TestFramework (INFO): At block height >= 128 this multisig is 3-of-4
2024-04-13T23:05:41.674000Z TestFramework (INFO): Check that the time-locked transaction is too immature to spend with 3-of-4 at block height 103...
2024-04-13T23:05:41.675000Z TestFramework (INFO): Generate blocks to reach the time-lock block height 128 and broadcast the transaction...
2024-04-13T23:05:42.888000Z TestFramework (INFO): Check that balances are correct after the transaction has been included in a block.

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:147 in 2422b90978 outdated

 141 | +        self.log.info("Check that balances are correct after the transaction has been included in a block.")
 142 | +        self.generate(self.nodes[0], 1)
 143 | +        assert_approx(participants["multisigs"][0].getbalance(), deposit_amount - value, vspan=0.001)
 144 | +        assert_equal(participants["signers"][self.N - 1].getbalance(), value)
 145 | +
 146 | +        self.log.info("Send more transactions from the multisig as required signers decay, this time with a daisy chained signing flow (one after another in series)!")

rkrux commented at 9:07 AM on April 13, 2024:

Most of the code before this one is very similar to wallet_multisig_descriptor_psbt.py test - participants_create_multisigs is different though. As pointed out in other reviews as well, I feel this test can go in the same file and some commonalities can be extracted, but I don't think this is blocking though.

mjdietzx commented at 1:19 AM on April 14, 2024:

Yeah there is def some duplicated code between the two tests. But I want to focus on making this simple and easy to understand. It's trying to demonstrate/document an example wallet and could be used as a reference. Kind of like a quick start / tutorial

If I were to combine these tests it would become harder for someone to quickly and easily follow, and would lose the purpose of being a quick example. I think it's quite common to have code duplication in these types of tests/examples for that reason and hope this is fine

rkrux approved

rkrux commented at 9:07 AM on April 13, 2024: contributor

tACK 5023b47

Build and all functional tests successful. I like the clarity and the expressiveness with which this test has been written, and believe this is a good addition. Thanks for coming up with this @mjdietzx!

Suggested adding a log message in the last portion of the test.

mjdietzx force-pushed on Apr 13, 2024

mjdietzx force-pushed on Apr 14, 2024

mjdietzx commented at 1:13 AM on April 14, 2024: contributor

Thanks for the review @rkrux !

Suggested adding a log message in the last portion of the test.

Nice suggestion, I did this just now when I rebased on latest master

in doc/descriptors.md:66 in de93b86358 outdated

  62 | @@ -63,6 +63,7 @@ Output descriptors currently support:
  63 |  - `wsh(sortedmulti(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/0/0/*))` describes a set of *1-of-2* P2WSH multisig outputs where one multisig key is the *1/0/`i`* child of the first specified xpub and the other multisig key is the *0/0/`i`* child of the second specified xpub, and `i` is any number in a configurable range (`0-1000` by default). The order of public keys in the resulting witnessScripts is determined by the lexicographic order of the public keys at that index.
  64 |  - `tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,{pk(fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),pk(e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13)})` describes a P2TR output with the `c6...` x-only pubkey as internal key, and two script paths.
  65 |  - `tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,sortedmulti_a(2,2f8bde4d1a07209355b4a7250a5c5128e88b84bddc619ab7cba8d569b240efe4,5cbdf0646e5db4eaa398f365f2ea7a0e3d419b7e0330e39ce92bddedcac4f9bc))` describes a P2TR output with the `c6...` x-only pubkey as internal key, and a single `multi_a` script that needs 2 signatures with 2 specified x-only keys, which will be sorted lexicographically.
  66 | +- `wsh(thresh(4,pk([7258e4f9/44h/1h/0h]tpubDCZrkQoEU3845aFKUu9VQBYWZtrTwxMzcxnBwKFCYXHD6gEXvtFcxddCCLFsEwmxQaG15izcHxj48SXg1QS5FQGMBx5Ak6deXKPAL7wauBU/0/*),s:pk([c80b1469/44h/1h/0h]tpubDD3UwwHoNUF4F3Vi5PiUVTc3ji1uThuRfFyBexTSHoAcHuWW2z8qEE2YujegcLtgthr3wMp3ZauvNG9eT9xfJyxXCfNty8h6rDBYU8UU1qq/0/*),s:pk([4e5024fe/44h/1h/0h]tpubDDLrpPymPLSCJyCMLQdmcWxrAWwsqqssm5NdxT2WSdEBPSXNXxwbeKtsHAyXPpLkhUyKovtZgCi47QxVpw9iVkg95UUgeevyAqtJ9dqBqa1/0/*),s:pk([3b1d1ee9/44h/1h/0h]tpubDCmDTANBWPzf6d8Ap1J5Ku7J1Ay92MpHMrEV7M5muWxCrTBN1g5f1NPcjMEL6dJHxbvEKNZtYCdowaSTN81DAyLsmv6w6xjJHCQNkxrsrfu/0/*),sln:after(840000),sln:after(1050000),sln:after(1260000)))#k28080kv` describes a Miniscript multisig with spending policy: `thresh(4,pk(key_1),pk(key_2),pk(key_3),pk(key_4),after(t1),after(t2),after(t3))` that starts as 4-of-4 and "decays" to 3-of-4, 2-of-4, and finally 1-of-4 at each future halvening block height. This descriptor was generated by a test run of [test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py](/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py). For brevity, the internal "change" descriptor accompanying the above external "receiving" descriptor is not included here, but it typically differs only in the xpub derivation steps, ending in `/1/*` for change addresses.

achow101 commented at 7:11 PM on April 16, 2024:

In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

It's not necessary to state where the descriptor comes from.

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:24 in de93b86358 outdated

  19 | +class WalletMiniscriptDecayingMultisigDescriptorPSBTTest(BitcoinTestFramework):
  20 | +    def add_options(self, parser):
  21 | +        self.add_wallet_options(parser, legacy=False)
  22 | +
  23 | +    def set_test_params(self):
  24 | +        self.num_nodes = 4

achow101 commented at 7:12 PM on April 16, 2024:

In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

Using multiple wallets is preferred over using multiple nodes.

hodlinator commented at 10:11 PM on February 15, 2025:

(I'm happy wallet_multisig_descriptor_psbt.py tests using isolated nodes, but that also means there's less need for that in this test).

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:76 in de93b86358 outdated

  71 | +                    "internal": True,
  72 | +                    "timestamp": "now",
  73 | +                },
  74 | +            ])
  75 | +            assert all(r["success"] for r in result)
  76 | +            yield multisig

achow101 commented at 7:17 PM on April 16, 2024:

In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

A lot of this is very similar to/the same as wallet_multisig_descriptor_psbt.py. I would prefer if they were combined into one test file. Note that you can (and should) make separate functions for the different test cases so that there is a distinct separation between them, while still using the same utility functions.

mjdietzx commented at 6:13 PM on April 28, 2024:

After addressing comments from your review, I made some simplifications and the two tests are now different enough I think this is no longer the case. The tests are still similar in their high-level structure, but otherwise not much code is duplicated (and definitely no complex/deep logic)

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:144 in de93b86358 outdated

 139 | +        coordinator_wallet.sendrawtransaction(finalized["hex"])
 140 | +
 141 | +        self.log.info("Check that balances are correct after the transaction has been included in a block.")
 142 | +        self.generate(self.nodes[0], 1)
 143 | +        assert_approx(participants["multisigs"][0].getbalance(), deposit_amount - value, vspan=0.001)
 144 | +        assert_equal(participants["signers"][self.N - 1].getbalance(), value)

achow101 commented at 7:22 PM on April 16, 2024:

In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

This workflow is very similar to what will happen in the loop below. Can they be consolidated?

mjdietzx commented at 5:50 PM on April 28, 2024:

I don't think it was ever necessary for me to demonstrate this alternative signing flow as part of this test. So I simplified and removed this code. Now all sends are tested in the single loop

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:81 in de93b86358 outdated

  76 | +            yield multisig
  77 | +
  78 | +    def run_test(self):
  79 | +        self.M = self.num_nodes
  80 | +        self.N = self.num_nodes
  81 | +        self.locktimes = [128, 256, 512]  # in real-world align with future halvenings: [840000, 1050000, 1260000]

achow101 commented at 7:22 PM on April 16, 2024:

In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

Mining hundreds of blocks can take a while and is unnecessary for this test. This could run quite a bit faster with locktimes that are closer to each other.

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:164 in de93b86358 outdated

 159 | +                assert_equal(psbt["complete"], m == self.M - 1)
 160 | +            self.log.info("Check that the time-locked transaction is too immature to spend...")
 161 | +            assert_equal(current_height >= locktime, False)
 162 | +            assert_raises_rpc_error(-26, "non-final", coordinator_wallet.sendrawtransaction, psbt["hex"])
 163 | +
 164 | +            self.generate(self.nodes[0], locktime)

achow101 commented at 7:25 PM on April 16, 2024:

In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

This massively overshoots the locktime block each time. I think it would be better to mine just enough blocks to reach the locktime height.

achow101 commented at 7:28 PM on April 16, 2024: member

e0b0a385350d8f7a9d00542cc3d14bb47224d889 "tests: miniscript decaying multisig signing order does not matter" could be squashed into de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig".

mjdietzx commented at 6:07 PM on April 22, 2024: contributor

Thank you for the thorough review @achow101! I'll update this PR addressing all of your comments and ping you when I've pushed (I'll need to find a chunk of time to do this so it may be a week or two)

mjdietzx force-pushed on Apr 28, 2024

mjdietzx commented at 6:10 PM on April 28, 2024: contributor

I've addressed all your comments and rebased/squashed @achow101

Using multiple wallets is preferred over using multiple nodes.

This point led me to simplify and remove some unnecessary code/checks. When I had first implemented test/functional/wallet_multisig_descriptor_psbt.py there was feedback/interest in different nodes/participants coordinating together. That led to some additional checks, code, and showing two different signing flows.

In this test I don't think coordination across nodes is necessary or in the scope of what it's trying to show (focused on the number of required signers decaying at the defined block heights). So I made this improvement as you suggested which led to simplifications and removing chunks of now unnecessary code.

RandyMcMillan commented at 10:45 PM on April 28, 2024: contributor

Concept ACK 7ddfc6d

DrahtBot added the label Needs rebase on Jul 10, 2024

mjdietzx force-pushed on Jul 10, 2024

DrahtBot removed the label Needs rebase on Jul 10, 2024

DrahtBot added the label CI failed on Jul 24, 2024

DrahtBot commented at 1:34 PM on July 24, 2024: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/27260554001</sub>

<details><summary>Hints</summary>

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

mjdietzx force-pushed on Jul 27, 2024

DrahtBot removed the label CI failed on Jul 28, 2024

mjdietzx requested review from achow101 on Aug 15, 2024

DrahtBot added the label CI failed on Sep 27, 2024

maflcko removed the label CI failed on Oct 1, 2024

DrahtBot added the label CI failed on Nov 4, 2024

DrahtBot removed the label CI failed on Nov 4, 2024

rkrux commented at 12:00 PM on November 11, 2024: contributor

@mjdietzx I will review this PR again soon as I want to see a decaying multi-sig using miniscript example checked in. Would you be willing to rebase this over master to get the CMake build changes? The other option is for me (and other reviewers) to use the older build commands to test this PR locally, which is fine as well but at the cost of a much longer build time and having two building tools in my local. Also, it would be nice to ensure this works with the latest build system though there are only python changes.

mjdietzx commented at 3:18 AM on November 15, 2024: contributor

@rkrux thanks for the heads up. I didn't expect changes to build system would affect this. I'll rebase asap, also want to bring in achow's #22838

mjdietzx force-pushed on Dec 3, 2024

mjdietzx commented at 8:14 PM on December 3, 2024: contributor

@rkrux I updated to master and rebased. There were no conflicts and this test continues to pass without needing any modifications. This should be ready to merge

Note: I will update to multi-path descriptors now that it's merged in a tiny followup PR. but I don't want to mess with all the prior reviews on this PR so not introducing any new changes

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:72 in aae0320153 outdated

  67 | +    def run_test(self):
  68 | +        self.node = self.nodes[0]
  69 | +        self.M = 4  # starts as 4-of-4
  70 | +        self.N = 4
  71 | +
  72 | +        self.locktimes = [104, 106, 108]  # in real-world align with future halvenings, eg: [840000, 1050000, 1260000]

rkrux commented at 1:03 PM on December 9, 2024:

Nit: While the message in the comment is a neat idea for the timelocks, I don't think it is necessary to add in the test.

mjdietzx commented at 3:08 AM on December 13, 2024:

yeah maybe it's over specific. I'll either remove or improve it

mjdietzx commented at 5:51 PM on January 22, 2025:

Removed this comment

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:34 in aae0320153 outdated

  29 | +
  30 | +    def skip_test_if_missing_module(self):
  31 | +        self.skip_if_no_wallet()
  32 | +        self.skip_if_no_sqlite()
  33 | +
  34 | +    @staticmethod

rkrux commented at 1:24 PM on December 9, 2024:

Why is this static method?

mjdietzx commented at 2:47 AM on December 13, 2024:

bc it doesn't use self so it indicates that it's a utility function that doesn't access class-data. just a convention, doesn't really matter

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:76 in aae0320153 outdated

  71 | +
  72 | +        self.locktimes = [104, 106, 108]  # in real-world align with future halvenings, eg: [840000, 1050000, 1260000]
  73 | +        assert_equal(len(self.locktimes), self.N - 1)
  74 | +
  75 | +        self.name = f"{self.M}_of_{self.N}_decaying_multisig"
  76 | +        self.log.info(f"Testing a miniscript multisig which starts as 4-of-4 and 'decays' to 3-of-4 at block height {self.locktimes[0]}, 2-of-4 at {self.locktimes[1]}, and finally 1-of-4 at {self.locktimes[2]}...")

rkrux commented at 1:27 PM on December 9, 2024:

Nit:

        self.log.info(f"Testing a miniscript multisig which starts as {self.N}-of-{self.N} and 'decays' to {self.N-1}-of-{self.N} at block height {self.locktimes[0]}, {self.N-2}-of-{self.N} at {self.locktimes[1]}, and finally {self.N-3}-of-{self.N} at {self.locktimes[2]}...")

mjdietzx commented at 3:07 AM on December 13, 2024:

seems more complicated to me tbh

rkrux commented at 5:15 AM on December 13, 2024:

Wanted to avoid hardcoding, feel free to ignore.

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:112 in aae0320153 outdated

 108 | +            # the random sample asserts that any of the signing keys can sign for the 3-of-4,
 109 | +            # 2-of-4, and 1-of-4. While this is basic behavior of the miniscript thresh primitive,
 110 | +            # it is a critical property of this wallet.
 111 | +            for i, m in enumerate(random.sample(range(self.M), self.M)):
 112 | +                psbt = signers[m].walletprocesspsbt(psbt["psbt"])
 113 | +                assert_equal(psbt["complete"], i == self.M - 1)

rkrux commented at 1:32 PM on December 9, 2024:

This is a neat check! Do you think there is value in calling the testmempoolaccept RPC as well in the case psbt is complete?

mjdietzx commented at 3:01 AM on December 13, 2024:

I don't think it'd hurt to add, but also not necessary because we will be broadcasting this to mempool next and that would revert. if it is simple to add testmempoolaccept though I'll add this as an improvement when I rebase

mjdietzx commented at 5:52 PM on January 22, 2025:

I looked into adding this, but psbt object does not include the raw tx that we can broadcast (psbt['hex') until the psbt is complete. so it would have been weird/difficult to add this additional check because the interface already prevents it. so i won't add this

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:130 in aae0320153 outdated

 126 | +            sent += amount
 127 | +
 128 | +            self.log.info("Check that balances are correct after the transaction has been included in a block...")
 129 | +            self.generate(self.node, 1)
 130 | +            assert_approx(multisig.getbalance(), deposit_amount - sent, vspan=0.001)
 131 | +            assert_equal(receiver.getbalance(), sent)

rkrux commented at 1:33 PM on December 9, 2024:

Why is assert_approx not needed here?

mjdietzx commented at 3:07 AM on December 13, 2024:

because the receiver gets the full amount sent, while the sender sent the full amount plus a small txn fee (which is why sender needs to use assert approx to keep it simple)

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:102 in aae0320153 outdated

  97 | +        for locktime in [0] + self.locktimes:
  98 | +            self.log.info(f"At block height >= {locktime} this multisig is {self.M}-of-{self.N}")
  99 | +            current_height = self.node.getblock(self.node.getbestblockhash())['height']
 100 | +
 101 | +            amount = 1.5
 102 | +            receiver = signers[0]

rkrux commented at 1:39 PM on December 9, 2024:

sent is updated cumulatively here: https://github.com/bitcoin/bitcoin/pull/29156/files#diff-6d077a4bc1ef7303d3259ceb996ba18ac82741c9bf65afd403fb938ffbef2ba7R126 but the receiver is update every time in the for loop! Not a bug because the same receiver is picked every time but receiver should be defined once outside the loop just like the sent var.

mjdietzx commented at 2:59 AM on December 13, 2024:

good point, seems like amount is also hardcoded and would make more sense to be defined outside loop. will update when I rebase

mjdietzx commented at 5:52 PM on January 22, 2025:

Done

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:91 in aae0320153 outdated

  86 | +        coordinator_wallet = self.node.get_wallet_rpc(self.node.createwallet(wallet_name=f"coordinator", descriptors=True)["name"])
  87 | +        self.generatetoaddress(self.node, 101, coordinator_wallet.getnewaddress())
  88 | +
  89 | +        self.log.info("Send funds to the multisig's receiving address...")
  90 | +        deposit_amount = 6.15
  91 | +        coordinator_wallet.sendtoaddress(multisig.getnewaddress(), deposit_amount)

rkrux commented at 1:45 PM on December 9, 2024:

If I'm not missing something, why is coordinator wallet needed here? Is there any issue in sending funds directly to the multi-sig address?

mjdietzx commented at 2:50 AM on December 13, 2024:

coordinator has a balance (the block rewards) so this is where the funds originate. it then sends to multisig and we assert expected values. I guess we could send directly to multisig but then it'd be doing self sends and we wouldn't be able to do simple checks on balances since it wouldn't change

rkrux commented at 5:14 AM on December 13, 2024:

I don't think there would be self sends from wallets point of view because in the loop the funds are transferred from the multisig wallet to the signer wallet, both of which are two different wallets with different balances as I checked.

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:99 in aae0320153 outdated

  92 | +        self.generate(self.node, 1)
  93 | +        assert_approx(multisig.getbalance(), deposit_amount, vspan=0.001)
  94 | +
  95 | +        self.log.info("Send transactions from the multisig as required signers decay...")
  96 | +        sent = 0
  97 | +        for locktime in [0] + self.locktimes:

rkrux commented at 1:51 PM on December 9, 2024:

Why is 0 appended? Isn't the block height 102 at the start of the loop?

mjdietzx commented at 2:50 AM on December 13, 2024:

because at block 0 it is a 4 of 4 multisig. we test that case, then go through the lock times which correspond to 3 of 4, 2 of 4, 1 of 4 as it decays at those block heights

rkrux commented at 5:14 AM on December 13, 2024:

I get it now, 0 acts a "default/initial" locktime.

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:121 in aae0320153 outdated

 117 | +                assert_equal(current_height >= locktime, False)
 118 | +                assert_raises_rpc_error(-26, "non-final", multisig.sendrawtransaction, psbt["hex"])
 119 | +
 120 | +                self.log.info(f"Generate blocks to reach the time-lock block height {locktime} and broadcast the transaction...")
 121 | +                self.generate(self.node, locktime - current_height)
 122 | +            else:

rkrux commented at 1:53 PM on December 9, 2024:

2024-12-09T13:49:54.971000Z TestFramework (INFO): At block height >= 0 this multisig is 4-of-4
2024-12-09T13:49:54.980000Z TestFramework (INFO): We can always spend with all signers!

I found this log slightly confusing. Before the block height 104, 4 sigs are indeed required. Maybe the phrase "We can always spend with all signers" is more suited while spending after the height 108?

mjdietzx commented at 3:06 AM on December 13, 2024:

It's just ensuring that no matter what 4 of 4 signers can always spend. So this is independent of block height. So this is asserted by checking that 4 of 4 can spend before the first lock time. Maybe it's confusing bc it's obvious but I thought it was important to check

rkrux commented at 5:14 AM on December 13, 2024:

I do believe it's important to check and if being done at any height less than the first locktime, then indeed all the signers are needed. I got a little thrown off by the word "always" in the middle because I expected a phrase like "All the signers are required to spend before the first locktime".

mjdietzx commented at 5:53 PM on January 22, 2025:

I updated the log to what you expected because i think it makes more sense. Changed from "We can always spend with all signers!" to "We can always spend with all signers"

rkrux commented at 7:12 AM on January 27, 2025:

I believe you updated it to "All the signers are required to spend before the first locktime" that is easier for me to understand in the given context, thank you.

rkrux commented at 1:55 PM on December 9, 2024: contributor

tACK aae032015367be812154a0e87955e6fd5ea5c192

My comments are mostly nits and few questions.

bitcoin deleted a comment on Dec 9, 2024

DrahtBot added the label CI failed on Dec 12, 2024

DrahtBot commented at 6:45 AM on December 12, 2024: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/33873388110</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

mjdietzx commented at 3:09 AM on December 13, 2024: contributor

Thank you for the thorough review @rkrux! I've responded to all your comments. I will @ you when I've rebased and addressed the ones that need it

mjdietzx force-pushed on Jan 22, 2025

tests: add functional test for miniscript decaying multisig

This is similar in structure to test/functional/wallet_multisig_descriptor_psbt.py
both in code and concept. It should serve as some integration testing for
Miniscript descriptors, and also documents a simple multisig that starts as 4-of-4
and decays to 3-of-4, 2-of-4, and finally 1-of-4 at block heights (I think in the
real world aligning this to halvenings would be nice).

bb633c9407

mjdietzx force-pushed on Jan 22, 2025

mjdietzx commented at 5:58 PM on January 22, 2025: contributor

@maflcko I fixed the lint errors, but CI failed for an unrelated technical reason. Are you able to trigger CI to re-run lint job? @rkrux I've rebased with the improvements based on your review and resolved/responded to your comments in PR

DrahtBot removed the label CI failed on Jan 22, 2025

rkrux approved

rkrux commented at 7:23 AM on January 27, 2025: contributor

reACK bb633c9407c46eeadf6fe85e859cea1fed24473f @mjdietzx Thanks for addressing all the comments!

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:20 in bb633c9407

  15 | +    assert_equal,
  16 | +    assert_raises_rpc_error,
  17 | +)
  18 | +
  19 | +
  20 | +class WalletMiniscriptDecayingMultisigDescriptorPSBTTest(BitcoinTestFramework):

hodlinator commented at 8:21 PM on February 15, 2025:

The "original" WalletMultisigDescriptorPSBTTest doesn't mention Miniscript in the name, so I would drop that, and possibly more ("decaying multisig" hopefully implies descriptors and PSBTs being used).

class WalletDecayingMultisigTest(BitcoinTestFramework):

(Filename should be kept in sync if you rename).

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:116 in bb633c9407

 111 | +                psbt = signers[m].walletprocesspsbt(psbt["psbt"])
 112 | +                assert_equal(psbt["complete"], i == self.M - 1)
 113 | +
 114 | +            if self.M < self.N:
 115 | +                self.log.info(f"Check that the time-locked transaction is too immature to spend with {self.M}-of-{self.N} at block height {current_height}...")
 116 | +                assert_equal(current_height >= locktime, False)

hodlinator commented at 8:45 PM on February 15, 2025:

Seems like a good time to use:

                assert_greater_than(locktime, current_height)

If you prefer a visible comparison operator, this is simpler (states what we are testing against first and avoids negation):

                assert_equal(True, current_height < locktime)

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:49 in bb633c9407

  44 | +        self.node.createwallet(wallet_name=f"{self.name}", blank=True, descriptors=True, disable_private_keys=True)
  45 | +        multisig = self.node.get_wallet_rpc(f"{self.name}")
  46 | +        # spending policy: `thresh(4,pk(key_1),pk(key_2),pk(key_3),pk(key_4),after(t1),after(t2),after(t3))`
  47 | +        # IMPORTANT: when backing up your descriptor, the order of key_1...key_4 must be correct!
  48 | +        external = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({'),s:pk('.join(external_xpubs)}),sln:after({'),sln:after('.join(map(str, self.locktimes))})))")
  49 | +        internal = multisig.getdescriptorinfo(f"wsh(thresh({self.N},pk({'),s:pk('.join(internal_xpubs)}),sln:after({'),sln:after('.join(map(str, self.locktimes))})))")

hodlinator commented at 9:08 PM on February 15, 2025:

Might be nice to add this just after to aid understanding of the complex expressions above:

self.log.debug(f'Miniscript: {external["descriptor"]}')

in test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py:69 in bb633c9407

  64 | +        assert all(r["success"] for r in result)
  65 | +        return multisig
  66 | +
  67 | +    def run_test(self):
  68 | +        self.node = self.nodes[0]
  69 | +        self.M = 4  # starts as 4-of-4

hodlinator commented at 10:55 PM on February 15, 2025:

M should probably be lowercase to signal later mutation? Also is only used in this function.

        m = 4  # starts as 4-of-4

(Could optionally make self.N local too if you passed it in as an argument to create_multisig().

hodlinator approved

hodlinator commented at 3:04 PM on February 17, 2025: contributor

ACK bb633c9407c46eeadf6fe85e859cea1fed24473f

Pretty neat to see thresholds being used in that way - as block heights are reached, each fulfilled locktime contributes to the count for the thresh. Was imagining some gigantic repeated sortedmulti + or + after combo (which would not have the key order problem inside of each sortedmulti, but would still require correct order of everything else and be prohibitively big).

PR helped me realize how different Miniscript syntax is from Output Descriptors. Miniscript is more of an intermediate representation than I remembered.

Good to see the test attempts premature signing of the PSBTs and verifies failure. It means the less readable "identity" specifiers in the Miniscript are very likely correct. Added the suggested log statement to get the miniscript for the receive addresses, replaced keys+paths with letters to get:

thresh(4,pk(A),s:pk(B),s:pk(C),s:pk(D),sln:after(104),sln:after(106),sln:after(108))

Put that into the analyzer at https://bitcoin.sipa.be/miniscript/ and got a Bitcoin Script that seems to do the right thing; adding together CHECKSIG results with optional CLTV checks to see if we get 4 in total. Quite magical how the CLTV checks are opted into automatically without having to specify more than the locktime-parameter to the walletcreatefundedpsbt-RPC.

Was only able to find some nits at this point, but PR has gone through a fair amount of changes already (switching to 1 node, only doing PSBTs-signing in 1 way instead of 2).

achow101 commented at 2:10 AM on February 19, 2025: member

ACK bb633c9407c46eeadf6fe85e859cea1fed24473f

achow101 merged this on Feb 19, 2025

achow101 closed this on Feb 19, 2025

TheCharlatan referenced this in commit f78a01a560 on Feb 22, 2025

rkrux commented at 1:21 PM on February 25, 2025: contributor

Glad to see this PR merged!