Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCfe0b9d2657b7: [64-bit ints] Refactor `CScriptNum` to require setting `nMaxNumSize`

Summary

Currently, CScriptNum's constructor and IsMinimallyEncoded have a default for nMaxNumSize, which defaults to 4 (eCash's maximum integer size).

For upgrading Script integers from 32-bit to 64-bit, having to make the integer size explicit allows us to upgrade them based on a script flag.

This has no change in behavior.

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 25 2024, 00:21

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 25 2024, 00:21

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

Harbormaster completed remote builds in B30827: Diff 50418.Oct 25 2024, 00:59

rebase, also do the same change with ScriptNum10

Harbormaster completed remote builds in B30835: Diff 50429.Oct 25 2024, 09:20

tobias_ruck added a child revision: D17001: [64-bit ints] Add overflow checks to CScriptNum for +, -, +=, -=.Oct 25 2024, 09:55

rebase, add const

tobias_ruck edited the summary of this revision. (Show Details)Dec 6 2024, 00:11

tobias_ruck removed a parent revision: D16996: [64-bit ints] Change `CScriptNum::getint` to return `int64_t`.

Harbormaster completed remote builds in B31544: Diff 51430.Dec 6 2024, 01:02

tobias_ruck published this revision for review.Dec 6 2024, 10:27

Fabien requested changes to this revision.Dec 6 2024, 13:43

Fabien added inline comments.

src/script/script.h
227 ↗	(On Diff #51430)	This should probably no longer be a CScriptNum member. Why would you pass a static member to the constructor ? When going from 4 to 8 this variable will need to be moved out anyway.

This revision now requires changes to proceed.Dec 6 2024, 13:43

tobias_ruck added inline comments.Dec 6 2024, 20:57

src/script/script.h
227 ↗	(On Diff #51430)	well we can just reference CScriptNum::MAXIMUM_ELEMENT_SIZE, no? but happy to move it, keep it inside script.h ? keep in mind that there will be two variables during transition, MAXIMUM_ELEMENT_SIZE_32_BIT and MAXIMUM_ELEMENT_SIZE_64_BIT

move MAX_ELEMENT_SIZE out of CScriptNum, rename to MAX_SCRIPTNUM_BYTE_SIZE

Harbormaster completed remote builds in B34030: Diff 55082.Aug 1 2025, 14:12

Fabien added inline comments.Aug 1 2025, 18:38

src/core_write.cpp
125 ↗	(On Diff #55082)	Can %d from tinyformat handle 64 bits integers ? If not this will become an issue after you set MAX_SCRIPTNUM_BYTE_SIZE to 8

Fabien accepted this revision.Aug 1 2025, 18:38

This revision is now accepted and ready to land.Aug 1 2025, 18:38

tobias_ruck added inline comments.Aug 1 2025, 18:43

src/core_write.cpp
125 ↗	(On Diff #55082)	we already rely on the behavior you mention, PackageToValidate::ToString uses it on a int64_t (NodeId is a typedef for it), so should be fine

Closed by commit rABCfe0b9d2657b7: [64-bit ints] Refactor `CScriptNum` to require setting `nMaxNumSize` (authored by tobias_ruck). · Explain WhyAug 1 2025, 18:44

This revision was automatically updated to reflect the committed changes.

tobias_ruck added a commit: rABCfe0b9d2657b7: [64-bit ints] Refactor `CScriptNum` to require setting `nMaxNumSize`.

[64-bit ints] Refactor `CScriptNum` to require setting `nMaxNumSize`
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 55086

src/core_write.cpp

src/script/interpreter.cpp

src/script/script.h

src/test/monolith_opcodes_tests.cpp

src/test/scriptnum_tests.cpp

[64-bit ints] Refactor `CScriptNum` to require setting `nMaxNumSize`ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 55086

src/core_write.cpp

src/script/interpreter.cpp

src/script/script.h

src/test/monolith_opcodes_tests.cpp

src/test/scriptnum_tests.cpp

[64-bit ints] Refactor `CScriptNum` to require setting `nMaxNumSize`
ClosedPublic
Actions

Revision Contents
Changeset List