Fix misleading error message: Clean stack rule by sanket1729 · Pull Request #20006 · bitcoin/bitcoin

sanket1729 · 2020-09-23T21:49:36Z

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.

darosior

ACK c2f00b7e2f6c17083b7c72555f5a366c9ff8c673

Agree that this is misleading.

glozow

ACK c2f00b7
An empty stack is a SCRIPT_ERR_EVAL_FALSE, yes?

darosior · 2020-09-24T12:05:47Z

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:

bitcoin/src/script/interpreter.cpp

Lines 1519 to 1522 in 1b313ca

    
           // Scripts inside witness implicitly require cleanstack behaviour 
        
           if (stack.size() != 1) return set_error(serror, SCRIPT_ERR_CLEANSTACK); 
        
           if (!CastToBool(stack.back())) return set_error(serror, SCRIPT_ERR_EVAL_FALSE); 
        
           return true;

glozow · 2020-09-24T12:19:11Z

@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 :)

instagibbs

ACK c2f00b7

instagibbs · 2020-09-24T13:38:45Z

src/script/script_error.cpp

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

Agreed. This sounds better.

theStack · 2020-09-24T14:04:07Z

ACK c2f00b7

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:

bitcoin/src/script/interpreter.cpp

Lines 1516 to 1522 in 1b313ca

    
           // Run the script interpreter. 
        
           if (!EvalScript(stack, scriptPubKey, flags, checker, sigversion, serror)) return false; 
        
           // Scripts inside witness implicitly require cleanstack behaviour 
        
           if (stack.size() != 1) return set_error(serror, SCRIPT_ERR_CLEANSTACK); 
        
           if (!CastToBool(stack.back())) return set_error(serror, SCRIPT_ERR_EVAL_FALSE); 
        
           return true;

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

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.

sipa · 2020-09-24T17:38:34Z

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 · 2020-09-24T17:45:34Z

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

sipa · 2020-09-24T17:46:43Z

@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 · 2020-09-24T17:53:12Z

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

sanket1729 · 2020-09-24T19:39:20Z

@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

re-ACK af57766

instagibbs · 2020-09-25T13:40:25Z

re-ACK af57766

darosior

re ACK af57766

glozow · 2020-09-26T14:32:43Z

reACK af57766

af57766 Fix misleading error message: Clean stack rule (sanket1729) Pull request description: 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. ACKs for top commit: instagibbs: re-ACK bitcoin@af57766 gzhao408: reACK bitcoin@af57766 theStack: re-ACK af57766 darosior: re ACK af57766 Tree-SHA512: 88e77416e220b080246fec368f5552a891d102d072b7bee62ac560d5e31c4a8c2ee9cbe569740b253e9df177d21dc788d10d856b2a542ab47761bb81698e4082

CleanStack rule is consensus enforced in Segwitv0. See discussion in bitcoin#20006 for motivation

Summary: 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. This is a backport of [[bitcoin/bitcoin#20006 | core#20006]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10411

DrahtBot added the Consensus label Sep 23, 2020

darosior approved these changes Sep 24, 2020

View reviewed changes

glozow reviewed Sep 24, 2020

View reviewed changes

instagibbs approved these changes Sep 24, 2020

View reviewed changes

Fix misleading error message: Clean stack rule

af57766

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.

sanket1729 force-pushed the clean_stk branch from c2f00b7 to af57766 Compare September 24, 2020 17:37

instagibbs mentioned this pull request Sep 24, 2020

split policy/error consensus codes for CLEANSTACK, MINAMALIF #20009

Closed

theStack approved these changes Sep 25, 2020

View reviewed changes

darosior approved these changes Sep 25, 2020

View reviewed changes

laanwj merged commit 4f5ae52 into bitcoin:master Sep 30, 2020

sipa mentioned this pull request Oct 2, 2020

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #19953

Merged

sanket1729 mentioned this pull request Oct 8, 2020

Script: split policy/error consensus codes for CLEANSTACK, MINIMALIF #20100

Closed

sanket1729 added a commit to sanket1729/bitcoin that referenced this pull request Jan 26, 2021

Separate error code for Segwit CleanStack

39987fd

CleanStack rule is consensus enforced in Segwitv0. See discussion in bitcoin#20006 for motivation

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

sanket1729 commented Sep 23, 2020

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

darosior commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Sep 24, 2020

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

sanket1729 Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

theStack commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Sep 24, 2020

Uh oh!

instagibbs commented Sep 24, 2020

Uh oh!

sipa commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Sep 24, 2020

Uh oh!

sanket1729 commented Sep 24, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Sep 25, 2020

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Sep 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

darosior commented Sep 24, 2020 •

edited

Loading

theStack commented Sep 24, 2020 •

edited

Loading

sipa commented Sep 24, 2020 •

edited

Loading