test: fix the unreachable code at feature_taproot

[brunoerg]

This PR removes the unnecessary return statement at the beginning of the function that makes the rest of the function unreachable.

[michaelfolkson]

Concept ACK from #20354 (comment)

There should only be one commit @brunoerg. There are currently three. You should work on top of latest version of master to avoid these merge commits but you may be able to resolve this by squashing commits

[michaelfolkson]

I don't know if you need this @brunoerg but if you do you can do a reset on your branch to the current top commit on master, then re-adding your commit and force pushing.

git reset -hard insert_current_top_commit_hash_on_master

Then add a single commit on top and force pushing. @fanquake helped me a while back with that :)

[brunoerg]

Thanks a lot! It helped me! @michaelfolkson

[theStack]

Concept ACK, almost approach ACK
I'd suggest to be a bit more verbose in the commit subject line (you could e.g. simply use the PR title).

[maflcko]

Removed "refactor"

[practicalswift]

Another nice catch @brunoerg!

And another case of "how the heck did this pass our peer review"? :)

Given enough eyeballs, all bugs are shallow.

Nice to have your eyeballs joining us! Again: warm welcome! :)

[practicalswift]

Got so excited that I missed my actual review comment! :D

cr ACK 5e0cd25: patch looks correct!

[maflcko]

If this is merged we'll have to regenerate the unit tests, I presume?

[flack]

And another case of "how the heck did this pass our peer review"? :)

I'm more surprised that this wasn't caught by some linter. Or is unreachable code detection in Python just not reliable enough to be useful?

[maflcko]

Or is unreachable code detection in Python just not reliable enough to be useful?

See f4beb49. Maybe things have changes since then?

[theStack]

Tested ACK 5e0cd25 🏔️
Ran feature_taproot.py multiple (>10) times on the PR branch to verify that it succeeds with all code paths in random_checksig_style.

If this is merged we'll have to regenerate the unit tests, I presume?

Seems reasonable. I couldn't find any taproot unit tests in this repo, but I suppose you mean https://github.com/bitcoin-core/qa-assets/blob/master/unit_test_data/script_assets_test.json

[sanket1729]

tACK 5e0cd25. I noted this a while ago while fixing feature_taproot.py for elements. Verified that the extreme ranges of CScriptNum are correct and the overflow case for CHECKSIGADD works as intended. Adding 1 to 2^31 - 1 results in an overflow, but the interpreter puts a vch of corresponding to 2^31 on stack. Even though it cannot be converted to CscriptNum(restricted to 4 bytes), it's result can still be compared by OP_EQUAL.

There are also some control flow warnings that I discovered. I was not sure if they're worth fixing because this was test code.

All of them are of the form:

for ...
   # declare var in loop
   var = 
# use var outside loop.
f(var)

instances: hashtype and common in

bitcoin/test/functional/feature_taproot.py

Lines 668 to 672 in 6c61408

	add_spender(spenders, "sighash/annex", tap=tap, leaf="pk_codesep", key=secs[1], hashtype=hashtype, standard=False, **SINGLE_SIG, annex=bytes([ANNEX_TAG]), failure={"sighash": override(default_sighash, annex=None)}, **ERR_SIG_SCHNORR)
	add_spender(spenders, "sighash/script", tap=tap, leaf="pk_codesep", key=secs[1], **common, **SINGLE_SIG, failure={"sighash": override(default_sighash, script_taproot=tap.leaves["codesep_pk"].script)}, **ERR_SIG_SCHNORR)
	add_spender(spenders, "sighash/leafver", tap=tap, leaf="pk_codesep", key=secs[1], **common, **SINGLE_SIG, failure={"sighash": override(default_sighash, leafversion=random.choice([x & 0xFE for x in range(0x100) if x & 0xFE != 0xC0]))}, **ERR_SIG_SCHNORR)
	add_spender(spenders, "sighash/scriptpath", tap=tap, leaf="pk_codesep", key=secs[1], **common, **SINGLE_SIG, failure={"sighash": override(default_sighash, leaf=None)}, **ERR_SIG_SCHNORR)
	add_spender(spenders, "sighash/keypath", tap=tap, key=secs[0], **common, failure={"sighash": override(default_sighash, leaf="pk_codesep")}, **ERR_SIG_SCHNORR)

pubkey1 and eckey1 here:

bitcoin/test/functional/feature_taproot.py

Line 1117 in 6c61408

add_spender(spenders, "compat/nocsa", hashtype=hashtype, p2sh=p2sh, witv0=witv0, standard=standard, script=CScript([OP_IF, OP_11, pubkey1, OP_CHECKSIGADD, OP_12, OP_EQUAL, OP_ELSE, pubkey1, OP_CHECKSIG, OP_ENDIF]), key=eckey1, sigops_weight=4-3*witv0, inputs=[getter("sign"), b''], failure={"inputs": [getter("sign"), b'\x01']}, **ERR_UNDECODABLE)

[sanket1729]

If this is merged we'll have to regenerate the unit tests, I presume?

I don't follow this. Is a JSON dump for these used elsewhere?

[sipa]

All of them are of the form:

That's allowed in Python. All variables are global or function scope; there is no scope associated with individual code blocks.

I don't follow this. Is a JSON dump for these used elsewhere?

Yes, there is a dump extracted & minimized from the Python tests, that runs as a unit test (see src/test/script.cpp's script_asset_tests).

If this is merged we'll have to regenerate the unit tests, I presume?

No need to start over, but we should use the additional output generated from this to add more cases to the minimized dump. I'll do so if nobody else does.

[sipa]

ACK 5e0cd25.

I think this is a leftover from debugging...

[maflcko]

Done in bitcoin-core/qa-assets#46