Verify that a 999-of-999 taproot multisig wallet is possible and can spend from it, and that neither a 1-of-0 nor 1-of-1000 is allowed.
The tests will require some time to run. On my Mac M1 the new tests run in about 40 seconds.
Had to bump fee rate to resolve assertions in sendtoaddress and psbt methods:
Fixes #28179
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process. A summary of reviews will appear here.
No conflicts as of last run.
512+ # Test 999-of-999
513+ self.do_test_k_of_n(999, 999)
514+ # Test 1-of-1000, should not succeed
515+ try:
516+ self.do_test_k_of_n(1, 1000)
517+ except JSONRPCException as e:
won’t this pass if no exception is raised? Shouldn’t we add something like either:
0try:
1 self.do_test_k_of_n(1, 1000)
2 assert False, "Expected exception was not raised for 1-of-1000 test"
3except JSONRPCException as e:
or via unittest:
0with self.assertRaises(JSONRPCException) as cm:
1 self.do_test_k_of_n(1, 1000)
2self.assertEqual(cm.exception.error["code"], -5)
3self.assertIn("Cannot have 1000 keys in multi_a; must have between 1 and 999 keys, inclusive", cm.exception.error["message"])
?
assert_raises_rpc_error is probably what you want
assert_raises_rpc_error. Just take a look on how it’s used elsewhere?
0# Test that 1-of-1000 and 999-of-1000 raise exception
1for k in [1, 999]:
2 assert_raises_rpc_error(-5, "Cannot have 1000 keys in multi_a; must have between 1 and 999 keys, inclusive", self.do_test_k_of_n, k, 1000)
or maybe even:
0for k in [0, 1, 999, 1000]:
if that makes sense.
387@@ -387,6 +388,14 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change)
388 psbt_online.unloadwallet()
389 psbt_offline.unloadwallet()
390
391+ def do_test_k_of_n(self, k, n):
It’s a function that allows setup and testing of any k-of-n wallet. It builds on the idea of the existing test that @darosior pointed to, which starts on L495 https://github.com/bitcoin/bitcoin/blob/64440bb733896a7a2caf902825e0406cb993e666/test/functional/wallet_taproot.py#L495 where a random number of n is used.
The part that differs is how the [KEY_1,...,KEY_N] array inside the multi_a is built - where the previous function uses ([H_POINT] * rnd_pos) up until k1 and then ([H_POINT] * (MAX_PUBKEYS_PER_MULTI_A - 1 - rnd_pos) after, the new function uses k1 for the whole array of.
However, because this previous test is not documented, I’m not entirely sure about the H_POINT parts do and if I need them in my test somehow, or if the way I’ve written it is enough to verify the 999-of-999 and 1-of-1000 cases.
524- self.do_test_k_of_n(999, 1000)
525- except JSONRPCException as e:
526- assert e.error["code"] == -5 and "Cannot have 1000 keys in multi_a; must have between 1 and 999 keys, inclusive" in e.error["message"]
527+ # Test that all of the combinations 0-of-1000, 1-of-1000 and 999-of-1000 and 1000-of-1000 raise exception
528+ for k in [0, 1, 999, 1000]:
529+ assert_raises_rpc_error(-5, "Cannot have 1000 keys in multi_a; must have between 1 and 999 keys, inclusive", self.do_test_k_of_n, k, 1000)
isn’t it weird that the error message states “Cannot have 1000 keys” and k is ignored?
Based on the error the k doesn’t even matter (it fails for 500 as well), so is it even important to test multiple k values here?
Maybe keep it simple and just test 1 of 1000.
In that case I also don’t think you need the k_of_n helper function.
k, not n.
303@@ -300,7 +304,7 @@ def do_test_sendtoaddress(self, comment, pattern, privmap, treefn, keys_pay, key
304 test_balance = int(rpc_online.getbalance() * 100000000)
305 ret_amnt = random.randrange(100000, test_balance)
306 # Increase fee_rate to compensate for the wallet's inability to estimate fees for script path spends.
307- res = rpc_online.sendtoaddress(address=self.boring.getnewaddress(), amount=Decimal(ret_amnt) / 100000000, subtractfeefromamount=True, fee_rate=200)
308+ res = rpc_online.sendtoaddress(address=self.boring.getnewaddress(), amount=Decimal(ret_amnt) / 100000000, subtractfeefromamount=True, fee_rate=500)
If I didn’t bump the fee rate I would run into the two error codes mentioned in the PR message:
0Had to bump fee rate to resolve assertions in sendtoaddress and psbt methods:
1
2 code -5: assert rpc_online.gettransaction(txid)["confirmations"] > 0
3 code -26: min relay fee not met
The previous fee_rate = 200 limit seems to be sufficient up until n ~ 500. I assume this is because the script size increases with higher numbers of n.
389@@ -387,6 +390,14 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change)
390 psbt_online.unloadwallet()
391 psbt_offline.unloadwallet()
392
393+ def do_test_k_of_n(self, k, n):
394+ self.do_test(
395+ f"tr(XPUB,multi_a({k},XPRV_1,...,XPRV_N)",
396+ f"tr($2/*,multi_a({k}" + (",$1/*" * n) + "))",
10@@ -11,7 +11,10 @@
11 from test_framework.address import output_key_to_p2tr
12 from test_framework.key import H_POINT
13 from test_framework.test_framework import BitcoinTestFramework
14-from test_framework.util import assert_equal
15+from test_framework.util import (
16+ assert_equal,
17+ assert_raises_rpc_error
0 assert_raises_rpc_error,
style-nit: Add the missing comma here? Otherwise someone will have to touch this line again when appending something.
A (force) push should also make CI less red.
389@@ -387,6 +390,14 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change)
390 psbt_online.unloadwallet()
391 psbt_offline.unloadwallet()
392
393+ def do_test_k_of_n(self, k, n):
394+ self.do_test(
395+ f"tr(XPUB,multi_a({k},XPRV_1,...,XPRV_N)",
396+ f"tr($2/*,multi_a({k}" + (",$1/*" * n) + "))",
397+ [True, False],
privmap argument determines for which keys an xpriv is inserted into the descriptor.
509@@ -499,6 +510,20 @@ def run_test(self):
510 [True, False],
511 lambda k1, k2: (key(k2), [multi_a(1, ([H_POINT] * rnd_pos) + [k1] + ([H_POINT] * (MAX_PUBKEYS_PER_MULTI_A - 1 - rnd_pos)))])
512 )
513+
514+ # Test 999-of-999
515+ self.do_test_k_of_n(999, 999)
Unfortunately this leads to extreme CPU usage and a sendtoaddress RPC timeout even on my 2019 MacBook Pro.
I suggest sticking to a lower maximum for now, so we at least get this useful helper function, and leaving a note like:
0// TODO: fix or work around performance bottleneck in `sendtoaddress`
1// in order to test the maximum Taproot allowed 999-of-999.
My Macbook can easily handle 1-of-100. The 30 second timeout is hit somewhere between 1-of-750 and 1-of-1000. The CI machines may be slower.
In term of fixing the bottleneck, you could try the more modern send RPC though I doubt that makes a difference. Maybe @achow101 has an idea?
send RPC approach would work efficiently here.
@Sjors just to make sure, did you mean 100-of-100? Or should I leave this out completely and just keep the 1-of-0 and 1-of-1000 tests further down that utilises the same new helper method?
0 # Test 100-of-100
1 self.do_test_k_of_n(100, 100)
The CI machines seem to make it through without timeout: https://github.com/bitcoin/bitcoin/actions/runs/7221218374/job/19675746505?pr=28212#step:7:3594 https://github.com/bitcoin/bitcoin/actions/runs/7221218374/job/19675746796?pr=28212#step:27:330
However, the test runs for 15 minutes on the Win64 job and for 20 minutes on the macOS job, making it the longest running functional test by far.
If it still takes that long you probably want to pick a smaller K and/or N.
did you mean 100-of-100? Or should I leave this out completely and just keep the 1-of-0 and 1-of-1000 tests further down that utilises the same new helper method?
The other two tests only check the failure case. A 1-of-100 would check the success path. 100-of-100 might be fine too, but maybe slower?
100-of-100 seems to run okay, less than 16 sec to complete the whole test. However, after the rebase I see the same behavior on my Mac M1 as you’re seeing on your 2019 MacBook Pro. Even the 1-of-999 test doesn’t pass without an RPC timeout, which it did before! So perhaps there’s been some change while the PR has been open that now causes the test to perform slower? 🤔
1-of-999 gives me:
'sendall' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
999-of-999 gives me
'sendtoaddress' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
git clean -dfx) and building again. To try on an older version of master, you can you check out any old commit and then do a git cherry-pick ....
Interesting. Try cleaning your working directory (e.g.
git clean -dfx) and building again. To try on an older version of master, you can you check out any old commit and then do agit cherry-pick ....
Git bisecting this I found that the tests start timing out with commit 4f473ea515bc77b9138323dab8a741c063d32e8f
Are you able to verify this on your setup? @darosior would you be able to check as well? 🙏
This is how I did it:
0git checkout 4f473ea515bc77b9138323dab8a741c063d32e8f
1git cherry-pick 387c12e0813a863bc9728777719acbafe7b12a34
2make
3python3 test/functional/wallet_taproot.py
4...
5test_framework.authproxy.JSONRPCException: 'sendtoaddress' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
Test previous commit:
0git checkout HEAD~1
1git cherry-pick 387c12e0813a863bc9728777719acbafe7b12a34
2make
3python3 test/functional/wallet_taproot.py
4...
52023-12-15T20:46:16.689000Z TestFramework (INFO): Tests successful
389@@ -387,6 +390,14 @@ def do_test_psbt(self, comment, pattern, privmap, treefn, keys_pay, keys_change)
390 psbt_online.unloadwallet()
391 psbt_offline.unloadwallet()
392
393+ def do_test_k_of_n(self, k, n):
394+ self.do_test(
395+ f"tr(XPUB,multi_a({k},XPRV_1,...,XPRV_N)",
...,XPRIV_{n}
