Fix misleading error message: Clean stack rule

sanket1729 commented at 9:49 pm on September 23, 2020: contributor

Error messages in clean stack is misleading as it lets the user believe that there are extra elements on the stack which is incorrect if the stack is empty.

Let me know if this requires additional test.

DrahtBot added the label Consensus on Sep 23, 2020

darosior approved

darosior commented at 11:59 am on September 24, 2020: member

ACK c2f00b7e2f6c17083b7c72555f5a366c9ff8c673

Agree that this is misleading.

glozow commented at 12:02 pm on September 24, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/20006/commits/c2f00b7e2f6c17083b7c72555f5a366c9ff8c673 An empty stack is a SCRIPT_ERR_EVAL_FALSE, yes?

darosior commented at 12:05 pm on September 24, 2020: member

An empty stack is a SCRIPT_ERR_EVAL_FALSE, yes?

No, it’s SCRIPT_ERR_CLEANSTACK.

That’s actually the point of this change :) Relevant line: https://github.com/bitcoin/bitcoin/blob/1b313cacc99a1b372238f9036abed5491f9d28f7/src/script/interpreter.cpp#L1519-L1522

glozow commented at 12:19 pm on September 24, 2020: member

@darosior I meant to clarify that, if we want a clean stack (i.e. script flag SCRIPT_VERIFY_CLEANSTACK), we want to end up with 1 item exactly. We never want 0 items, as that would be a SCRIPT_ERR_EVAL_FALSE. I wouldn’t ACK without thinking there’s a point :)

in src/script/script_error.cpp:79 in c2f00b7e2f outdated

75@@ -76,7 +76,7 @@ std::string ScriptErrorString(const ScriptError serror)
76         case SCRIPT_ERR_PUBKEYTYPE:
77             return "Public key is neither compressed or uncompressed";
78         case SCRIPT_ERR_CLEANSTACK:
79-            return "Extra items left on stack after execution";
80+            return "Stack size must be one after execution";

instagibbs commented at 1:38 pm on September 24, 2020:

since this is trivial change anyways, maybe “must be exactly one”, somehow reads better to me

sanket1729 commented at 5:34 pm on September 24, 2020:

Agreed. This sounds better.

instagibbs approved

instagibbs commented at 1:39 pm on September 24, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/20006/commits/c2f00b7e2f6c17083b7c72555f5a366c9ff8c673

theStack commented at 2:04 pm on September 24, 2020: member

ACK https://github.com/bitcoin/bitcoin/commit/c2f00b7e2f6c17083b7c72555f5a366c9ff8c673

An empty stack is a SCRIPT_ERR_EVAL_FALSE, yes?

That was also my initial thought. After digging deeper into the VerifyScript() function though I think this is only true for native txs (w/o SegWit). With SegWit involved, in the following part, in the function ExecuteWitnessScript() it could happen that after running the witness script the stack is empty, but still SCRIPT_ERR_CLEANSTACK is thrown:

https://github.com/bitcoin/bitcoin/blob/1b313cacc99a1b372238f9036abed5491f9d28f7/src/script/interpreter.cpp#L1516-L1522

Don’t know though if that is intended or it was just forgotten to implicitely check for empty and return SCRIPT_ERR_EVAL_FALSE in this place. (For practical matters it shouldn’t make a difference, in any case the script fails).

Fix misleading error message: Clean stack rule

Error messages in cleanstack is misleading as
it lets the user believe that there are extra
elements on stack which is incorrect if the
stack is empty.

af57766182

sanket1729 force-pushed on Sep 24, 2020

sipa commented at 5:38 pm on September 24, 2020: member

For context, I believe the current behavior is:

For non-segwit spends, SCRIPT_ERR_CLEANSTACK is only returned when SCRIPT_VERIFY_CLEANSTACK is enabled, and refers strictly to stacks larger than 1 (because if the size was 0, SCRIPT_ERR_EVAL_FALSE would have been triggered already).
For segwit spends, SCRIPT_ERR_CLEANSTACK is returned for any script size different from exactly 1, and doesn’t need the SCRIPT_VERIFY_CLEANSTACK flag.

I’m ok with this PR as is, but perhaps it’s more insightful to split up the two cases entirely, and give them separate error codes?

instagibbs commented at 5:45 pm on September 24, 2020: member

@sipa if we do that might as well do minimalif too and any others similar

sipa commented at 5:46 pm on September 24, 2020: member

@instagibbs I don’t think there are any others like that currently. Splitting out the consensus-defined MINIMALIF behavior in #19953 into a separate error code does make sense.

instagibbs commented at 5:53 pm on September 24, 2020: member

filed an issue in case someone wants to do it later(or sanket now if he wants)

sanket1729 commented at 7:39 pm on September 24, 2020: contributor

@theStack, yup I faced when evaluating Segwit script.

I can take on #20009. I think still PR is conceptually independent, so I will keep this open. Let me know if I should close this and address this issue along with the separation of Errors.

theStack approved

theStack commented at 1:36 pm on September 25, 2020: member

re-ACK af57766182013e17c23245671a33463f754ccd28

instagibbs commented at 1:40 pm on September 25, 2020: member

re-ACK https://github.com/bitcoin/bitcoin/pull/20006/commits/af57766182013e17c23245671a33463f754ccd28

darosior approved

darosior commented at 1:42 pm on September 25, 2020: member

re ACK af57766182013e17c23245671a33463f754ccd28

glozow commented at 2:32 pm on September 26, 2020: member

reACK https://github.com/bitcoin/bitcoin/commit/af57766182013e17c23245671a33463f754ccd28

laanwj merged this on Sep 30, 2020

laanwj closed this on Sep 30, 2020

sidhujag referenced this in commit ccc6548d41 on Sep 30, 2020

sanket1729 referenced this in commit 720b36fb16 on Oct 8, 2020

sanket1729 referenced this in commit b74d4aa30f on Oct 19, 2020

sanket1729 referenced this in commit d81f64cc59 on Dec 2, 2020

sanket1729 referenced this in commit 39987fdb51 on Jan 26, 2021

Fabcien referenced this in commit e6676216b7 on Nov 3, 2021

DrahtBot locked this on Feb 15, 2022

Fix misleading error message: Clean stack rule #20006