test: fix the unreachable code at feature

brunoerg commented at 12:45 pm on February 4, 2021: member

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

fix the unreachable code at feature_taproot 5e0cd25e29

michaelfolkson commented at 1:10 pm on February 4, 2021: contributor

Concept ACK from #20354 (review)

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

fanquake added the label Tests on Feb 4, 2021

michaelfolkson commented at 1:23 pm on February 4, 2021: contributor

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 force-pushed on Feb 4, 2021

brunoerg commented at 1:54 pm on February 4, 2021: member

Thanks a lot! It helped me! @michaelfolkson

theStack commented at 1:56 pm on February 4, 2021: member

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).

brunoerg force-pushed on Feb 4, 2021

MarcoFalke renamed this:
~~test, refactor: fix the unreachable code at feature_taproot~~
test: fix the unreachable code at feature_taproot
on Feb 4, 2021

MarcoFalke commented at 5:27 pm on February 4, 2021: member

Removed “refactor”

practicalswift commented at 8:39 am on February 5, 2021: contributor

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 commented at 8:43 am on February 5, 2021: contributor

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

cr ACK 5e0cd25e29541e6c19559fb5c2555e008ed896fa: patch looks correct!

MarcoFalke commented at 9:00 am on February 5, 2021: member

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

flack commented at 11:09 am on February 5, 2021: contributor

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?

MarcoFalke commented at 3:55 pm on February 5, 2021: member

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

See f4beb4996d27f2cdaf4f0a63e7dc044bf17decce. Maybe things have changes since then?

theStack approved

theStack commented at 10:07 pm on February 6, 2021: member

Tested ACK 5e0cd25e29541e6c19559fb5c2555e008ed896fa 🏔️ 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 approved

sanket1729 commented at 0:55 am on February 8, 2021: contributor

tACK 5e0cd25e29541e6c19559fb5c2555e008ed896fa. 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:

0for ...
1   # declare var in loop
2   var = 
3# use var outside loop.
4f(var)

instances: hashtype and common in https://github.com/bitcoin/bitcoin/blob/6c6140846f37de8c132b3b6abf09f3d7940554a7/test/functional/feature_taproot.py#L668-L672 pubkey1 and eckey1 here: https://github.com/bitcoin/bitcoin/blob/6c6140846f37de8c132b3b6abf09f3d7940554a7/test/functional/feature_taproot.py#L1117

sanket1729 commented at 1:05 am on February 8, 2021: contributor

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 commented at 1:14 am on February 8, 2021: member

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 commented at 1:15 am on February 8, 2021: member

ACK 5e0cd25e29541e6c19559fb5c2555e008ed896fa.

I think this is a leftover from debugging…

fanquake merged this on Feb 8, 2021

fanquake closed this on Feb 8, 2021

MarcoFalke referenced this in commit 4607019798 on Feb 8, 2021

MarcoFalke commented at 9:28 am on February 8, 2021: member

Done in https://github.com/bitcoin-core/qa-assets/pull/46

sidhujag referenced this in commit c694f572c4 on Feb 8, 2021

MarcoFalke referenced this in commit d19639d2b6 on Feb 15, 2021

sidhujag referenced this in commit 41bc01f866 on Feb 15, 2021

brunoerg deleted the branch on Feb 16, 2021

UdjinM6 referenced this in commit 68f62d4e93 on Sep 10, 2021

UdjinM6 referenced this in commit 31b4209318 on Sep 24, 2021

UdjinM6 referenced this in commit 5071a8070b on Oct 4, 2021

UdjinM6 referenced this in commit dbfa92990a on Oct 5, 2021

kittywhiskers referenced this in commit 8949ce83f8 on Oct 12, 2021

pravblockc referenced this in commit 6872c4a44d on Nov 18, 2021

DrahtBot locked this on Aug 16, 2022

test: fix the unreachable code at feature_taproot #21081