consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script #29589

Christewart commented at 4:47 PM on March 7, 2024: contributor

This PR fixes inconsistent behavior for encoding/decoding small integers in Script discovered when working on #29221.

OP_1NEGATE is the minimally encoded version of the number -1 in Script. All other minimally encoded small integers (OP_0, OP_1,...,OP_16) are handled correctly by DecodeOP_N() and EncodeOP_N().

This PR extends this functionality to handling OP_1NEGATE in the same way we treat other small integers in Script.

We treat all small integers consistently in EvalScript() - so we should treat their encoding and decoding helper functions consistently as well. It doesn't make sense to handle OP_1NEGATE separately imo.

This is useful for future extensions of the Script language - such as 64bit arithmetic. If this change isn't accepted, we need to adhoc handle OP_1NEGATE separately from other small integers when encoding/decoding OP_1NEGATE.

DrahtBot commented at 4:47 PM on March 7, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29589.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	dgpv, theuni, darosior, ajtowns
Concept ACK	theStack, petertodd
Stale ACK	sipa, vostrnad, itornaza

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Mar 7, 2024

Christewart marked this as ready for review on Mar 7, 2024

in test/functional/test_framework/script.py:71 in 12cdde8fb4 outdated

  66 | @@ -67,11 +67,13 @@ def encode_op_pushdata(d):
  67 |      @staticmethod
  68 |      def encode_op_n(n):
  69 |          """Encode a small integer op, returning an opcode"""
  70 | -        if not (0 <= n <= 16):
  71 | +        if not (-1 <= n <= 16):
  72 |              raise ValueError('Integer must be in range 0 <= n <= 16, got %d' % n)

vostrnad commented at 3:21 AM on March 8, 2024:

            raise ValueError('Integer must be in range -1 <= n <= 16, got %d' % n)

Christewart force-pushed on Mar 8, 2024

Christewart requested review from vostrnad on Mar 8, 2024

vostrnad commented at 2:03 PM on March 8, 2024: none

Should decode_op_n and is_small_int perhaps be fixed as well?

Christewart commented at 2:42 PM on March 8, 2024: contributor

Should decode_op_n and is_small_int perhaps be fixed as well?

Done in 8bd0a8b18d68bbacee731f3e0583397f81ca078e , good catch :raised_hands:

sipa commented at 3:32 PM on March 8, 2024: member

utACK 8bd0a8b18d68bbacee731f3e0583397f81ca078e

DrahtBot removed review request from vostrnad on Mar 8, 2024

Christewart requested review from vostrnad on Mar 8, 2024

in test/functional/test_framework/script.py:75 in 8bd0a8b18d outdated

  72 | +        if not (-1 <= n <= 16):
  73 | +            raise ValueError('Integer must be in range -1 <= n <= 16, got %d' % n)
  74 |  
  75 |          if n == 0:
  76 |              return OP_0
  77 | +        if n == -1:

vostrnad commented at 4:59 PM on March 8, 2024:

nit

        elif n == -1:

in test/functional/test_framework/script.py:87 in 8bd0a8b18d outdated

  84 |          if self == OP_0:
  85 |              return 0
  86 | +        if self == OP_1NEGATE:
  87 | +            return -1
  88 |  
  89 |          if not (self == OP_0 or OP_1 <= self <= OP_16):

vostrnad commented at 4:59 PM on March 8, 2024:

Checking OP_0 is unnecessary here, but now that it's not checking OP_1NEGATE it's inconsistently unnecessary. How about using is_small_int to avoid code duplication?

def decode_op_n(self):
    """Decode a small integer opcode, returning an integer"""
    if not self.is_small_int():
        raise ValueError('op %r is not an OP_N' % self)

    if self == OP_0:
        return 0
    elif self == OP_1NEGATE:
        return -1
    else:
        return int(self - OP_1 + 1)

Christewart commented at 8:28 PM on March 8, 2024:

Done in 6cf08dd2d532766f1b7063f40e7ab25f8ee67e45

in test/functional/test_framework/script.py:94 in 8bd0a8b18d outdated

  90 | @@ -87,7 +91,7 @@ def decode_op_n(self):
  91 |  
  92 |      def is_small_int(self):
  93 |          """Return true if the op pushes a small integer to the stack"""
  94 | -        if 0x51 <= self <= 0x60 or self == 0:
  95 | +        if 0x51 <= self <= 0x60 or self == 0 or self == -1:

vostrnad commented at 4:59 PM on March 8, 2024:

self is never -1, the check should be for 0x4f. Maybe we could rewrite this using named opcodes instead of bytes:

        if self == OP_0 or OP_1 <= self <= OP_16 or self == OP_1NEGATE:

