Add units to all fee calculations: #2910
Conversation
|
@HowardHinnant Could you make sure this one builds on MacOS and that I didn't reintroduce any problems? |
| @@ -711,22 +711,23 @@ Json::Value checkFee ( | |||
| } | |||
|
|
|||
| // Default fee in fee units. | |||
| std::uint64_t const feeDefault = config.TRANSACTION_FEE_BASE; | |||
| FeeUnit32 const feeDefault = config.TRANSACTION_FEE_BASE; | |||
There was a problem hiding this comment.
I intentionally changed this from 64 to 32 to match the ledger, and to make downstream conversions easier
Still 👍 here. |
Woo hoo! Thanks! |
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
01d60aa to
1265995
Compare
|
Rebased on top of 1.3.0-b3. Everything went swimmingly. |
Jenkins Build SummaryBuilt from this commit Built at 20190823 - 16:18:24 Test Results
|
| std::is_unsigned<T2>::value && | ||
| sizeof(T2) <= sizeof(std::uint64_t) > | ||
| > | ||
| void lowestTerms(T1& a, T2& b) |
There was a problem hiding this comment.
is this non-unit version invoked at all? could we remove it?
There was a problem hiding this comment.
Looks like "no" and "yes".
| std::is_same_v<Unit1, units::drop_unit> | ||
| > | ||
| > | ||
| void lowestTerms(boost::units::quantity<Unit1, T1>& a, |
There was a problem hiding this comment.
the asymmetry of arguments bugs me, but the use is very limited (to just this file) so I guess I'm less concerned. That said, if there were some way to accomplish this with lambdas I'd possibly opt for that because these two overloads don't seem broadly useful to me.
There was a problem hiding this comment.
I was able to make it work with lambdas local to scaleFeeLoad.
src/ripple/basics/feeunits.h
Outdated
| return(os); | ||
| } | ||
|
|
||
| // get string representation of a quantity |
There was a problem hiding this comment.
wouldn't this be the same thing boost::lexical_cast<std::string>(t) would do? can we just use that?
There was a problem hiding this comment.
Even better, I can just do return to_string(t.value());. I originally used operator<< to future-proof it in case I only wanted to change the output format in the future. I doubt that's going to happen, though, honestly.
src/ripple/basics/feeunits.h
Outdated
| // Need to cap to uint64 to uint32 due to JSON limitations | ||
| template<class Unit> | ||
| boost::units::quantity<Unit, std::uint32_t> | ||
| clamp(boost::units::quantity<Unit, std::uint64_t> v) |
There was a problem hiding this comment.
because this has the same name as an std/boost function but slighty different arguments (the hi/low is implicit), I'd be inclined to give it a slightly different name (e.g. clampJSON,clamp32, etc.)
There was a problem hiding this comment.
Renamed to clamp32. Easy enough since it's in the detail namespace.
src/ripple/protocol/STObject.h
Outdated
| std::enable_if_t< | ||
| std::is_assignable_v<T, U> && | ||
| ( | ||
| std::is_same_v<Unit, units::drop_unit> || |
There was a problem hiding this comment.
do we actually need to restrict Unit to these three types? I'll admit I don't fully understand this code, but I was hoping the assign from argument type would be sufficient.
There was a problem hiding this comment.
I consider this an important safety mechanism because we're stripping out unit information to make the assignment. Mechanically, the main intention is to keep values with "intermediate" or "composed" units from incorrectly being assigned into objects and losing that unit information. Consider the following pseudocode:
STObject o = [whatever];
auto x = 150 * units::drops;
auto y = 10 * units::feeunits;
o[sfField1] = x; // This is good. x is in drops, so it makes sense to allow this assignment. sfField1 is 150.
o[sfField2] = y; // This is also good. y is fee units, so it makes sense to allow this assignment. sfField2 is 10.
o[sfField3] = x * y; // This is problematic. The unit of x*y is basically "fee_unit drops". sfField3 is 1500, but 1500 what?
o[sfField4] = x / y; // Wat. The unit of x/y is basically "drops/fee_units". sfField4 has a value of 15, but again 15 what?
So the intention of this restriction is to prevent assignments like sfField3 & 4 from happening by accident. If one ever needs to be done intentionally, then we can add that case later. For now, I'd rather it be restrictive.
Codecov Report
@@ Coverage Diff @@
## develop #2910 +/- ##
===========================================
+ Coverage 70.78% 70.84% +0.06%
===========================================
Files 689 690 +1
Lines 54296 54442 +146
===========================================
+ Hits 38432 38571 +139
- Misses 15864 15871 +7
Continue to review full report at Codecov.
|
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
46b85bc to
93aa1c0
Compare
|
Rebased on to 1.3.0-b4, and resolved a couple of conflicts |
| Fees const& fees, bool bUnlimited) | ||
| { | ||
| if (fee == 0) | ||
| return fee; | ||
| auto lowestTerms1 = [](auto& a, auto& b) |
There was a problem hiding this comment.
nit: would prefer a better name than lowestTerms1 and lowestTerms2, or find a way to combine the two functions with if constexpr something like...
auto lowestTerms = [](auto& a, auto& b) {
auto get_value = [](auto v) {
if constexpr (std::is_same_v<decltype(v), TYPE_WITH_VALUE_MEMBER_FUNC>) {
return v.value();
} else {
return v;
}
};
if (auto const g = gcd(get_value(a), get_value(b)))
{
a /= g;
b /= g;
}
};
but no big deal...
|
@ximinez I didn't do a deep review of this PR, but I took a quick look and have two concerns. The first is the use of the boost units library. This is usually used for dimensional analysis, and it doesn't appear like that's how it's used here. For example, the PR defines three base dimensions: "drop", "fee", and "fee level". However, both "drop" and "fee" are measured in XRP, so they shouldn't be distinct dimensions - they are both currencies. In fact, "LoadFeeTrack.cpp" has a function "maybe_swap" what swaps a variable of type "fee" with one of type "drops", showing they have the same dimension. Also "fee levels" appears to be dimensionless: multiplying a "fee" by a "fee level" gives a fee. I don't object to making types for "fees" and "fee levels" and making the interface more type safe by doing so. However, using a dimensional analysis library for this seems like the wrong tool for the job. For this PR, there are very few combinations of drops, fees, and fee units that make sense. I'd just define those functions and lose the boost units library. My second concern is adding yet another type to represent XRP amounts. We can if we must, but is there something you need that |
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
93aa1c0 to
2f2dd16
Compare
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
2f2dd16 to
b8f7fa0
Compare
ximinez
force-pushed
the
x-strong-fee-typing
branch
3 times, most recently
from
ab9735c to
3b93c0e
Compare
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
cfeb6a8 to
ba2f2da
Compare
| std::uint64_t const | ||
| SYSTEM_CURRENCY_START = SYSTEM_CURRENCY_GIFT * SYSTEM_CURRENCY_USERS * SYSTEM_CURRENCY_PARTS; | ||
| inline | ||
| bool isLegalAmount (std::uint64_t amount) |
There was a problem hiding this comment.
I don't like this. It's a dangerous API to use: it accepts a value in drops, making it really easy for someone to do the wrong thing. And it only gives us a marginal benefit in a couple of cases, in fee vote processing.
I'd prefer that, wherever this overload is called from, we just construct an instance of XRPAmount and pass that into the isLegalAmount variant above. That way it's very clear exactly what's happening and you can't accidentally pass in XRP where drops are expected, or vice-versa.
There was a problem hiding this comment.
AFAIK, outside of unit test code, every place that works with XRP uses a value expressed in drops, so I don't know how risky this really is. The bigger problem is that in the few places this overload is used, it's also dangerous to construct an XRPAmount directly because an absurdly large value will overflow int64_t, and thus end up as a negative value, which would also be really bad.
I'm really happy that I was able to convert existing fees to use XRPAmount, but there are a couple of cases where the signed/unsigned conversions make it just a little awkward.
There are a couple of potential solutions:
- Get rid of the overload, check only for
int64_toverflow in these couple of places, then construct anXRPAmountas you suggested. - Define an unsigned
XRPAmount-like instance of theTaggedFeeclass. and use that as the overload parameter for these couple of cases. - Decide that using
XRPAmountwasn't the best idea and use aTaggedFeeinstance everywhere for fees.
Thoughts? I suspect @seelabs will have opinions about some of these options.
src/ripple/basics/XRPAmount.h
Outdated
| , private boost::equality_comparable <XRPAmount, std::int64_t> | ||
| , private boost::additive <XRPAmount, std::int64_t> | ||
| , private boost::dividable <XRPAmount, std::int64_t> | ||
| , private boost::modable <XRPAmount, std::int64_t> |
There was a problem hiding this comment.
I have some concerns about implementing multiplication and significant concerns about implementing division in the general purpose class that represents XRP.
src/ripple/app/misc/FeeVoteImpl.cpp
Outdated
| if(auto const v = baseFee.dropsAs<std::uint64_t>()) | ||
| obj[sfBaseFee] = *v; | ||
| else | ||
| Throw<std::runtime_error>("XRPAmount conversion out of range"); |
There was a problem hiding this comment.
This is a new exception and I'm not sure that we ever catch this. Can you point to the catch handler @ximinez?
There was a problem hiding this comment.
There is no catch handler. The original intent was that if the values here are not valid, then something has gone horribly wrong, so the intention was to allow the throw to crash the program.
However, I've given your feedback some thought, and came up with two alternate approaches that I think address all of your concerns
- 7ba8fbb (Comparison to develop: develop...ximinez:strong-xrpfee). This change makes a new class,
XRPFee, which is derived fromXRPAmount, and adds the functionality that isn't appropriate forXRPAmountincluding: division,dropsAs.decimalXRP,jsonClipped, and the>>operator. This allows us to continue to useXRPAmountas the class to represent XRP / drops, while still having functionality to represent fees when we need to. It also gets rid of these conversion throws by allowingdropsAsto return a default value if the stored value is out of range, and uses the fee values from the LCL as those defaults. - ce568a5 (Comparison to develop: develop...ximinez:strong-template). This change basically puts
XRPAmountback in protocol (with a few improvements), and createsXRPFeeas an instance ofTaggedFee, and then converts values as necessary. This approach has already been rejected once thanks to @seelabs, but it does also get rid of the throws because the fees are always the right type throughout the voting process. I also don't like this approach, because there are so many conversions between the two types that I worry about one of them being done incorrectly.
So, @nbougalis, @seelabs, thoughts?
There was a problem hiding this comment.
@ximinez What do you think about removing the division, mod, increment, and decrement functions from XRPAmount? Here's a patch that does so: seelabs@6cf2c38
The only place that loses some of the type safety you added was the lambda in scaleFeeLoad, but I think that's OK, and on balance I think that's worth it to remove those operators (which I agree with @nbougalis, I'd rather they weren't part of the XRPAmount class).
I did a patch that got rid of the multiplication operator, but I think that operator is worth keeping. We only allow multiplication by a scalar, and it's useful for calculating reserves (multiply by owner count), scaling fees, as converting "whole xrps" to drops (tho that could easily be a function if we wanted).
See what you think of this patch, and if @nbougalis is OK with keeping multiplication, I'd rather do something like this than derive a subclass from XRPAmount.
Note: this does not solve the issues with throwing, but does solve the operators problem.
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
ba2f2da to
0ce6ae6
Compare
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
d9a61cb to
31281cb
Compare
src/ripple/basics/XRPAmount.h
Outdated
| if ((drops_ > std::numeric_limits<Dest>::max()) || | ||
| (!std::numeric_limits<Dest>::is_signed && drops_ < 0) || | ||
| (std::numeric_limits<Dest>::is_signed && | ||
| drops_ < std::numeric_limits<Dest>::lowest())) |
There was a problem hiding this comment.
Fine as-is. However, we've been moving toward indenting as the .clang_format in the project indents. That current does not add the extra space.
ximinez
force-pushed
the
x-strong-fee-typing
branch
2 times, most recently
from
52df1bb to
2cc5476
Compare
|
@mellery451 @jwbusch @nbougalis @seelabs |
| @@ -96,21 +96,17 @@ scaleFeeLoad(FeeUnit64 fee, LoadFeeTrack const& feeTrack, | |||
| // compatible. This function is an exception. | |||
| auto lowestTerms = [](auto& a, auto& b) | |||
| { | |||
| auto value = [](auto val) | |||
| auto const value = [](auto val) | |||
There was a problem hiding this comment.
I'd remove the const from the lambdas. Declaring a lambda const doesn't add any restrictions (the lambda's operator()(...) is already const).
Edit: I suppose you could argue that it distinguishes between mutable and non-mutable lambdas. However, we haven't tended to declare our other lambdas const. I'd stick to what the rest of the codebase does (and mutable lambdas are pretty rarely used anyway)
There was a problem hiding this comment.
That's fair. I was thinking that const didn't hurt, particularly when I captured it in the second lambda, but it makes sense to remove it, especially if I remove the second lambda.
| { | ||
| if constexpr(std::is_arithmetic_v<decltype(val)>) | ||
| return val; | ||
| else | ||
| return val.value(); | ||
| }; | ||
|
|
||
| auto divide = [](auto& val, auto gcd) | ||
| auto const divide = [&value](auto& val, auto gcd) |
There was a problem hiding this comment.
I'd remove this lambda. At this point the lambda is so simple that it's better to just replace the call to the lambda with this code directly.
src/ripple/app/misc/FeeVoteImpl.cpp
Outdated
| @@ -171,7 +171,7 @@ FeeVoteImpl::doVoting( | |||
| if (val->isFieldPresent (sfBaseFee)) | |||
| { | |||
| auto const vote = val->getFieldU64 (sfBaseFee); | |||
| if (isLegalAmount(vote)) | |||
| if (vote <= std::numeric_limits<XRPAmount::value_type>::max()) | |||
There was a problem hiding this comment.
I think I prefer the lower amount restriction in isLegalAmount to the larger amount in value_type. Is there a motivation for changing this that I'm missing?
There was a problem hiding this comment.
Yes. vote is a uint64. I got rid of the isLegalAmount override that explicitly takes a uint64, so the only remaining definition takes an XRPAmount. XRPAmount has an underlying int64 value is implicitly constructed from any integral value. If I call isLegalAmount(vote), and vote is larger than the int64 limit, it will map to a negative value, and thus isLegalAmount will return true, which is a logic error.
I'll change it to check both.
|
Looked again, latest looks good 👍 |
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
abbf1eb to
3dc3a3c
Compare
ximinez
added
the
Passed
|
Squashed (mostly) and rebased on to 1.4.0-rc1. |
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
3dc3a3c to
c45f68d
Compare
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
c45f68d to
58ca425
Compare
|
Squashed and rebased on to 1.5.0-b1 to resolve conflicts. |
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
58ca425 to
279eedd
Compare
* Uses existing XRPAmount with units for drops, and a new TaggedFee for fee units (LoadFeeTrack), and fee levels (TxQ). * Resolves XRPLF#2451
ximinez
force-pushed
the
x-strong-fee-typing
branch
from
279eedd to
e3b5b80
Compare
fee units (LoadFeeTrack), and fee levels (TxQ).
This is a rewrite of the closed PR #2884.
While investigating #2451, I decided to do an audit of all fee calculations. I found a couple where units were not used correctly (eg. treating a drops value as fee units, and applying the conversion again), including those mentioned in the original problem. I figured that rather than fixing problems one at a time only to risk them being reintroduced, I'd convert all usages of fee values into a typed variable rather than a generic
uint32_toruint64_t.Implementation notes:
unsafe_castalongsidesafe_cast, because it helped me better visualize some of what was going on.Here are links to the locations in 1.2.4 (so the links will stay consistent) that fell out of this work as using the wrong units.
calculateBaseFeereturns fee units, but the result is treated as drops.calculateBaseFeereturns fee units, but the result is treated as drops.ApplyContextwants fee units, not drops.