feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee #27914

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:feeperk-wo-round-trip changing 1 files +1 −1

achow101 commented at 6:39 PM on June 19, 2023: member

Returning the sats/kvb does not need to round trip through GetFee(1000) since the feerate is already stored as sats/kvb.

Fixes #27913, although this does bring up a larger question of how we should handle such large feerates in fuzzing.

feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee

Returning the sats/kvb does not need to round trip through
GetFee(1000) since the feerate is already stored as sats/kvb.

11d650060a

DrahtBot commented at 6:39 PM on June 19, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK furszy

Concept ACK dimitaracev, luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
dimitaracev commented at 7:45 PM on June 19, 2023: contributor

Concept ACK
maflcko commented at 7:09 AM on June 20, 2023: member

this does bring up a larger question of how we should handle such large feerates in fuzzing.

I think this particular issue is using the RPC interface, not the fuzzing interface. (Although it was found by fuzzing). Generally, if the RPC (and all other interfaces) had a guard against large values, then the fuzzing interface can also guard the values to the appropriate range. If one interface doesn't have a guard, the fuzzing shouldn't guard either.
fanquake requested review from dergoegge on Jun 21, 2023
fanquake requested review from josibake on Jun 22, 2023
luke-jr approved
luke-jr commented at 2:25 PM on June 24, 2023: member

utACK
furszy approved
furszy commented at 2:52 PM on June 25, 2023: member

Code ACK 11d65006
bitcoin deleted a comment on Jun 25, 2023
fanquake merged this on Jun 26, 2023
fanquake closed this on Jun 26, 2023
murchandamus commented at 2:42 PM on June 26, 2023: contributor

although this does bring up a larger question of how we should handle such large feerates in fuzzing.

We use the fee_a × vsize_b > fee_b × vsize_a trick in a bunch of places. On the other hand, fee is calculated from vsize × feerate. So, in fuzzing based on a feerate input, we should limit the feerate to prevent integer overflows with int64_t to max_feerate = int64_t_max / (max_vsize²)?

If we assume that our code does not have to deal with transactions greater than 100_000 vB:

9_223_372_036_854_775_807÷(100_000²) = 922_337_203_685 [ṩ/kvB] ≈ 922_337_203 [ṩ/vB] ≈ 9_223 ₿/kvB

If we assume that our code should be able to handle transactions of up to 1_000_000 vB, we’d restrict ourselves to 1/100 of that, i.e. 9_223_372_036 ṩ/kvB or 9_223_372 ṩ/vB
maflcko commented at 2:56 PM on June 26, 2023: member

We use the fee_a × vsize_b > fee_b × vsize_a trick in a bunch of places. On the other hand, fee is calculated from vsize × feerate. So, in fuzzing based on a feerate input, we should limit the feerate to prevent integer overflows with int64_t to max_feerate = int64_t_max / (max_vsize²)?

I'd say no, because the tricks use double, not int64_t, no? Also, this is an rpc interface bug, not a fuzzing bug, see #27914 (comment)
murchandamus commented at 3:12 PM on June 26, 2023: contributor

Oh, I meant to reply to the “larger question”. I recently also bumped into an integer overflow when fuzzing coin selection stuff due to feerates being too large, and I figured that the underlying issue was the same.
sidhujag referenced this in commit 64e80e907c on Jun 26, 2023
bitcoin locked this on Jun 25, 2024