Now that the impl moved into the C++ file in 38ece2a9c853ae3db9afb5215611c3e23e40461f, it could make sense to document the rounding behavior in the header file doxygen comment.
Also, drop the "special case", which seems more confusing than helpful. (It is only a special case for negative feerates, which are mostly irrelevant and only possible with manual fee prioritization deltas)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | polespinasa, pablomartin4btc, sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
CAmount a class by hodlinator)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
(made the feefrac fuzz test stricter, thx Ralph review)
28 | @@ -29,7 +29,6 @@ CAmount CFeeRate::GetFee(int32_t virtual_bytes) const 29 | Assume(virtual_bytes >= 0); 30 | if (m_feerate.IsEmpty()) { return CAmount(0);} 31 | CAmount nFee = CAmount(m_feerate.EvaluateFeeUp(virtual_bytes)); 32 | - if (nFee == 0 && virtual_bytes != 0 && m_feerate.fee < 0) return CAmount(-1); 33 | return nFee;
nit: now that the special case is gone, the temporary isn't needed (and this matches GetFeePerK above):
return CAmount(m_feerate.EvaluateFeeUp(virtual_bytes));
thx, done
62 | @@ -63,7 +63,7 @@ BOOST_AUTO_TEST_CASE(GetFeeTest) 63 | feeRate = CFeeRate(-123); 64 | // Truncates the result, if not integer 65 | BOOST_CHECK_EQUAL(feeRate.GetFee(0), CAmount(0)); 66 | - BOOST_CHECK_EQUAL(feeRate.GetFee(8), CAmount(-1)); // Special case: returns -1 instead of 0 67 | + BOOST_CHECK_EQUAL(feeRate.GetFee(8), CAmount(0)); 68 | BOOST_CHECK_EQUAL(feeRate.GetFee(9), CAmount(-1)); 69 |
The new "(Rounds down)" doc on GetFeePerK isn't covered by a test with a fractional rate (all existing ones are exact per kvB). Maybe we can add something like the following (which also pins the 1-sat gap between the two getters):
// GetFeePerK rounds down for fractional rates
BOOST_CHECK_EQUAL(CFeeRate(1, 3).GetFeePerK(), CAmount{333});
BOOST_CHECK_EQUAL(CFeeRate(-1, 3).GetFeePerK(), CAmount{-334});
// GetFee rounds up, so it can differ from GetFeePerK at 1000 vB
BOOST_CHECK_EQUAL(CFeeRate(1, 3).GetFee(1000), CAmount{334});
BOOST_CHECK_EQUAL(CFeeRate(-1, 3).GetFee(1000), CAmount{-333});
cr ACK fa9aab2a29405852698e097d5615da6a3e997259
Left one nit and a test suggestion (built and ran amount_tests locally with it).
27 | @@ -28,9 +28,7 @@ CAmount CFeeRate::GetFee(int32_t virtual_bytes) const 28 | { 29 | Assume(virtual_bytes >= 0); 30 | if (m_feerate.IsEmpty()) { return CAmount(0);} 31 | - CAmount nFee = CAmount(m_feerate.EvaluateFeeUp(virtual_bytes)); 32 | - if (nFee == 0 && virtual_bytes != 0 && m_feerate.fee < 0) return CAmount(-1); 33 | - return nFee;
I guess this is worth to be mentioned in the commit message, just in case in the future someone checks this commits without context.
It is the function immediately above, so I think I'll leave as-is for now :)
ACK fa3d4f099282a79099e59427180dffce4012a2b6
The documentation change is clean :)
Tried to see why we were returning -1 before and I couldn't fine any specific reason for it. Seems good to remove the exception.
re-ACK fa3d4f099282a79099e59427180dffce4012a2b6
ACK fa3d4f099282a79099e59427180dffce4012a2b6
