Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCd9bedb004bfb: [64-bit ints] Change `CScriptNum::getint` to return `int64_t`

Summary

As part of the effort to upgrade Script integers to 64-bits, we upgrade the code to handle CScriptNum::getint to return int64_t.

There's only a few usages of that function anyway:

In core_write.cpp, to write script assembly, and tinyformat can handle int64_t for %d
In interpreter.cpp, to convert to integers (e.g. for OP_ROLL). Here we're just careful to avoid overflows.
In scriptnum_tests.cpp, to compare to ScriptNum10. This test is now remarkably outdated (almost 9 years, looking at the copyright?), and doesn't really test something we worry about anymore. Instead, we remove CScriptNum10 and change the tests to actually test the behavior of CScriptNum.

(NB: BCH added getint32 and getint64 for the same change, but this is unnecessary and it's cleaner to just stick to one function)

Depends on D17290.

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tobias_ruck created this revision.Oct 24 2024, 23:56

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 24 2024, 23:56

tobias_ruck requested review of this revision.Oct 24 2024, 23:56

tobias_ruck added a child revision: D16997: [64-bit ints] Refactor `CScriptNum` to require setting `nMaxNumSize`.Oct 25 2024, 00:21

Harbormaster completed remote builds in B30826: Diff 50417.Oct 25 2024, 00:44

Fabien requested changes to this revision.Oct 25 2024, 08:15

Fabien added inline comments.

src/script/script.h
353 ↗	(On Diff #50417)	This is useless. What is the point of testing a function that is only used in said tests, and even more in tests that check against a past behavior ? What you want to check is that you don't introduce behavior changes for the places where the code is actually changed.
src/test/scriptnum_tests.cpp
59 ↗	(On Diff #50417)	This is testing nothing at all

This revision now requires changes to proceed.Oct 25 2024, 08:15

remove getint32, change ScriptNum10::getint instead to support int64_t

(in my defense, I looked at BCH code)

tobias_ruck edited the summary of this revision. (Show Details)Oct 25 2024, 08:45

Harbormaster completed remote builds in B30834: Diff 50427.Oct 25 2024, 09:04

I still don't get what you're doing with the test. Look at the scriptnum10 purpose: it's an older implementation and is here to check the new implementation behaves the same.
Your change is breaking that expectation, deliberately. To check if this introduces a problem:

You need to make sure opcodes are well covered by existing tests, including edge cases. I suggest you add these tests if they don't already exist in another diff to prove this one introduces no change in behavior.
Just remove that legacy compliant test, which is no longer relevant exactly because of that change.

src/script/interpreter.cpp
588 ↗	(On Diff #50427)	this is safe because stack.size() is limited to MAX_STACK_SIZE (1000), and the cast is safe for n >= 0
1004 ↗	(On Diff #50427)	This is safe because MAX_PUBKEYS_PER_MULTISIG is 20 and will never grow to a large enough value that it could cause issue
1021 ↗	(On Diff #50427)	same

This revision now requires changes to proceed.Oct 25 2024, 12:21

address review, remove CScriptNum10 and replace tests with tests that test the behavior of CScriptNum instead of testing whether it matches CScriptNum10

tobias_ruck edited the summary of this revision. (Show Details)Wed, Dec 4, 23:06

tobias_ruck edited the summary of this revision. (Show Details)Wed, Dec 4, 23:18

tobias_ruck added a parent revision: D17290: [64-bit ints] Add script_tests for `getint` coverage.

Harbormaster completed remote builds in B31518: Diff 51396.Wed, Dec 4, 23:26

Fabien requested changes to this revision.Thu, Dec 5, 10:06

Fabien added inline comments.

src/script/interpreter.cpp
588 ↗	(On Diff #51396)	This is a change in behavior vs n >= int64_t(stack.size()), and I don't think you choose the best option here. On 64 bits platforms, size_t will be 64 bits (and is unsigned) so equivalent to uint64_t. But n is an int64_t so if stack.size() > int64_t::max() you are not limiting the stack with n. I think restricting stack.size() to int64_t is a bit safer because we are sure n can always be >= stack.size(), and we should add a static assert that MAX_STACK_SIZE <= int64_t::max(). Note that in the real world both solutions are equivalent due to script/tx size limitations, so this is nit picking.
src/test/scriptnum_tests.cpp
63 ↗	(On Diff #51396)

This revision now requires changes to proceed.Thu, Dec 5, 10:06

tobias_ruck added inline comments.Thu, Dec 5, 17:24

src/script/interpreter.cpp
588 ↗	(On Diff #51396)	I think this is the better option, because in the other side of the OR branch, we are operating only with unsigned numbers, so it seems better to make that (more or less) explicit by converting to uint64_t. n (on this side of the branch) always fits in uint64_t (and int64_t), but stack.size() only into a uint64_t (and not into a int64_t, in all cases). So (arguing from set theory) it seems incorrect to use int64_t for stack.size(). Why add a static assert if we can just handle it here using the correct types?
src/test/scriptnum_tests.cpp
63 ↗	(On Diff #51396)	oups

fix CheckNegate

Harbormaster completed remote builds in B31532: Diff 51413.Thu, Dec 5, 18:08

I still think it's not the best option but I'm not dying on the hill since both are valid anyway.

src/script/interpreter.cpp
588 ↗	(On Diff #51396)	Unless I'm missing something the left side of the OR is signed, not unsigned (and the compiler would warn otherwise)

This revision is now accepted and ready to land.Thu, Dec 5, 19:24

Closed by commit rABCd9bedb004bfb: [64-bit ints] Change `CScriptNum::getint` to return `int64_t` (authored by tobias_ruck). · Explain WhyThu, Dec 5, 23:56

This revision was automatically updated to reflect the committed changes.

tobias_ruck added a commit: rABCd9bedb004bfb: [64-bit ints] Change `CScriptNum::getint` to return `int64_t`.

tobias_ruck removed a child revision: D16997: [64-bit ints] Refactor `CScriptNum` to require setting `nMaxNumSize`.Fri, Dec 6, 00:12

[64-bit ints] Change `CScriptNum::getint` to return `int64_t`
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 51429

src/script/interpreter.cpp

src/script/script.h

src/test/scriptnum10.h

src/test/scriptnum_tests.cpp

[64-bit ints] Change `CScriptNum::getint` to return `int64_t`ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 51429

src/script/interpreter.cpp

src/script/script.h

src/test/scriptnum10.h

src/test/scriptnum_tests.cpp

[64-bit ints] Change `CScriptNum::getint` to return `int64_t`
ClosedPublic
Actions

Revision Contents
Changeset List