Differential D11358

Fix a bug in ParseMoney that prevents from parsing large numbers
ClosedPublic
Actions

Authored by Fabien on Apr 14 2022, 10:07.

Details

Reviewers

PiRK

Group Reviewers

Restricted Project

Commits

rABC764e6c6ff197: Fix a bug in ParseMoney that prevents from parsing large numbers

Summary

With the currency unit change, large numbers are failed to parse due to the 63 bits overflow prevention not being adjusted to account for the new decimal number. This diff fixes the issue, update the tests that was not checking for the correct error/not using the correct value and make sure we can parse the max money. The related avalanche processor test is expanded a bit for better coverage.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

fix_parsemoney_overflow

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 18825
Build 37440: Build Diff	lint-circular-dependencies · build-clang-tidy · build-without-wallet · build-diff · build-debug · build-clang
Build 37439: arc lint + arc unit

Event Timeline

Fabien created this revision.Apr 14 2022, 10:07

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 14 2022, 10:07

Fabien requested review of this revision.Apr 14 2022, 10:07

Harbormaster completed remote builds in B18824: Diff 33219.Apr 14 2022, 10:11

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

[332/517] Building CXX object src/CMakeFiles/common.dir/rpc/util.cpp.o
[333/517] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/pubkey.cpp.o
[334/517] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[335/517] Building CXX object src/CMakeFiles/script.dir/script/standard.cpp.o
[336/517] Building CXX object src/CMakeFiles/script.dir/script/sign.cpp.o
[337/517] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[338/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[339/517] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[340/517] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[341/517] Building CXX object src/CMakeFiles/script.dir/script/signingprovider.cpp.o
[342/517] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[343/517] Linking C static library src/secp256k1/libsecp256k1.a
[344/517] Linking C executable src/secp256k1/ecmult-bench
[345/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[346/517] Linking C executable src/secp256k1/internal-bench
[347/517] Linking C executable src/secp256k1/sign-bench
[348/517] Linking C executable src/secp256k1/verify-bench
[349/517] Linking C executable src/secp256k1/recover-bench
[350/517] Building CXX object src/CMakeFiles/script.dir/script/descriptor.cpp.o
[351/517] Installing component secp256k1
-- Install configuration: "Debug"
-- Install component: "secp256k1"
-- Installing: /results/artifacts/lib/libsecp256k1.a
-- Installing: /results/artifacts/include/secp256k1.h
-- Installing: /results/artifacts/include/secp256k1_preallocated.h
-- Installing: /results/artifacts/include/secp256k1_recovery.h
-- Installing: /results/artifacts/include/secp256k1_schnorr.h
[352/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[353/517] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[354/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[355/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[356/517] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[357/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[358/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[359/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[360/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[361/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[362/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[363/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[364/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[365/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[366/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[367/517] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[368/517] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[369/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[370/517] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[371/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[372/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[373/517] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[374/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[375/517] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[376/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[377/517] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[378/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[379/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[380/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[381/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[382/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

[305/463] Building CXX object src/CMakeFiles/server.dir/policy/settings.cpp.o
[306/463] Building CXX object src/CMakeFiles/server.dir/pow/daa.cpp.o
[307/463] Building CXX object src/CMakeFiles/server.dir/node/context.cpp.o
[308/463] Building CXX object src/CMakeFiles/server.dir/noui.cpp.o
[309/463] Building CXX object src/CMakeFiles/server.dir/rpc/command.cpp.o
[310/463] Building CXX object src/CMakeFiles/server.dir/node/transaction.cpp.o
[311/463] Building CXX object src/CMakeFiles/server.dir/pow/eda.cpp.o
[312/463] Building CXX object src/CMakeFiles/server.dir/pow/pow.cpp.o
[313/463] Building CXX object src/CMakeFiles/server.dir/pow/grasberg.cpp.o
[314/463] Building CXX object src/CMakeFiles/server.dir/script/scriptcache.cpp.o
[315/463] Building CXX object src/CMakeFiles/server.dir/rpc/abc.cpp.o
[316/463] Building CXX object src/CMakeFiles/server.dir/shutdown.cpp.o
[317/463] Building CXX object src/CMakeFiles/server.dir/node/ui_interface.cpp.o
[318/463] Building CXX object src/CMakeFiles/server.dir/net.cpp.o
[319/463] Building CXX object src/CMakeFiles/server.dir/rest.cpp.o
[320/463] Building CXX object src/CMakeFiles/server.dir/timedata.cpp.o
[321/463] Building CXX object src/CMakeFiles/server.dir/rpc/misc.cpp.o
[322/463] Building CXX object src/CMakeFiles/server.dir/script/sigcache.cpp.o
[323/463] Building CXX object src/CMakeFiles/server.dir/rpc/server.cpp.o
[324/463] Building CXX object src/CMakeFiles/server.dir/versionbits.cpp.o
[325/463] Building CXX object src/CMakeFiles/server.dir/rpc/mining.cpp.o
[326/463] Building CXX object src/CMakeFiles/server.dir/rpc/avalanche.cpp.o
[327/463] Building C object src/secp256k1/CMakeFiles/sign-bench.dir/src/bench_sign.c.o
[328/463] Building C object src/secp256k1/CMakeFiles/verify-bench.dir/src/bench_verify.c.o
[329/463] Building C object src/secp256k1/CMakeFiles/recover-bench.dir/src/bench_recover.c.o
[330/463] Building CXX object src/CMakeFiles/server.dir/dummywallet.cpp.o
[331/463] Building CXX object src/CMakeFiles/server.dir/txdb.cpp.o
[332/463] Building CXX object src/CMakeFiles/server.dir/rpc/net.cpp.o
[333/463] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[334/463] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[335/463] Building CXX object src/CMakeFiles/server.dir/validationinterface.cpp.o
[336/463] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[337/463] Linking C static library src/secp256k1/libsecp256k1.a
[338/463] Linking C executable src/secp256k1/ecmult-bench
[339/463] Linking C executable src/secp256k1/internal-bench
[340/463] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[341/463] Linking C executable src/secp256k1/sign-bench
[342/463] Linking C executable src/secp256k1/verify-bench
[343/463] Linking C executable src/secp256k1/recover-bench
[344/463] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[345/463] Building CXX object src/CMakeFiles/server.dir/torcontrol.cpp.o
[346/463] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[347/463] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[348/463] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[349/463] Building CXX object src/CMakeFiles/server.dir/txmempool.cpp.o
[350/463] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[351/463] Building CXX object src/CMakeFiles/server.dir/init.cpp.o
[352/463] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[353/463] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[354/463] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[355/463] Building CXX object src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o
[356/463] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[357/463] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[358/463] Building CXX object src/CMakeFiles/server.dir/rpc/blockchain.cpp.o
[359/463] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[360/463] Building CXX object src/CMakeFiles/server.dir/net_processing.cpp.o
[361/463] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[362/463] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

[332/517] Building C object src/secp256k1/CMakeFiles/recover-bench.dir/src/bench_recover.c.o
[333/517] Building CXX object src/CMakeFiles/script.dir/script/interpreter.cpp.o
[334/517] Building CXX object src/CMakeFiles/script.dir/script/sign.cpp.o
[335/517] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[336/517] Building CXX object src/CMakeFiles/common.dir/rpc/util.cpp.o
[337/517] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[338/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[339/517] Building CXX object src/CMakeFiles/script.dir/script/signingprovider.cpp.o
[340/517] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[341/517] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[342/517] Linking C static library src/secp256k1/libsecp256k1.a
[343/517] Linking C executable src/secp256k1/ecmult-bench
[344/517] Linking C executable src/secp256k1/internal-bench
[345/517] Linking C executable src/secp256k1/sign-bench
[346/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[347/517] Linking C executable src/secp256k1/verify-bench
[348/517] Linking C executable src/secp256k1/recover-bench
[349/517] Installing component secp256k1
-- Install configuration: "RelWithDebInfo"
-- Install component: "secp256k1"
-- Installing: /results/artifacts/lib/libsecp256k1.a
-- Installing: /results/artifacts/include/secp256k1.h
-- Installing: /results/artifacts/include/secp256k1_preallocated.h
-- Installing: /results/artifacts/include/secp256k1_recovery.h
-- Installing: /results/artifacts/include/secp256k1_schnorr.h
[350/517] Building CXX object src/CMakeFiles/script.dir/script/descriptor.cpp.o
[351/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[352/517] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[353/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[354/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[355/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[356/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[357/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[358/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[359/517] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[360/517] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[361/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[362/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[363/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[364/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[365/517] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[366/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[367/517] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[368/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[369/517] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[370/517] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[371/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[372/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[373/517] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[374/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[375/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[376/517] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[377/517] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[378/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[379/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[380/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[381/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[382/517] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

Fabien planned changes to this revision.Apr 14 2022, 11:24

Fix signedness, assert this subtraction is valid (and explain why), and prevent a pointless copy that should never have existed to begin with

Harbormaster completed remote builds in B18825: Diff 33220.Apr 14 2022, 11:55

Fabien added a child revision: D11359: [avalanche] Add an easy to use shortcut flag to turn on avalanche.Apr 14 2022, 12:36

PiRK accepted this revision.Apr 14 2022, 15:38

This revision is now accepted and ready to land.Apr 14 2022, 15:38

Closed by commit rABC764e6c6ff197: Fix a bug in ParseMoney that prevents from parsing large numbers (authored by Fabien). · Explain WhyApr 14 2022, 21:57

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABC764e6c6ff197: Fix a bug in ParseMoney that prevents from parsing large numbers.