consensus: fix `OP_1NEGATE` handling in `CScriptOp`

[Christewart]

This was discovered when working on #29221

OP_1NEGATE is the minimally encoded version of the number -1 in the interpreter. When we don't encode -1 as OP_1NEGATE then we end up getting errors inside of CheckMinimalPush() saying -1 was not minimally encoded.

https://github.com/bitcoin/bitcoin/blob/67fb94ce42205e290e3f382a682ebfc8a13053bd/src/script/script.cpp#L358

If you would like to see the work that unearthed this bug please see my branch implementing 64bit arithmetic with CScriptNum

https://github.com/Christewart/bitcoin/tree/64bit-arith-cscriptnum

[DrahtBot]

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.

[vostrnad]

Should decode_op_n and is_small_int perhaps be fixed as well?

[Christewart]

Should decode_op_n and is_small_int perhaps be fixed as well?

Done in 8bd0a8b18d68bbacee731f3e0583397f81ca078e , good catch :raised_hands:

[sipa]

utACK 8bd0a8b18d68bbacee731f3e0583397f81ca078e

[Christewart]

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 ✔️

[theStack]

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]

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

[Christewart]

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]

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.

[itornaza]

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

[dgpv]

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

(the reasoning is given above, in https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-1996854762)

[petertodd]

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]

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.

[Christewart]

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.

[DrahtBot]

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

_{Debug: https://github.com/bitcoin/bitcoin/runs/25233209000}

[itornaza]

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

[Christewart]

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

Done, i squashed everything into 1 commit as well.

[theuni]

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.

[Christewart]

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]

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

[Christewart]

Rebased

[fanquake]

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

[ajtowns]

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]

Closing for now.