Inspired by discussion in #31640 (comment) I made an example adding coverage I think is missing, with some extra commentary that might help future contributors (including myself when I forget how it works again).
Open for suggestions how we can make it more welcoming beyond this.
cc darosior EthanHeilman sipa
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32114.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept ACK
Not sure about having the documentation to add new spenders mixed within this particular set of spenders. What if I want to add new spenders Maybe it would be better in a comment block at the top of the test or in the docstring of spenders_taproot_active?
651@@ -652,6 +652,30 @@ def spenders_taproot_active():
652
653 spenders = []
654
655+ # === To add a spender: ===
I agree with this comment #32114 (comment) that commentary like this can be put in a more “generic” place like the Spender tuple declaration here: https://github.com/bitcoin/bitcoin/blob/770d39a37652d40885533fecce37e9f71cc0d051/test/functional/feature_taproot.py#L465-L480
Something that lets the reader/tester quickly know in a generic way on how to use the Spender tuple, i.e.
One more (smaller) reason to avoid adding here is that the above flow is also used in the spenders_taproot_nonstandard function, someone updating that function (although rarely) might miss the commentary here.
True. I mentioned it because one would end up looking at it once they see make_spender. This place would be more useful if the Spender type was also used more in the test such as while initialising the spenders list.
I find right at the top a suitable alternative as well though I missed this section entirely the first time I read the test: https://github.com/bitcoin/bitcoin/blob/770d39a37652d40885533fecce37e9f71cc0d051/test/functional/feature_taproot.py#L121
Fine either way.
Open for suggestions how we can make it more welcoming beyond this.
One point I want to mention is that initially it was not evident to me that a spender is a test case in itself until I saw err_msg being a part of it. Technically this is mentioned in the file albeit in a buried location: https://github.com/bitcoin/bitcoin/blob/770d39a37652d40885533fecce37e9f71cc0d051/test/functional/feature_taproot.py#L1330
A more elaborate name like spender_test_case or spender_tc would be useful but Idk if it’s feasible to incorporate a relatively bigger change like this now. If not, then maybe highlighting this comment by extracting it out could be useful as well.
Concept ACK
Putting in my 2 sats here because I recently had the opportunity to go through this test couple weeks back. Although it took me some time to understand the overall structure of the test but I was able to appreciate it soon after.
@rkrux pushed an update with another related direction. Ideally a new contributor can add a set of spenders under a new function, aggregate to the list, without having to touch the current walls of current spender making functions?
Let me know if I’m getting warmer.
Nice, I like this approach. In addition to making it easier to onboard by adding the commentary, it also helps in not increasing the size of the spenders_taproot_active, which is more than 500 lines long already.
I will look at it in detail soon.
1229@@ -1230,6 +1230,41 @@ def spenders_taproot_nonstandard():
1230
1231 return spenders
1232
1233+def sample_spenders():
1234+
1235+ # Create key(s) for output creation, as well as key and script-spends
1236+ secs = [generate_privkey() for _ in range(2)]
1237+ pubs = [compute_xonly_pubkey(sec)[0] for sec in secs]
1229@@ -1230,6 +1230,41 @@ def spenders_taproot_nonstandard():
1230
1231 return spenders
1232
1233+def sample_spenders():
# === Actual test cases ===. I worry people might not scroll down the 600 lines to find this. It is a good introduction to the rest of the tests
run_test() from where they can navigate to the implementation of sample_spenders().