Christewart commented at 8:28 PM on March 8, 2024:

Done in 6cf08dd2d532766f1b7063f40e7ab25f8ee67e45

vostrnad changes_requested

Christewart commented at 8:29 PM on March 8, 2024: contributor

Thanks for the code review @vostrnad , I took your changes and added them to this PR. I also cherry-picked them and tested them on 64bit-cscriptnum and everything ✔️

Christewart requested review from vostrnad on Mar 8, 2024

DrahtBot removed review request from vostrnad on Mar 9, 2024

DrahtBot requested review from sipa on Mar 9, 2024

vostrnad approved

vostrnad commented at 1:07 PM on March 9, 2024: none

ACK 6cf08dd2d532766f1b7063f40e7ab25f8ee67e45 (one style nit not addressed)

PR should also be renamed, e.g. "tests: fix OP_1NEGATE handling in CScriptOp"

Christewart renamed this:
~~tests: Fix bug `CScriptOp.encode_op_n()` where `OP_1NEGATE` was not handled correctly~~
tests: fix `OP_1NEGATE` handling in `CScriptOp`
on Mar 9, 2024

theStack commented at 11:07 AM on March 12, 2024: contributor

Concept ACK

Now that encode_op_n handles also -1, the following call-site in CScript.__coerce_instance can be simplified: https://github.com/bitcoin/bitcoin/blob/31be1a47675e4449f856e61beb2b4bfc228ea219/test/functional/test_framework/script.py#L446-L450

petertodd commented at 4:46 PM on March 12, 2024: contributor

Concept ACK

I'm proposing this for python-bitcoinlib too, which is where all this code comes from originally: https://github.com/petertodd/python-bitcoinlib/pull/299

petertodd referenced this in commit 0797d1c87b on Mar 12, 2024

Christewart commented at 8:00 PM on March 12, 2024: contributor

Concept ACK

Now that encode_op_n handles also -1, the following call-site in CScript.__coerce_instance can be simplified:

https://github.com/bitcoin/bitcoin/blob/31be1a47675e4449f856e61beb2b4bfc228ea219/test/functional/test_framework/script.py#L446-L450

Done in f0c077a388b980566590d7887d8c6dd97cbaa20a

dgpv commented at 8:33 AM on March 14, 2024: contributor

I'm not sure that it is wise to change the behavior of decode_op_n() and encode_op_n() to be different from CScript::DecodeOP_N() and CScript::EncodeOP_N(): https://github.com/bitcoin/bitcoin/blob/a85e5a7c9ab75209bc88e49be6991ba0a467034e/src/script/script.h#L506-L519

Having different behavior between code in C++ and python ~can~ will add confusion.

I would suggest to just handle -1/OP_1NEGATE in-place where they are relevant, rather than changing the assumption 'N is from 0 to 16' as it clearly is handled this way by EncodeOP_N/DecodeOP_N.

rkrux commented at 11:05 AM on April 8, 2024: none

Make is successful
All functional tests pass

Would it be possible to share where this error pops up in the branch specified in the description?

"When we don't encode -1 as OP_1NEGATE then we end up getting errors inside of CheckMinimalPush() saying -1 was not minimally encoded."

itornaza commented at 5:13 PM on April 12, 2024: contributor

tested ACK f0c077a388b980566590d7887d8c6dd97cbaa20a

Performed a code review and everything lgtm
Configured with --with-incompatible-bdb and --enable-suppress-external-warnings
Built the PR, ran unit tests with make check and all tests pass
Ran all functional tests with no extra flags and all tests pass
Ran all functional tests with --extended and all tests pass

DrahtBot requested review from vostrnad on Apr 12, 2024

DrahtBot requested review from theStack on Apr 12, 2024

dgpv commented at 5:27 PM on April 12, 2024: contributor

NACK for changing behavior of decode_op_n() and encode_op_n()

