refactor: Use uint64_t over size_t for serialize corruption check in fees.dat

maflcko commented at 7:18 am on December 19, 2025: member

Serialization should not behave differently on different architectures. See also the related commit 3789215f73466606eb111714f596a2a5e9bb1933.

However, on fees.dat file corruption, 32-bit builds may run into an unsigned integer overflow and report the wrong corruption reason, or may even silently continue after the corruption.

This is a bit hard to reproduce, because 32-bit platforms are rare and most of them don’t support running the unsigned integer overflow sanitizer. So the possible options to reproduce are:

Run on armhf and manually annotate the code to detect the overflow
Run on i386 with the integer sanitizer (possibly via podman run -it --rm --platform linux/i386 'debian:trixie')
Run the integer sanitizer on any 64-bit platform and manually replace type in the affected line by uint32_t

Afterwards, the steps to reproduce are:

0export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git  --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config  python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev  systemtap-sdt-dev  libcapnp-dev capnproto  libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev  clang llvm libc++-dev libc++abi-dev   -y
1
2cmake -B ./bld-cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero --preset=dev-mode
3
4cmake --build ./bld-cmake --parallel  $(nproc)
5
6curl -fLO 'https://github.com/bitcoin-core/qa-assets/raw/b5ad78e070e4cf36beb415d7b490d948d70ba73f/fuzz_corpora/policy_estimator_io/607473137013139e3676e30ec4b29639e673fa9b'
7
8UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=policy_estimator_io ./bld-cmake/bin/fuzz ./607473137013139e3676e30ec4b29639e673fa9b

