fees: Return false for incompatible fee estimates #35830

HowHsu commented at 2:49 PM on July 28, 2026: contributor

policy_estimator_io deliberately reuses a CBlockPolicyEstimator because constructing one for every fuzz input severely reduces throughput. However, Read() returns true for an incompatible old fee estimates file without replacing the estimator state. The target then calls Write() with state loaded by a previous input, making coverage depend on corpus order.

Return false for incompatible files so the target skips Write() when no state was loaded. This keeps the estimator reuse optimization instead of resetting the expensive object before every fuzz input.

For the in-tree production caller, incompatible files remain non-fatal and the estimator still starts from its default state. Read() now reports failure, so startup emits one additional non-fatal warning. Node startup and estimator state are unchanged, as is RPC behavior.

DrahtBot added the label TX fees and policy on Jul 28, 2026

DrahtBot commented at 2:49 PM on July 28, 2026: 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/35830.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	maflcko, sedited
Stale ACK	ismaelsadeeq

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

bucketmap -> bucketMap [misspelled identifier in the comment; it may confuse readers by not matching the actual variable name]

ismaelsadeeq approved

ismaelsadeeq commented at 9:52 AM on July 29, 2026: member

ACK f23debfee28169388ee6a2e9f7d38ee99fbfda40

Also seems misleading that we do not warn that the block estimator file was not read.

in src/policy/fees/block_policy_estimator.cpp:1019 in f23debfee2 outdated

1015 | @@ -1016,6 +1016,7 @@ bool CBlockPolicyEstimator::Read(AutoFile& filein)
1016 |  
1017 |          if (nVersionRequired < CURRENT_FEES_FILE_VERSION) {
1018 |              LogWarning("Incompatible old fee estimation data (non-fatal). Version: %d", nVersionRequired);
1019 | +            return false;

sedited commented at 9:56 AM on July 29, 2026:

Is there still a need for the else case after this?

HowHsu commented at 12:50 PM on July 29, 2026:

Thanks for the review, I've updated this.

HowHsu force-pushed on Jul 29, 2026

ismaelsadeeq commented at 2:55 PM on July 30, 2026: member

reACK 80299ac874bf2472a932d78b0bcfa58b19102054

The last change was to make the code cleaner by removing an unnecessary else branch.

I think this should have been at least a warning by tidy.

in src/policy/fees/block_policy_estimator.cpp:1019 in 80299ac874

1041 | -            buckets = fileBuckets;
1042 | -            bucketMap.clear();
1043 | -            for (unsigned int i = 0; i < buckets.size(); i++) {
1044 | -                bucketMap[buckets[i]] = i;
1045 | -            }
1046 | +            return false;

maflcko commented at 1:15 PM on July 31, 2026:

every other "return" uses a "throw" in this function, so this one should follow the pattern and be a throw as well.

HowHsu force-pushed on Aug 5, 2026

ismaelsadeeq commented at 12:19 PM on August 5, 2026: member

Please do not rebase when there is no need to, it's easier to review again by just looking at the diff.

HowHsu commented at 12:24 PM on August 5, 2026: contributor

Please do not rebase when there is no need to, it's easier to review again by just looking at the diff.

Do you mean add another commit to this PR rather than amend the change to the old one?

ismaelsadeeq commented at 12:44 PM on August 5, 2026: member

Do you mean add another commit to this PR rather than amend the change to the old one?

Normally, how I do it is: I do an interactive rebase on the commit I want to modify, I edit the file to make the changes, add the hunks, then amend the commit, and continue the rebase. No need to rebase onto upstream master after that if there are no merge conflicts.

The difference is clear in this compare of your first force push here 8b19102054..9e96d72f and the recent compare 8b19102054..9e96d72f17 which has changes to other src file since you committed 80299ac874bf2472a932d78b0bcfa58b19102054

No worries, you don't need to do anything now, I will fetch the commit and review locally. I'm just telling you because reviewing the diff is easier and faster than fetching locally and re-reviewing commits after force pushes.

maflcko commented at 12:59 PM on August 5, 2026: member

Heh, I think a rebase is nice, because it makes reviewing on GitHub harder. :)

We should move toward locally reviewing anyway. Also, GitHub is now more aggressively deleting older commits, so the GitHub diff will be broken in most cases anyway.

in src/policy/fees/block_policy_estimator.cpp:1012 in 9e96d72f17

1007 | @@ -1008,51 +1008,51 @@ bool CBlockPolicyEstimator::Read(AutoFile& filein)
1008 |          if (nVersionRequired > CURRENT_FEES_FILE_VERSION) {
1009 |              throw std::runtime_error{strprintf("File version (%d) too high to be read.", nVersionRequired)};
1010 |          }
1011 | +        if (nVersionRequired < CURRENT_FEES_FILE_VERSION) {
1012 | +            throw std::runtime_error{strprintf("Incompatible old fee estimation data (non-fatal). Version: %d", nVersionRequired)};

maflcko commented at 1:01 PM on August 5, 2026:

Should probably say:

            throw std::runtime_error{strprintf("File version (%d) incompatible: Too old to be read", nVersionRequired)};

to avoid a duplicate (non-fatal)

HowHsu commented at 2:11 PM on August 5, 2026: contributor

Do you mean add another commit to this PR rather than amend the change to the old one?

Normally, how I do it is: I do an interactive rebase on the commit I want to modify, I edit the file to make the changes, add the hunks, then amend the commit, and continue the rebase. No need to rebase onto upstream master after that if there are no merge conflicts.

Ah, I see what you mean now, my normal process is almost as same as yours, the only different thing is I often do git pull --rebase upstream master in the first palce to keep the branch up to date first..., didn't realize the review problem caused by that, sorry.

HowHsu force-pushed on Aug 5, 2026

DrahtBot added the label CI failed on Aug 5, 2026

fees: Return false for incompatible fee estimates

policy_estimator_io deliberately reuses a CBlockPolicyEstimator because
constructing one for every fuzz input severely reduces throughput.
However, Read() returns true for an incompatible old fee estimates file
without replacing the estimator state. The target then calls Write()
with state loaded by a previous input, making coverage depend on corpus
order.

Return false for incompatible files so the target skips Write() when no
state was loaded. This keeps the estimator reuse optimization instead of
resetting the expensive object before every fuzz input.

For the in-tree production caller, incompatible files remain non-fatal
and the estimator still starts from its default state. Read() now
reports failure, so startup emits one additional non-fatal warning.
Node startup and estimator state are unchanged, as is RPC behavior.

Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>

b9d573e4a9

HowHsu force-pushed on Aug 5, 2026

DrahtBot removed the label CI failed on Aug 5, 2026

maflcko commented at 10:28 AM on August 7, 2026: member

review ACK b9d573e4a9594e460e100042f195392f3a59480b 📩

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK b9d573e4a9594e460e100042f195392f3a59480b 📩
gQieCXI64sp2/oxF+H3Y34YZ7u8KoFRInFEnthngM5leH7l23MicF+ejb7SNmELgKiUsV7XsEs2d6TYnXEVEDg==

</details>

DrahtBot requested review from ismaelsadeeq on Aug 7, 2026

sedited approved

sedited commented at 12:14 PM on August 7, 2026: contributor

ACK b9d573e4a9594e460e100042f195392f3a59480b

sedited merged this on Aug 7, 2026

sedited closed this on Aug 7, 2026