From fa2dada0c9ab61266bcca86fcd28ced873976916 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 25 Mar 2024 11:33:07 +0100 Subject: [PATCH 1/3] rpc: Avoid getchaintxstats invalid results --- src/rpc/blockchain.cpp | 28 +++++++++++++++++++-------- test/functional/feature_assumeutxo.py | 6 +++--- test/sanitizer_suppressions/ubsan | 2 -- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index e7856786140..9db5d58cd39 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1645,13 +1645,19 @@ static RPCHelpMan getchaintxstats() RPCResult::Type::OBJ, "", "", { {RPCResult::Type::NUM_TIME, "time", "The timestamp for the final block in the window, expressed in " + UNIX_EPOCH_TIME}, - {RPCResult::Type::NUM, "txcount", "The total number of transactions in the chain up to that point"}, + {RPCResult::Type::NUM, "txcount", /*optional=*/true, + "The total number of transactions in the chain up to that point, if known. " + "It may be unknown when using assumeutxo."}, {RPCResult::Type::STR_HEX, "window_final_block_hash", "The hash of the final block in the window"}, {RPCResult::Type::NUM, "window_final_block_height", "The height of the final block in the window."}, {RPCResult::Type::NUM, "window_block_count", "Size of the window in number of blocks"}, - {RPCResult::Type::NUM, "window_tx_count", /*optional=*/true, "The number of transactions in the window. Only returned if \"window_block_count\" is > 0"}, + {RPCResult::Type::NUM, "window_tx_count", /*optional=*/true, + "The number of transactions in the window. " + "Only returned if \"window_block_count\" is > 0 and if txcount exists for the start and end of the window."}, {RPCResult::Type::NUM, "window_interval", /*optional=*/true, "The elapsed time in the window in seconds. Only returned if \"window_block_count\" is > 0"}, - {RPCResult::Type::NUM, "txrate", /*optional=*/true, "The average rate of transactions per second in the window. Only returned if \"window_interval\" is > 0"}, + {RPCResult::Type::NUM, "txrate", /*optional=*/true, + "The average rate of transactions per second in the window. " + "Only returned if \"window_interval\" is > 0 and if window_tx_count exists."}, }}, RPCExamples{ HelpExampleCli("getchaintxstats", "") @@ -1692,19 +1698,25 @@ static RPCHelpMan getchaintxstats() const CBlockIndex& past_block{*CHECK_NONFATAL(pindex->GetAncestor(pindex->nHeight - blockcount))}; const int64_t nTimeDiff{pindex->GetMedianTimePast() - past_block.GetMedianTimePast()}; - const int nTxDiff = pindex->nChainTx - past_block.nChainTx; + const auto window_tx_count{ + (pindex->nChainTx != 0 && past_block.nChainTx != 0) ? std::optional{pindex->nChainTx - past_block.nChainTx} : std::nullopt, + }; UniValue ret(UniValue::VOBJ); ret.pushKV("time", (int64_t)pindex->nTime); - ret.pushKV("txcount", (int64_t)pindex->nChainTx); + if (pindex->nChainTx) { + ret.pushKV("txcount", pindex->nChainTx); + } ret.pushKV("window_final_block_hash", pindex->GetBlockHash().GetHex()); ret.pushKV("window_final_block_height", pindex->nHeight); ret.pushKV("window_block_count", blockcount); if (blockcount > 0) { - ret.pushKV("window_tx_count", nTxDiff); + if (window_tx_count) { + ret.pushKV("window_tx_count", *window_tx_count); + } ret.pushKV("window_interval", nTimeDiff); - if (nTimeDiff > 0) { - ret.pushKV("txrate", ((double)nTxDiff) / nTimeDiff); + if (nTimeDiff > 0 && window_tx_count) { + ret.pushKV("txrate", double(*window_tx_count) / nTimeDiff); } } diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index 658eea0a0e9..ad8ba78a6a6 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -301,7 +301,7 @@ class AssumeutxoTest(BitcoinTestFramework): the snapshot, and final values after the snapshot is validated.""" for height, block in blocks.items(): tx = n1.getblockheader(block.hash)["nTx"] - chain_tx = n1.getchaintxstats(nblocks=1, blockhash=block.hash)["txcount"] + chain_tx = n1.getchaintxstats(nblocks=1, blockhash=block.hash).get("txcount", None) # Intermediate nTx of the starting block should be set, but nTx of # later blocks should be 0 before they are downloaded. @@ -311,11 +311,11 @@ class AssumeutxoTest(BitcoinTestFramework): assert_equal(tx, 0) # Intermediate nChainTx of the starting block and snapshot block - # should be set, but others should be 0 until they are downloaded. + # should be set, but others should be None until they are downloaded. if final or height in (START_HEIGHT, SNAPSHOT_BASE_HEIGHT): assert_equal(chain_tx, block.chain_tx) else: - assert_equal(chain_tx, 0) + assert_equal(chain_tx, None) check_tx_counts(final=False) diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 9818d73fdf1..65effa7a276 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -51,7 +51,6 @@ unsigned-integer-overflow:CCoinsViewCache::Uncache unsigned-integer-overflow:CompressAmount unsigned-integer-overflow:DecompressAmount unsigned-integer-overflow:crypto/ -unsigned-integer-overflow:getchaintxstats* unsigned-integer-overflow:MurmurHash3 unsigned-integer-overflow:CBlockPolicyEstimator::processBlockTx unsigned-integer-overflow:TxConfirmStats::EstimateMedianVal @@ -63,7 +62,6 @@ implicit-integer-sign-change:CBlockPolicyEstimator::processBlockTx implicit-integer-sign-change:SetStdinEcho implicit-integer-sign-change:compressor.h implicit-integer-sign-change:crypto/ -implicit-integer-sign-change:getchaintxstats* implicit-integer-sign-change:TxConfirmStats::removeTx implicit-integer-sign-change:prevector.h implicit-integer-sign-change:verify_flags From faf2a6750b2da97a18c48a3acf9c9da2aebe86d0 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 26 Apr 2024 17:59:05 +0200 Subject: [PATCH 2/3] rpc: Reorder getchaintxstats output --- src/rpc/blockchain.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 9db5d58cd39..d0065cb1a6e 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1651,10 +1651,10 @@ static RPCHelpMan getchaintxstats() {RPCResult::Type::STR_HEX, "window_final_block_hash", "The hash of the final block in the window"}, {RPCResult::Type::NUM, "window_final_block_height", "The height of the final block in the window."}, {RPCResult::Type::NUM, "window_block_count", "Size of the window in number of blocks"}, + {RPCResult::Type::NUM, "window_interval", /*optional=*/true, "The elapsed time in the window in seconds. Only returned if \"window_block_count\" is > 0"}, {RPCResult::Type::NUM, "window_tx_count", /*optional=*/true, "The number of transactions in the window. " "Only returned if \"window_block_count\" is > 0 and if txcount exists for the start and end of the window."}, - {RPCResult::Type::NUM, "window_interval", /*optional=*/true, "The elapsed time in the window in seconds. Only returned if \"window_block_count\" is > 0"}, {RPCResult::Type::NUM, "txrate", /*optional=*/true, "The average rate of transactions per second in the window. " "Only returned if \"window_interval\" is > 0 and if window_tx_count exists."}, @@ -1711,12 +1711,12 @@ static RPCHelpMan getchaintxstats() ret.pushKV("window_final_block_height", pindex->nHeight); ret.pushKV("window_block_count", blockcount); if (blockcount > 0) { + ret.pushKV("window_interval", nTimeDiff); if (window_tx_count) { ret.pushKV("window_tx_count", *window_tx_count); - } - ret.pushKV("window_interval", nTimeDiff); - if (nTimeDiff > 0 && window_tx_count) { - ret.pushKV("txrate", double(*window_tx_count) / nTimeDiff); + if (nTimeDiff > 0) { + ret.pushKV("txrate", double(*window_tx_count) / nTimeDiff); + } } } From 2342b46c451658a418f8e28e50b2ad0e5abd284d Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sat, 27 Apr 2024 15:48:15 +0200 Subject: [PATCH 3/3] test: Add coverage for getchaintxstats in assumeutxo context --- test/functional/feature_assumeutxo.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index ad8ba78a6a6..718a86befb2 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -33,6 +33,7 @@ from dataclasses import dataclass from test_framework.messages import tx_from_hex from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( + assert_approx, assert_equal, assert_raises_rpc_error, ) @@ -301,14 +302,28 @@ class AssumeutxoTest(BitcoinTestFramework): the snapshot, and final values after the snapshot is validated.""" for height, block in blocks.items(): tx = n1.getblockheader(block.hash)["nTx"] - chain_tx = n1.getchaintxstats(nblocks=1, blockhash=block.hash).get("txcount", None) + stats = n1.getchaintxstats(nblocks=1, blockhash=block.hash) + chain_tx = stats.get("txcount", None) + window_tx_count = stats.get("window_tx_count", None) + tx_rate = stats.get("txrate", None) + window_interval = stats.get("window_interval") # Intermediate nTx of the starting block should be set, but nTx of # later blocks should be 0 before they are downloaded. + # The window_tx_count of one block is equal to the blocks tx count. + # If the window tx count is unknown, the value is missing. + # The tx_rate is calculated from window_tx_count and window_interval + # when possible. if final or height == START_HEIGHT: assert_equal(tx, block.tx) + assert_equal(window_tx_count, tx) + if window_interval > 0: + assert_approx(tx_rate, window_tx_count / window_interval, vspan=0.1) + else: + assert_equal(tx_rate, None) else: assert_equal(tx, 0) + assert_equal(window_tx_count, None) # Intermediate nChainTx of the starting block and snapshot block # should be set, but others should be None until they are downloaded.