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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32114.
<!--021abf342d371248e50ceaed478a90ca-->
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.
Nice. Concept ACK on having an example there.
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.
I find that particular code block really tucked away and hard to find fwiw.
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.
@fjahr I could do that, where the comment is literal examples that aren't an attempt to expand coverage. I think seeing it right at the top could be helpful
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.
The new design also allows for using different keys and pubs in different tests instead of using one set in all the tests. The overall commentary looks great, the commits can be cleaned up.
@rkrux done thanks
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]
Why are two secs and two pubs created if only one is used? Is to show a pattern of generating more than one so other people can add additional tests to this?
yep, don't feel strongly on this one but that's probably an expected pattern
1229 | @@ -1230,6 +1230,41 @@ def spenders_taproot_nonstandard(): 1230 | 1231 | return spenders 1232 | 1233 | +def sample_spenders():
This is helpful for understanding how to write these tests. I might make sense to put this as the first testcase just below # === 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
Seems like a matter of taste; leaving as-is for now?
Though a practical suggestion but I think it can be left as-is. It's highly likely that the reader would go through the run_test() from where they can navigate to the implementation of sample_spenders().
ut_ack
Code review ACK 3dd29b12e1931147145251944d4a1b733be841f9
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.
can you give a concrete suggestion with a diff?
tried being a little more explicit, but I'm punting larger refactors for Future Work
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.
# Spender that is non-standard but otherwise valid, with extraneous signature data from inner key for optional failure condition
+ '''
+ The arg "key" is internally used to sign the inputs, refer the defaults set in the `make_spender` & `spend` fns.
+ The default general flow of the callables for taproot inputs is like below (varies slightly for keypath/scriptpath spend):
+ default_witness -> default_witness_taproot -> default_sign -> default_signature -> default_key_tweaked
+ '''
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.
git 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.
I could put quotes around the existing "key" comment to make it clearer it's referring to the arg, if that helps?
Yes, I believe that could be helpful to some extent.
Sorry if these comments are coming in a bit late.
ACK f974359e218edc3f31685015e234d00243a79452
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:
diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py
index 68c2c2e501..33dbc8f334 100755
--- a/test/functional/feature_taproot.py
+++ b/test/functional/feature_taproot.py
@@ -1238,12 +1238,12 @@ def sample_spenders():
pubs = [compute_xonly_pubkey(sec)[0] for sec in secs]
# Create a list of scripts which will be built into a taptree
scripts = [
# leaf label, followed by CScript
- ("encodeable_pushdata1", CScript([OP_DROP, OP_PUSHDATA1, b'aa' * 75])),
- ("nonstd_encodeable_pushdata1", CScript([OP_PUSHDATA1, b'aa'])),
+ ("encodeable_pushdata1", CScript([OP_DROP, b'\xaa\xaa'])),
+ ("nonstd_encodeable_pushdata1", CScript.fromhex(f"{OP_PUSHDATA1:x}{2:02x}aaaa")),
("dummyleaf", CScript([])),
]
# Build TaprootInfo using scripts and appropriate pubkey for output creation
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?
took your suggestion and pushed a fixup PR