(the reasoning is given above, in #29589 (comment))

petertodd commented at 5:05 PM on April 17, 2024: contributor

On Wed, Apr 17, 2024 at 04:32:14AM -0700, Chris Stewart wrote:

@dgpv @petertodd what do you think is the path forward for this?

Seems to me that one reasonable path forward is to just do nothing and not merge these changes.

achow101 commented at 5:51 PM on April 25, 2024: member

I concur with @dgpv that these functions should behave the same as the C++ functions.

However, I think it would be okay for both the C++ and the test functions to be modified to handle OP_1NEGATE, although that may touch consensus code so the bar for merging will be higher.

achow101 referenced this in commit 50b09e8173 on Apr 25, 2024

Christewart force-pushed on May 2, 2024

Christewart commented at 9:29 PM on May 2, 2024: contributor

I've updated this PR to take @achow101 's suggestion. After 08f2c78658d8261199c75c7956fecc26efc17f3a the behavior between the c++ and python codebase is the same. Both codebases handle OP_1NEGATE correctly.

As mentioned by @dgpv and @petertodd , this now touches consensus code. Can a maintainer please add the consensus label?

Here is what I see for the implications. I'm not super familar with the c++ codebase, so please let me know if I am missing some occurrences.

`EncodeOP_N()`

I believe this function does not have any consensus implications.

`DecodeOP_N()`

`IsWitnessProgram()`

This will retain the previous behavior, because of this check in IsWitnessProgram()

https://github.com/bitcoin/bitcoin/blob/62ef33a718c9891d37d7c757968876033c4f794d/src/script/script.cpp#L231

`CScript::GetSigOpCount()`

This will retain previous behavior because the call to GetSigOpCount() by this check:

https://github.com/bitcoin/bitcoin/blob/62ef33a718c9891d37d7c757968876033c4f794d/src/script/script.cpp#L173

With that being said, I understand why this might not get merged. At minimum this can be archived and cherry-picked if this is needed for a soft fork in the future.

Christewart renamed this:
~~tests: fix `OP_1NEGATE` handling in `CScriptOp`~~
consensus: fix `OP_1NEGATE` handling in `CScriptOp`
on May 2, 2024

Christewart force-pushed on May 21, 2024

DrahtBot added the label CI failed on May 21, 2024

DrahtBot commented at 6:50 PM on May 21, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/25233209000</sub>

DrahtBot removed the label CI failed on May 22, 2024

in src/script/script.h:523 in 0abf50a71b outdated

 506 | @@ -507,14 +507,18 @@ class CScript : public CScriptBase
 507 |      {
 508 |          if (opcode == OP_0)
 509 |              return 0;
 510 | +        if (opcode == OP_1NEGATE)
 511 | +            return -1;
 512 |          assert(opcode >= OP_1 && opcode <= OP_16);

itornaza commented at 4:09 PM on May 27, 2024:

Non-blocking comment: Maybe move the assert line at the beginning of this member function so it is even more similar to its EncodeOP_N counterpart.

itornaza approved

itornaza commented at 4:23 PM on May 27, 2024: contributor

tested ACK 0abf50a71b0439ee7f4e94c548b29d726c58477a

Examined all changes from 3fb1b7043a7150f70a4cf1e172968e32c056dc79 up to this commit using git difftool 3fb1b70 0abf50a on the changed files. Did a code review on the changes that touches consensus code insrc/script/script.h as well as the associated test file test/functional/test_framework/script.py.

The changes in the python test functions encode_op_n and decode_op_n are now one to one compatible with the consensusEncodeOP_N and DecodeOP_N member functions in the cpp codebase.

I also freshly built and tested this commit and all unit, functional and extended tests pass.

Christewart force-pushed on Jun 3, 2024

Christewart force-pushed on Aug 23, 2024

DrahtBot added the label CI failed on Sep 5, 2024

DrahtBot removed the label CI failed on Sep 15, 2024

achow101 removed review request from vostrnad on Oct 15, 2024

achow101 requested review from theuni on Oct 15, 2024

achow101 requested review from achow101 on Oct 15, 2024

achow101 requested review from mzumsande on Oct 15, 2024

itornaza commented at 10:08 AM on October 29, 2024: contributor

Hey @Christewart! Do you plan to update for cmake?

Christewart force-pushed on Oct 29, 2024

Christewart commented at 2:17 PM on October 29, 2024: contributor

Hey @Christewart! Do you plan to update for cmake?

Done, i squashed everything into 1 commit as well.

theuni commented at 3:52 PM on October 29, 2024: member

I'm ~0 on this leaning towards NACK. As far as I can tell, all code paths are currently gated by if (>= OP_1 && <= OP_16) when calling EncodeOP_N/DecodeOP_N.

So this is really only changing the intention of those functions. And since it would be unsafe (because of OP_RESERVED) to change those gates in the callers to >= OP_1NEGATE && <= OP_16, it seems like OP_1NEGATE is going to have to remain a special case for callers anyway.

in test/functional/test_framework/script.py:92 in 1d8777ad66 outdated

  95 | +            return int(self - OP_1 + 1)
  96 |  
  97 |      def is_small_int(self):
  98 |          """Return true if the op pushes a small integer to the stack"""
  99 | -        if 0x51 <= self <= 0x60 or self == 0:
 100 | +        if self == OP_0 or OP_1 <= self <= OP_16 or self == OP_1NEGATE:

mzumsande commented at 7:34 PM on November 4, 2024:

This would mean that C++ (IsSmallInteger in solver.cpp) would have a different notion of a small interger.

Christewart commented at 3:10 PM on November 5, 2024:

I think this was already the case before this PR? It seems that the currently implementation in solver.cpp doesn't handle OP_0 correctly, while the python codebase does?

https://github.com/bitcoin/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/test/functional/test_framework/script.py#L89

https://github.com/bitcoin/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/src/script/solver.cpp#L61

This goes to the purpose of this PR, handle them consistently across codebases and remove special cases such as OP_1NEGATE.

Christewart commented at 3:17 PM on November 5, 2024:

In f2f8796 I modified IsSmallInteger() to handle OP_1NEGATE and OP_0 correctly. Previously only the python codebase handled OP_0 correctly.

mzumsande commented at 3:34 PM on November 5, 2024:

I don't think that "correct" is the right term here, it's more about what these functions are intended to be used for. In this case, the comment Test for "small positive integer" indicates that the function wasn't designed to deal with OP_0, so if it's just the name could just rename it to IsSmallPositiveInteger to not introduce a change in behavior.

Christewart commented at 5:18 PM on November 5, 2024:

This seems like a reasonable thing to do. However it seems like the script.py implementation should still handle OP_1NEGATE imo.

IIUC the solver.cpp's IsSmallInteger() and script.py's is_small_int() serve 2 different purposes, and renaming the solver.cpp function name would help differentiate them.

However if we aren't going to change any functionality in this file, perhaps its best to not do anything and just revert it and move on? 🤷‍♂️

mzumsande commented at 8:22 PM on November 4, 2024: contributor

I didn’t see earlier versions of this PR, but It’s not clear to me what the goal of this PR is from the OP / Title. Should the title be updated, is this still supposed to fix a bug?

As mentioned above, DecodeOP_N / EncodeOP_N currently don't need to handle OP_1NEGATE in any of the current use cases - so I think that there should be some explanation why they should be changed to support it.

I noticed that EncodeOP_N is currently called from PushAll in sign.cpp with an unsigned char (valtype), where there is already special-casing for OP_1NEGATE as 0x81, which seems like a possible source of confusion if the function was extended to deal with OP_1NEGATE as -1.

Also, the commit message could be fixed / made more coherent, it reads like it contains multiple commit messages from before a squash.

Christewart renamed this:
~~consensus: fix `OP_1NEGATE` handling in `CScriptOp`~~
consensus: Consistently encoded and decode `OP_1NEGATE` similar to other small ints in Script
on Nov 5, 2024

Christewart renamed this:
~~consensus: Consistently encoded and decode `OP_1NEGATE` similar to other small ints in Script~~
consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script
on Nov 5, 2024

Christewart force-pushed on Nov 5, 2024

Christewart commented at 3:15 PM on November 5, 2024: contributor

Thanks for looking at this @mzumsande !

I didn’t see earlier versions of this PR, but It’s not clear to me what the goal of this PR is from the OP / Title. Should the title be updated, is this still supposed to fix a bug?

Modified the title, hopefully its more clear.

As mentioned above, DecodeOP_N / EncodeOP_N currently don't need to handle OP_1NEGATE in any of the current use cases - so I think that there should be some explanation why they should be changed to support it.

I edited the original post to hopefully clarify why this change is useful.

Also, the commit message could be fixed / made more coherent, it reads like it contains multiple commit messages from before a squash.

done.

darosior commented at 3:30 PM on November 5, 2024: member

Concept NACK. This introduces dead code and i don't think the justification for this meets the bar for touching consensus code.

consensus: Consistently encode and decode OP_1NEGATE similar to other small ints in Script 6940761036

Christewart force-pushed on Feb 8, 2025

Christewart commented at 2:54 PM on February 8, 2025: contributor

Rebased

fanquake commented at 5:56 PM on February 20, 2025: member

@mzumsande do you want to circle back for another look here? Or @ajtowns?

ajtowns commented at 7:54 PM on February 20, 2025: contributor

Concept NACK. This introduces dead code and i don't think the justification for this meets the bar for touching consensus code.

I think it would make more sense to carry this patch as part of the 64 bit arithmetic patchset. If there's consensus to merge/activate 64 bit arithmetic, it could make sense at that point to split this into its own PR to make review easier, but without that motivation, I don't think this change makes much sense on its own. So Concept NACK from me too for now, unless there's some other tangible benefit now?

fanquake commented at 8:01 PM on February 20, 2025: member

Closing for now.

fanquake closed this on Feb 20, 2025