Skip to content

test(amount): fix GetFeeTest int64 overflow causing 318-failure cascade#444

Open
JSanchezFDZ wants to merge 1 commit into
Raptor3um:developfrom
JSanchezFDZ:fix/amount-test-max-money-overflow
Open

test(amount): fix GetFeeTest int64 overflow causing 318-failure cascade#444
JSanchezFDZ wants to merge 1 commit into
Raptor3um:developfrom
JSanchezFDZ:fix/amount-test-max-money-overflow

Conversation

@JSanchezFDZ

Copy link
Copy Markdown

Summary

amount_tests/GetFeeTest aborts in current develop because the final boundary check feeds Raptoreum's MAX_MONEY (21B * COIN = 2.1e18) into CFeeRate's two-arg constructor. The constructor multiplies by 1000 internally, producing 2.1e21, which overflows int64_t (max ~9.2e18). On modern toolchains the signed-overflow trap raises SIGABRT, Boost.test catches the signal but unwinds without running BasicTestingSetup's destructor, so ECC_Stop() is never called. Every subsequent test case's fixture constructor then trips assert(secp256k1_context_sign == nullptr) inside ECC_Start(), cascading into hundreds of follow-on failures.

The test was inherited verbatim from Bitcoin/Dash, where MAX_MONEY is 21M * COIN (1000x smaller) and MAX_MONEY * 1000 fits comfortably in int64_t. The 1000x bump to Raptoreum's cap predated this test; the boundary case just never got revisited.

Fix: use OLD_MAX_MONEY (= 21M * COIN, the Bitcoin-equivalent value already defined in amount.h) for that one line. The test still exercises the same "huge nBytes, should not crash" code path it always did, without triggering UB.

Before

$ ./src/test/test_raptoreum
Running 362 test cases...
[...amount_tests/GetFeeTest aborts at line 82...]
test_raptoreum: key.cpp:341: void ECC_Start():
  Assertion `secp256k1_context_sign == nullptr' failed.
[...318 cascade failures across the rest of the suite...]
*** 318 failures are detected in the test module "Raptoreum Test Suite"

After

$ ./src/test/test_raptoreum
Running 362 test cases...
*** No errors detected

Why this matters

The cascade hides every other regression in the suite. Once amount_tests aborts, anyone running the unit tests sees a sea of "secp256k1 context already initialised" assertions and can't tell whether unrelated suites pass or fail. This is the kind of test infrastructure bug that quietly degrades the signal of CI over time.

Confirmed locally on develop (commit 989ec2e) inside the project's depends/-based Ubuntu 22.04 + GCC 11.4 build environment.

Test plan

  • Build test/test_raptoreum against develop HEAD with the patch.
  • Run ./src/test/test_raptoreum --run_test=amount_tests: 4/4 cases pass.
  • Run ./src/test/test_raptoreum: 362/362 cases pass (UTF-8 locale; without it fs_tests/fsbridge_fstream fails on a separate locale-related issue, unrelated to this change).
  • Inspect diff: one line changed (plus comment explaining the why), no consensus code touched.

Note for reviewers

This is the minimal localized fix. The deeper question -- whether CFeeRate should be hardened to accept fee amounts up to Raptoreum's full MAX_MONEY without overflowing -- is a wider design conversation (e.g. switch internal arithmetic to __int128 or boost::multiprecision::int128_t, similar to how CAmount128 is already used elsewhere in the codebase for asset balances). I'd be happy to open a separate issue for that if you'd like, but felt the boundary-case test fix should land independently so the suite stops hiding other regressions in the meantime.

The `CFeeRate(MAX_MONEY, std::numeric_limits<size_t>::max() >> 1)` call
inherited from Bitcoin/Dash overflows int64_t inside `CFeeRate`'s
two-arg constructor on Raptoreum because Raptoreum's MAX_MONEY
(21B * COIN = 2.1e18) is 1000x larger than Bitcoin's. The constructor
computes `nFeePaid * 1000`, yielding 2.1e21 which overflows signed
64-bit (max ~9.2e18).

Under signed-overflow sanitizers / `-ftrapv` (and recent GCC/glibc
builds), the overflow aborts the process via SIGABRT. Boost.test
catches the signal but unwinds without running BasicTestingSetup's
destructor, so `ECC_Stop` is never called. The next test case's
fixture constructor then trips `assert(secp256k1_context_sign ==
nullptr)` in `ECC_Start()`, and every subsequent case cascades into
the same assertion. Locally this turns one abort in amount_tests
into 318 follow-on failures across the suite.

Switching the boundary case to OLD_MAX_MONEY (== Bitcoin's MAX_MONEY,
2.1e15) preserves the original test intent -- "Maximum size in bytes,
should not crash" -- without triggering UB. The 1000x bump to
MAX_MONEY predated this test; this is just catching up.

After the fix the full Boost test suite passes (362/362 cases).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant