Fix misleading error message: Clean stack rule #20006

pull sanket1729 wants to merge 1 commits into bitcoin:master from sanket1729:clean_stk changing 1 files +1 −1
  1. 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.

  2. DrahtBot added the label Consensus on Sep 23, 2020
  3. darosior approved
  4. darosior commented at 11:59 AM on September 24, 2020: member

    ACK c2f00b7e2f6c17083b7c72555f5a366c9ff8c673

    Agree that this is misleading.

  5. 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?

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

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

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

  9. instagibbs approved
  11. 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).

  12. 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
  13. sanket1729 force-pushed on Sep 24, 2020
  14. 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?

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

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

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

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

  19. theStack approved
  20. theStack commented at 1:36 PM on September 25, 2020: member

    re-ACK af57766182013e17c23245671a33463f754ccd28

  22. darosior approved
  23. darosior commented at 1:42 PM on September 25, 2020: member

    re ACK af57766182013e17c23245671a33463f754ccd28

  25. laanwj merged this on Sep 30, 2020
  26. laanwj closed this on Sep 30, 2020
  27. sidhujag referenced this in commit ccc6548d41 on Sep 30, 2020
  28. sanket1729 referenced this in commit 720b36fb16 on Oct 8, 2020
  29. sanket1729 referenced this in commit b74d4aa30f on Oct 19, 2020
  30. sanket1729 referenced this in commit d81f64cc59 on Dec 2, 2020
  31. sanket1729 referenced this in commit 39987fdb51 on Jan 26, 2021
  32. Fabcien referenced this in commit e6676216b7 on Nov 3, 2021
  33. DrahtBot locked this on Feb 15, 2022
Contributors
sanket1729darosiorglozowinstagibbstheStacksipa
Labels
Consensus
Linked (view graph)
#20009 split policy/error consensus codes for CLEANSTACK, MINAMALIF#20100 Script: split policy/error consensus codes for CLEANSTACK, MINIMALIF
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-26 03:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me