fix: Fix touchy "funds are conserved" assertion in LoanPay#6231
fix: Fix touchy "funds are conserved" assertion in LoanPay#6231ximinez merged 11 commits intoripple/staging-313from
Conversation
ximinez
added
the
DraftRunCI
ximinez
changed the title
ximinez
force-pushed
the
ximinez/loanpay-assertion
branch
from
5a41799 to
c1e161c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ripple/staging-313 #6231 +/- ##
==================================================
Coverage 79.5% 79.5%
==================================================
Files 840 840
Lines 71817 71890 +73
Branches 8198 8197 -1
==================================================
+ Hits 57061 57132 +71
- Misses 14756 14758 +2
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
ximinez
force-pushed
the
ximinez/loanpay-assertion
branch
from
c1e161c to
08170ff
Compare
ximinez
changed the title
ximinez
removed
the
DraftRunCI
| std::minmax_element(exponents.begin(), exponents.end()); | ||
| // IOU rounding can be interesting. Give a margin of error that reflects | ||
| // the orders of magnitude between the extremes. | ||
| if (!asset.integral() && *max < STAmount::cMaxOffset * 3 / 4) |
There was a problem hiding this comment.
I understand that you're using the 3 / 4 division to create a threshold that is an order of magnitude, but perhaps the magic numbers are better place somewhere else, or at least provided as a constant variable? Future generations (most likely one of us), might find this confusing.
There was a problem hiding this comment.
How about using withinRelativeDistance()?
There was a problem hiding this comment.
The tricky thing is how does one determine an acceptable delta?
There was a problem hiding this comment.
I understand that you're using the
3 / 4division to create a threshold that is an order of magnitude, but perhaps the magic numbers are better place somewhere else, or at least provided as a constant variable? Future generations (most likely one of us), might find this confusing.
Hell, I found it confusing when I wrote it. I've simplified it a bit, and it seems to be good enough for now.
| ahIGNORE_AUTH, | ||
| j_, | ||
| SpendableHandling::shFULL_BALANCE); | ||
| auto const balanceScale = [&]() { |
There was a problem hiding this comment.
Perhaps this change can be done separately, but wouldn't enforcing that there is no absolute balance change, is better served in Loan invariant?
There was a problem hiding this comment.
Perhaps this change can be done separately, but wouldn't enforcing that there is no absolute balance change, is better served in Loan invariant?
It would if I could figure out how to make it more "bulletproof". Even with the approximation I've implemented, I really don't trust that it's not going to have false negatives. With an assertion, no matter how "bad" the check is, it won't be included in release builds, and therefore can't break anything in production.
Additionally, if/when we ever do come up with a bulletproof way of checking, I would want to add a global conservation of funds invariant check, similar to TransfersNotFrozen, which accumulates changes across all trust lines, regardless of the transaction type. If you're going to spend the effort, don't limit yourself to just one transaction. That will probably require a new amendment, and is non-trivial.
| std::minmax_element(exponents.begin(), exponents.end()); | ||
| // IOU rounding can be interesting. Give a margin of error that reflects | ||
| // the orders of magnitude between the extremes. | ||
| if (!asset.integral() && *max < STAmount::cMaxOffset * 3 / 4) |
There was a problem hiding this comment.
Could you please expand on the balance scale logic? Suppose min = 7 and max = 9 and it's IOU. Then new max = 9 + (9 - 7) = 11. Why is it a better scale? I would have expected the lowest scale to be the best.
There was a problem hiding this comment.
Could you please expand on the balance scale logic? Suppose min = 7 and max = 9 and it's IOU. Then new max = 9 + (9 - 7) = 11. Why is it a better scale? I would have expected the lowest scale to be the best.
Scale is not always intuitive. It represents the exponent of the STAmount representation of a value. STAmount uses a 16 digit mantissa. So a simple number like "1" would have an exponent of -15. (1,000,000,000,000,000 * 10^-15)
Let's look at an example, which I got by using the minimum scale and waiting for the assertion to fail. These are from the log messages, but formatted for readability.
Before:
- account 100'999'726.0136946 (exponent: -7)
- vault 4'000'273.986277955 (exponent: -9)
- broker 94'500'000.0000274 (exponent: -8)
- total 199'500'000 (exponent: -7)
After: - account 100'999'452.0273892 (exponent: -7)
- vault 4'000'547.972555915 (exponent: -9)
- broker 94'500'000.00005479 (exponent: -8)
- total 199'499'999.9999999 (exponent -7)
Changes: - account -273.9863054, (exponent: -12)
- vault 273.98627796, (exponent: -12)
- broker 0.00002739, (exponent: -16)
- total -0.00000005 (exponent: -22)
The minimum scale of the balances is -9 (9 digits after the decimal point), and the max scale is -7 (7 digits after the decimal point).
If you round to the minimum scale, it will round all the numbers so they are accurate to 9 digits after the decimal point. That leaves all of these numbers unchanged. So when you add them together, or subtract them to get the "Changes", they are all at different scales, and give values that don't quite match.
Now let's look at what happens if we round to the max scale + 1, which is -6. (I simplified the balanceScale computation.)
Before:
- total 199'500'000
After: - total 199'500'000
Changes: - total 0
Internally, I don't add up the rounded balances - I round all three totals after adding - so I didn't include them here. That's because rounding the intermediate values can actually introduce errors. But the important thing is that after rounding to 6 digits after the decimal, the values all match.
| ahIGNORE_AUTH, | ||
| j_, | ||
| SpendableHandling::shFULL_BALANCE); | ||
| auto const balanceScale = [&]() { |
There was a problem hiding this comment.
currently these checks are in DEBUG mode. Do we want to bring some checks into RELEASE?
There was a problem hiding this comment.
I sort of addressed this question in #6231 (comment), but while answering @gregtatcam's question, I added some logging which could be useful in a release build.
So I just pushed c121d3d
ximinez
changed the base branch from
release-3.1
to
ximinez/release-3.1.0-rc2
ximinez
changed the base branch from
ximinez/staging-3.1
to
tapanito/lending-fix-amendment
ximinez
force-pushed
the
ximinez/loanpay-assertion
branch
from
87ad005 to
2bc61be
Compare
bthomee
requested review from
pratikmankawde and
vlntb
and removed request for
Tapanito
a1q123456
left a comment
a1q123456
left a comment
There was a problem hiding this comment.
All my concerns have been addressed
| XRPL_ASSERT_PARTS( | ||
| *assetsAvailableProxy <= *assetsTotalProxy, | ||
| assetsAvailableAfter <= assetsTotalAfter, | ||
| "xrpl::LoanPay::doApply", |
There was a problem hiding this comment.
| "xrpl::LoanPay::doApply", | |
| "rippled::LoanPay::doApply", |
| } | ||
| if (exponents.empty()) | ||
| { | ||
| UNREACHABLE("xrpl::LoanPay::doApply : all zeroes"); |
There was a problem hiding this comment.
| UNREACHABLE("xrpl::LoanPay::doApply : all zeroes"); | |
| UNREACHABLE("rippled::LoanPay::doApply : all zeroes"); |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
ximinez
dismissed
Tapanito’s stale review
Since @Tapanito is out for a while, I'm dismissing this review.
ximinez
added
the
Ready to merge
8 participants
High Level Overview of Change
IOU rounding when a Vault and a Loan are at significantly different scales can lead to the appearance of lost funds. An assertion in
LoanPaywants funds to be "conserved", but that isn't always possible.Context of Change
The new unit test
testYieldTheftRoundingwas submitted as a finding in ImmuneFi's attackathon. Due to earlier changes in how overpayments are calculated, the "theft" is now a non-issue, but the assertion still was causing problems.Update the rounding to improve accuracy, and calculate the amounts in a few different ways so if one still rounds unexpectedly, one of the others will not. This sounds weird, and it is, but IOU rounding can be weird. "Sometimes you eat the bear, and sometimes the bear eats you."
Type of Change