policy: reject invalid CFeeRate values when deserializing #36816

Problem: With the UnstableChain IPC interface from #29409 merged, an IPC client could make bitcoin-node hit an Assume failure (debug builds) or compute wrong fees (release builds) by passing a malformed feerate to broadcastTransaction, calculateIndividualBumpFees or calculateCombinedBumpFee. These methods take feerates as serialized Data fields, and CFeeRate deserialization reads the fee and size without the checks its constructors do, so it accepts a negative size, which fee calculations require to be positive, and a zero size with a nonzero fee, which compares as an infinite feerate. Nothing deserializes a CFeeRate on master today.

Solution: Make CFeeRate deserialization throw on values its constructors cannot produce, the same way other malformed serialized data is rejected. The change is a SER_READ check in src/policy/feerate.h plus a unit test. It is similar to #36520, which handles other invalid arguments to Chain IPC methods.

This issue was found by Project Loupe and passed on by instagibbs.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

External sites

• Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36816
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36816.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36816#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Can the IPC client not still cause aborts via ftrapv or incorrect results, by just passing a feerate so large that it overflows later calculations?

re: #36816 (comment)

Can the IPC client not still cause aborts via ftrapv or incorrect results, by just passing a feerate so large that it overflows later calculations?

This might be possible, and if it is, it seems like the type of fix that would belong in #36520, which adds extra error checking to IPC methods, since it's not specific to the CFeeRate class and could happen with other numeric arguments.

Or if you're suggesting the problem here does not necessarily need to be solved, and it's ok for debug builds to crash and release builds to produce bad results if they are given bad values, maybe that could make sense too.

This PR is narrower: it only rejects serialized values that CFeeRate's constructors can't produce, like a negative size, which hits Assume(size > 0) in FeeFrac::EvaluateFee. A feerate that's valid but too large for later calculations is a range problem, the same one RPC methods avoid by bounding feerates in ParseFeeRate, so I'd check it where the IPC method receives the value.

Ok, I see. Maybe turn this into a draft for now, given that it modifies dead/unreachable code and depends on #29409, which is not a draft, but again depends on https://github.com/bitcoin-core/libmultiprocess/pull/377?

Once the code is reachable, a small dummy test of the form #36093 could be added to this pull?