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().
1258+
1259+ # Spender with alternative failure tapscript via dictionary, along with expected err_msg / ERR_*
1260+ add_spender(spenders, comment="tutorial/pushdata1redux", tap=tap, leaf="encodeable_pushdata1", inputs=[b'\x00'], failure={"leaf": "dummyleaf"}, **ERR_NO_SUCCESS)
1261+
1262+ # Spender that is non-standard but otherwise valid, with extraneous signature data from inner key for optional failure condition
1263+ add_spender(spenders, comment="tutorial/nonminpushdata1", tap=tap, leaf="nonstd_encodeable_pushdata1", key=secs[0], standard=False, failure={"inputs": [getter("sign")]}, **ERR_CLEANSTACK)
The naming of the failure variable is unintuitive imo because the first time I read it, I assumed this is some returned failure object from some operation undergoing the test but that is not the case as per this comment in the make_spender().
At the cost of being verbose, it could be called failure_overrides or something similar because seeing around ~90 odd occurrences of this argument in the file does add some cognitive load for the reader over time.
However, this kind of change will increase the diff a bit and I can understand if the author is not inclined towards it in which case the commentary here can include the above mentioned context. I do see the term “failure condition” mentioned here already, maybe tying it up with the “failure” object could prove to be helpful.
A note here on the key object could be helpful because it appears to be a commonly used arg as there are around ~75 occurrences of this object in this file. Moreover, it’s not even a named argument in the make_spender() but a passthrough. The only mention of it is here:
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/test/functional/feature_taproot.py#L425
An indirect mention is here: https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/test/functional/feature_taproot.py#L443-L450
I believe the commentary in this function (with a note on key) will make it easier for the reader.
Yes, this is a bit more than what I originally intended but I find documenting the relationship b/w the callables helpful here instead of having to navigate it again when I look at this test after a while.
0 # Spender that is non-standard but otherwise valid, with extraneous signature data from inner key for optional failure condition
1+ '''
2+ The arg "key" is internally used to sign the inputs, refer the defaults set in the `make_spender` & `spend` fns.
3+ The default general flow of the callables for taproot inputs is like below (varies slightly for keypath/scriptpath spend):
4+ default_witness -> default_witness_taproot -> default_sign -> default_signature -> default_key_tweaked
5+ '''
6 add_spender(spenders, comment="tutorial/nonminpushdata1", tap=tap, leaf="nonstd_encodeable_pushdata1", key=secs[0], standard=False, failure={"inputs": [getter("sign")]}, **ERR_CLEANSTACK)
Ty, the diff here seems good enough to me.
0git range-diff 3dd29b1...f974359
I’d rather the PR focus on “how to add new cases” vs documenting call traces.
Is there a test case that would have been enlightening how it works, making the underlying code more greppable perhaps?
I’d rather the PR focus on “how to add new cases” vs documenting call traces.
That’s a fair point.
Is there a test case that would have been enlightening how it works, making the underlying code more greppable perhaps?
IIUC this^ question: The very need of this question was the idea behind my suggesting the above diff. The newly added spender case is very much like the existing cases and currently I have not spent enough time in this file to suggest any other case (if there is one). If makes sense the first line in the above diff can be added otherwise I guess the current state of the pull is also good enough for me to ack.
1239+ # Create a list of scripts which will be built into a taptree
1240+ scripts = [
1241+ # leaf label, followed by CScript
1242+ ("encodeable_pushdata1", CScript([OP_DROP, OP_PUSHDATA1, b'aa' * 75])),
1243+ ("nonstd_encodeable_pushdata1", CScript([OP_PUSHDATA1, b'aa'])),
1244+ ("dummyleaf", CScript([])),
I’m a bit confused by what these scripts are intended to be? CScript already does pushdata encoding aiui, so this results in:
Is that intended? It could make sense to just write out the scripts in hex, and use CScript.fromhex(...), eg:
CScript.fromhex("%02x%02x%s" % (OP_PUSHDATA1, 76, "aa"*76)) – minimal push of 76 bytes each 0xaa, same as CScript([b'\xaa'*76])CScript.fromhex("%02x%02x%s" % (OP_PUSHDATA1, 2, b"aa") – non-minimal push of 2 bytes, each 0xaa, different to CScript([b'\xaa\xaa']) which gives a minimal push
definitely not intended. how about this:
0diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py
1index 68c2c2e501..33dbc8f334 100755
2--- a/test/functional/feature_taproot.py
3+++ b/test/functional/feature_taproot.py
4@@ -1238,12 +1238,12 @@ def sample_spenders():
5 pubs = [compute_xonly_pubkey(sec)[0] for sec in secs]
6
7 # Create a list of scripts which will be built into a taptree
8 scripts = [
9 # leaf label, followed by CScript
10- ("encodeable_pushdata1", CScript([OP_DROP, OP_PUSHDATA1, b'aa' * 75])),
11- ("nonstd_encodeable_pushdata1", CScript([OP_PUSHDATA1, b'aa'])),
12+ ("encodeable_pushdata1", CScript([OP_DROP, b'\xaa\xaa'])),
13+ ("nonstd_encodeable_pushdata1", CScript.fromhex(f"{OP_PUSHDATA1:x}{2:02x}aaaa")),
14 ("dummyleaf", CScript([])),
15 ]
16
17 # Build TaprootInfo using scripts and appropriate pubkey for output creation
18 tap = taproot_construct(pubs[0], scripts)
[OP_DROP, b'\xaa\xaa'] isn’t pushdata1, it’s just a direct two-byte push after a DROP? Perhaps that just means the name should change though? "nonstd_2byte_pushdata1": CScript.fromhex("4c02aaaa") might be clearer/simpler for the second one?