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

Authored by tobias_ruck on Thu, Oct 24, 23:56.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

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. For this, we bump ScriptNum10::getint, too.

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

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

scriptnum-getint-64-bit

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30834
Build 61181: Build Diff	lint-circular-dependencies · build-debug · build-clang-tidy · build-clang · build-diff · build-without-wallet
Build 61180: arc lint + arc unit

Event Timeline

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

Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Oct 24, 23:56

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

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

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

Fabien requested changes to this revision.Fri, Oct 25, 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.Fri, Oct 25, 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)Fri, Oct 25, 08:45

Harbormaster completed remote builds in B30834: Diff 50427.Fri, Oct 25, 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	this is safe because stack.size() is limited to MAX_STACK_SIZE (1000), and the cast is safe for n >= 0
1004	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	same