The output will be something like:

 0/b-c/src/policy/fees/block_policy_estimator.cpp:448:25: runtime error: unsigned integer overflow: 346685954 * 219 cannot be represented in type 'unsigned int'
 1    [#0](/bitcoin-bitcoin/0/) 0x5b0b1bbe in TxConfirmStats::Read(AutoFile&, unsigned int) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:448:25
 2    [#1](/bitcoin-bitcoin/1/) 0x5b0b7d3f in CBlockPolicyEstimator::Read(AutoFile&) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:1037:29
 3    [#2](/bitcoin-bitcoin/2/) 0x592a9783 in policy_estimator_io_fuzz_target(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/./test/fuzz/policy_estimator_io.cpp:32:32
 4    [#3](/bitcoin-bitcoin/3/) 0x5896ba8e in void std::__invoke_impl<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(std::__invoke_other, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:61:14
 5    [#4](/bitcoin-bitcoin/4/) 0x5896b8eb in std::enable_if<is_invocable_r_v<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>, void>::type std::__invoke_r<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:111:2
 6    [#5](/bitcoin-bitcoin/5/) 0x5896b44b in std::_Function_handler<void (std::span<unsigned char const, 4294967295u>), void (*)(std::span<unsigned char const, 4294967295u>)>::_M_invoke(std::_Any_data const&, std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:290:9
 7    [#6](/bitcoin-bitcoin/6/) 0x59845c95 in std::function<void (std::span<unsigned char const, 4294967295u>)>::operator()(std::span<unsigned char const, 4294967295u>) const /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:591:9
 8    [#7](/bitcoin-bitcoin/7/) 0x5983a0da in test_one_input(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5
 9    [#8](/bitcoin-bitcoin/8/) 0x5983cb80 in main /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:271:13
10    [#9](/bitcoin-bitcoin/9/) 0xf75aecc2  (/lib/i386-linux-gnu/libc.so.6+0x24cc2) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75)
11    [#10](/bitcoin-bitcoin/10/) 0xf75aed87 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x24d87) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75)
12    [#11](/bitcoin-bitcoin/11/) 0x58932db6 in _start (/b-c/bld-cmake/bin/fuzz+0x235ddb6) (BuildId: 7d8d83a77923f14e99c0de64acbc5f5bfc2cce9b)
13
14SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /b-c/src/policy/fees/block_policy_estimator.cpp:448:25

Note: This is marked a “refactor”, because the code change does not affect 64-bit builds, and on the still remaining rare 32-bit builds today it is extremely unlikely to happen in production.

refactor: Use uint64_t over size_t for serialize corruption check in fees.dat fa1d17d56c

DrahtBot added the label Refactoring on Dec 19, 2025

DrahtBot commented at 7:18 am on December 19, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34109.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	bensig, ismaelsadeeq, luke-jr

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

bensig commented at 3:31 am on January 5, 2026: contributor

ACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012

Straightforward fix - using uint64_t instead of size_t prevents potential overflow on 32-bit platforms where size_t is 32-bit.

maflcko added this to the milestone 31.0 on Jan 5, 2026

maflcko requested review from ismaelsadeeq on Jan 8, 2026

ismaelsadeeq commented at 2:48 pm on January 8, 2026: member

Concept ACK.

Nice catch. It is important to fix this. However, changing maxConfirms to uint64_t would only prevent the unsigned integer overflow by allowing it to store a very large value (or the modulo of that value).

Let’s extend this by adding another commit that validates the scale. Since we allow reading a corrupted scale, an overflow where the modulo of the multiplication result is stored in maxConfirms (scale* maxPeriods) may go undetected. If that modulo happens to fall within one 1-1008, the in L-454 check would pass, but the resulting scale would be invalid; this is a contrived, rare edge case.

We could be more defensive by requiring that the scale is exactly one of SHORT_SCALE, MED_SCALE, or LONG_SCALE. Otherwise, a corruption that produces a different in-range value would still be accepted as well.

However, for consistency with existing validation (e.g. decay), it likely makes sense to only enforce the upper bound rather than requiring an exact match.

 0diff --git a/src/policy/fees/block_policy_estimator.cpp b/src/policy/fees/block_policy_estimator.cpp
 1index 23aecefea2a..03d1c854811 100644
 2--- a/src/policy/fees/block_policy_estimator.cpp
 3+++ b/src/policy/fees/block_policy_estimator.cpp
 4@@ -435,6 +435,10 @@ void TxConfirmStats::Read(AutoFile& filein, size_t numBuckets)
 5         throw std::runtime_error("Corrupt estimates file. Scale must be non-zero");
 6     }
 7
 8+    if (scale > LONG_SCALE) {
 9+        throw std::runtime_error("Corrupt estimates file. Scale must not exceed the long scale");
10+    }
11+
12     filein >> Using<VectorFormatter<EncodedDoubleFormatter>>(m_feerate_avg);
13     if (m_feerate_avg.size() != numBuckets) {
14         throw std::runtime_error("Corrupt estimates file. Mismatch in feerate average bucket count");
15diff --git a/src/policy/fees/block_policy_estimator.h b/src/policy/fees/block_policy_estimator.h
16index ec432dcbf53..a5ade9cd01d 100644
17--- a/src/policy/fees/block_policy_estimator.h
18+++ b/src/policy/fees/block_policy_estimator.h
19@@ -98,6 +98,12 @@ struct FeeCalculation
20     unsigned int best_height{0};
21 };
22
23+namespace {
24+static constexpr unsigned int SHORT_SCALE = 1;
25+static constexpr unsigned int MED_SCALE   = 2;
26+static constexpr unsigned int LONG_SCALE  = 24;
27+}
28+
29 /** \class CBlockPolicyEstimator
30  * The BlockPolicyEstimator is used for estimating the feerate needed
31  * for a transaction to be included in a block within a certain number of
32@@ -151,13 +157,10 @@ class CBlockPolicyEstimator : public CValidationInterface
33 private:
34     /** Track confirm delays up to 12 blocks for short horizon */
35     static constexpr unsigned int SHORT_BLOCK_PERIODS = 12;
36-    static constexpr unsigned int SHORT_SCALE = 1;
37     /** Track confirm delays up to 48 blocks for medium horizon */
38     static constexpr unsigned int MED_BLOCK_PERIODS = 24;
39-    static constexpr unsigned int MED_SCALE = 2;
40     /** Track confirm delays up to 1008 blocks for long horizon */
41     static constexpr unsigned int LONG_BLOCK_PERIODS = 42;
42-    static constexpr unsigned int LONG_SCALE = 24;
43     /** Historical estimates that are older than this aren't valid */
44     static const unsigned int OLDEST_ESTIMATE_HISTORY = 6 * 1008;

maflcko commented at 3:27 pm on January 8, 2026: member

However, changing maxConfirms to uint64_t would only prevent the unsigned integer overflow by allowing it to store a very large value (or the modulo of that value).

Let’s extend this by adding another commit that validates the scale. Since we allow reading a corrupted scale, an overflow where the modulo of the multiplication result is stored in maxConfirms (scale* maxPeriods) may go undetected. If that modulo happens to fall within one 1-1008, the in L-454 check would pass, but the resulting scale would be invalid; this is a contrived, rare edge case.

Not sure this is relevant to fix. A scale corruption is already extremely unlikely to happen (outside of fuzzing). And the scale corruption along with a file size corruption is impossible on modern filesystems, as they guard the metadata. And for older filesystems it would be cosmically unlikely and found by the next filesystem check.

Recall, that scale is u32, and the code is:

0    filein >> Using<VectorFormatter<VectorFormatter<EncodedDoubleFormatter>>>(confAvg);
1    maxPeriods = confAvg.size();
2    maxConfirms = scale * maxPeriods;

So, for u64 to overflow as a result of multiplication, a massive persisted file would be required (confAvg would have to deserialize correctly first). I don’t think any of this can ever happen in practise.

A scale corruption by itself may happen (and does happen in the fuzz target), so a trivial one-line fix for it seemed fine to me.

I am less sure about adding code to further validate the scale: After all, the code here is just writing (and reading) constants. So, either the serialize can be skipped, because it is redundant, brittle and useless. Or, adding guards to it would be wrong, because there is a use-case out there to serialize a not constant scale. Not sure, but it doesn’t seem worth it to me to write a larger patch for this.

ismaelsadeeq commented at 3:41 pm on January 8, 2026: member

It’s fine with me to limit this to the unsigned integer overflow fix. A follow-up can be opened to further improve this and to validate the scale. I brought it up as a suggestion for improvement in the review, and the comment can be linked in future discussions.

utACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012

maflcko commented at 3:55 pm on January 8, 2026: member

Yeah, seems fine to track this in a follow-up. Just for reference, if the scale serialization is indeed useless and brittle, it can be removed via:

 0diff --git a/src/policy/fees/block_policy_estimator.cpp b/src/policy/fees/block_policy_estimator.cpp
 1index 23aecefea2..8f31b98212 100644
 2--- a/src/policy/fees/block_policy_estimator.cpp
 3+++ b/src/policy/fees/block_policy_estimator.cpp
 4@@ -105,7 +105,7 @@ private:
 5     double decay;
 6 
 7     // Resolution (# of blocks) with which confirmations are tracked
 8-    unsigned int scale;
 9+    const unsigned int scale;
10 
11     // Mempool counts of outstanding transactions
12     // For each bucket X, track the number of transactions in the mempool
13@@ -430,9 +430,12 @@ void TxConfirmStats::Read(AutoFile& filein, size_t numBuckets)
14     if (decay <= 0 || decay >= 1) {
15         throw std::runtime_error("Corrupt estimates file. Decay must be between 0 and 1 (non-inclusive)");
16     }
17-    filein >> scale;
18-    if (scale == 0) {
19-        throw std::runtime_error("Corrupt estimates file. Scale must be non-zero");
20+    {
21+    uint32_t dummy;
22+    filein >> dummy;
23+    if (scale == 0||scale!=dummy) {
24+        throw std::runtime_error(strprintf("Corrupt estimates file. Scale must be non-zero and must be equal to %s", dummy));
25+    }
26     }
27 
28     filein >> Using<VectorFormatter<EncodedDoubleFormatter>>(m_feerate_avg);

(passes tests, fwiw)

luke-jr commented at 2:27 am on January 14, 2026: member

https://github.com/bitcoin/bitcoin/commit/163d3e5c137401b0145ec57c7e24947e68f908e6 fixes also the hypothetical/impractical overflow. Use it if you want.

Also, utACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012 as an improvement.

maflcko commented at 7:18 am on January 14, 2026: member

163d3e5 fixes also the hypothetical/impractical overflow. Use it if you want.

I’ll leave as-is for now, because I don’t think an additional patch is needed, and the change is also unclear, because three people wrote three different patches for this. So this seems better to leave as-is or for a follow-up.

Whereas this pull request should be trivially correct and already has 3 acks.

fanquake merged this on Jan 14, 2026

fanquake closed this on Jan 14, 2026

maflcko deleted the branch on Jan 14, 2026

refactor: Use uint64_t over size_t for serialize corruption check in fees.dat #34109

Code Coverage & Benchmarks

Reviews