won't this pass if no exception is raised?
Shouldn't we add something like either:

try:
    self.do_test_k_of_n(1, 1000)
    assert False, "Expected exception was not raised for 1-of-1000 test"
except JSONRPCException as e:

or via unittest:

with self.assertRaises(JSONRPCException) as cm:
    self.do_test_k_of_n(1, 1000)
self.assertEqual(cm.exception.error["code"], -5)
self.assertIn("Cannot have 1000 keys in multi_a; must have between 1 and 999 keys, inclusive", cm.exception.error["message"])

?

Thanks for pointing this out 🙏 I agree it shouldn't be possible for this test to succeed so an exception should be raised.

assert_raises_rpc_error is probably what you want

@paplorinc I like the second approach, it seems a bit cleaner and clearer to me. However, it doesn't seem to be used in other functional tests so maybe the first approach is better to be more consistent?

@instagibbs Thanks! I was looking at that option too, however I couldn't see a way to use it in this context without having to change a lot of the code, but maybe I'm missing something obvious? 🤔

if something should raise an error and a specific code/message, you use assert_raises_rpc_error. Just take a look on how it's used elsewhere?

# Test that 1-of-1000 and 999-of-1000 raise exception
for k in [1, 999]:
    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:

for k in [0, 1, 999, 1000]:

if that makes sense.

I much appreciate the input @paplorinc, this was the syntax I was looking for 🙏 I saw the first version of your message and immediately thought about doing the other part ([0, 1, 999, 1000]). You read my mind, this makes very much sense! Also, doing [1, 999] would be quite time consuming because each run lasts about a second so in total it would need 1000 seconds to complete. Nvm, I was thinking about testing the whole range when in fact it's just two tests. I'll update the PR to support the version with four tests.

@Sjors Given my basic understanding of output descriptors, I'd much appreciate your thoughts on this block of code, to make sure it meets the issue criterias and if this is a good way to test it.

I understand descriptors, but Python less well :-) What is this function trying to do?

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

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.

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.

I guess that would do the trick, however I'm unsure if by doing so we modify the test to test the assertion, which seems a bit incomplete/unintuitive to me, rather than testing the edge cases in k-of-N wallets? 🤔 If you understand what I mean 😄 If you don't think it's an issue, I'm all for keeping it simple and just test 1-of-1000.

Well the "edge" here is n = 999, but this test is varying k, not n.

Ah you're right. I'll simplify the test to only do 1-of-1000.

Would it make sense to test the other edge, 1-of-0?

Why are you changing the fee rate?

If I didn't bump the fee rate I would run into the two error codes mentioned in the PR message:

Had to bump fee rate to resolve assertions in sendtoaddress and psbt methods:

    code -5: assert rpc_online.gettransaction(txid)["confirmations"] > 0
    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.

Fee rate is the fee per vbyte, so the fee goes up as the script size goes up. But this may be related to #26573

Slightly unrelated: are there any tests that check multisig with different keys rather than repeating the same one, to make it more representative of a real-world scenario?
I'm just getting familiar with the codebase, I'm not sure whether it's important or not.

I think it's very related and also my concern, is this test as good as one with K different keys. I'm hoping @Sjors can provide some insight here. My understanding of the other tests in this file is limited because they are not documented, but they seem to be doing something similar to this. On the other hand I could of course do what the previous test did, to use the "H_POINT" approach and just throw one XPRV in there somewhere. If that makes the test more real-world like or improves the test in some other way I'm all for it.

Testing different keys makes sense, but let's keep this PR simple and leave it to a followup.

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.

Thanks for the comments. 🙏 I added the comma, rebased onto master and force pushed to trigger a new build to get the mac CI through.

Testing different keys makes sense, but let's keep this PR simple and leave it to a followup.

Note to other reviewers: this privmap argument determines for which keys an xpriv is inserted into the descriptor.

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:

// TODO: fix or work around performance bottleneck in `sendtoaddress`
// 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?

I'll lower it to 100 unless @achow101 thinks the 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?

        # Test 100-of-100 
        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)

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 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 a git cherry-pick ....

Git bisecting this I found that the tests start timing out with commit 4f473ea

Are you able to verify this on your setup?

@darosior would you be able to check as well? 🙏

This is how I did it:

git checkout 4f473ea515bc77b9138323dab8a741c063d32e8f
git cherry-pick 387c12e0813a863bc9728777719acbafe7b12a34
make
python3 test/functional/wallet_taproot.py
...
test_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:

git checkout HEAD~1
git cherry-pick 387c12e0813a863bc9728777719acbafe7b12a34
make
python3 test/functional/wallet_taproot.py
...
2023-12-15T20:46:16.689000Z TestFramework (INFO): Tests successful

Great find! Let's track this in a separate issue: #29098

...,XPRIV_{n}