From 1365039bd0753a373f98465a9d8362d736a8543e Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 27 Apr 2022 18:30:29 -0400 Subject: [PATCH 1/8] Convert fee voting and protocol messages to use XRPAmounts * Introduces amendment `featureXRPFees` * Includes Validations, Change transactions, the "Fees" ledger object, and subscription messages. --- src/ripple/app/consensus/RCLConsensus.cpp | 3 +- src/ripple/app/ledger/Ledger.cpp | 71 ++++--- src/ripple/app/misc/FeeVote.h | 8 +- src/ripple/app/misc/FeeVoteImpl.cpp | 208 +++++++++++++-------- src/ripple/app/misc/LoadFeeTrack.h | 2 +- src/ripple/app/misc/NetworkOPs.cpp | 27 ++- src/ripple/app/misc/impl/LoadFeeTrack.cpp | 60 +----- src/ripple/app/misc/impl/TxQ.cpp | 4 +- src/ripple/app/tx/applySteps.h | 2 +- src/ripple/app/tx/impl/ApplyContext.cpp | 2 +- src/ripple/app/tx/impl/ApplyContext.h | 4 +- src/ripple/app/tx/impl/Change.cpp | 64 ++++++- src/ripple/app/tx/impl/Change.h | 4 +- src/ripple/app/tx/impl/DeleteAccount.cpp | 15 +- src/ripple/app/tx/impl/DeleteAccount.h | 2 +- src/ripple/app/tx/impl/Escrow.cpp | 7 +- src/ripple/app/tx/impl/Escrow.h | 2 +- src/ripple/app/tx/impl/SetRegularKey.cpp | 4 +- src/ripple/app/tx/impl/SetRegularKey.h | 2 +- src/ripple/app/tx/impl/Transactor.cpp | 8 +- src/ripple/app/tx/impl/Transactor.h | 6 +- src/ripple/app/tx/impl/applySteps.cpp | 8 +- src/ripple/basics/FeeUnits.h | 5 - src/ripple/core/Config.h | 5 +- src/ripple/ledger/ReadView.h | 10 - src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/SField.h | 5 + src/ripple/protocol/SystemParameters.h | 2 +- src/ripple/protocol/impl/Feature.cpp | 1 + src/ripple/protocol/impl/LedgerFormats.cpp | 13 +- src/ripple/protocol/impl/SField.cpp | 5 + src/ripple/protocol/impl/STValidation.cpp | 5 + src/ripple/protocol/impl/TxFormats.cpp | 13 +- src/ripple/protocol/jss.h | 3 + src/ripple/rpc/handlers/NoRippleCheck.cpp | 2 +- src/ripple/rpc/impl/TransactionSign.cpp | 9 +- src/test/app/LoadFeeTrack_test.cpp | 25 ++- src/test/app/PseudoTx_test.cpp | 34 +++- src/test/app/TxQ_test.cpp | 1 - src/test/basics/FeeUnits_test.cpp | 40 ++-- src/test/ledger/Invariants_test.cpp | 2 +- src/test/rpc/Subscribe_test.cpp | 31 ++- 42 files changed, 432 insertions(+), 295 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index aec747e094c..12a150a4a54 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -839,7 +839,8 @@ RCLConsensus::Adaptor::validate( if (ledger.ledger_->isVotingLedger()) { // Fees: - feeVote_->doValidation(ledger.ledger_->fees(), v); + feeVote_->doValidation( + ledger.ledger_->fees(), ledger.ledger_->rules(), v); // Amendments // FIXME: pass `v` and have the function insert the array diff --git a/src/ripple/app/ledger/Ledger.cpp b/src/ripple/app/ledger/Ledger.cpp index 71311448505..f82a26c8275 100644 --- a/src/ripple/app/ledger/Ledger.cpp +++ b/src/ripple/app/ledger/Ledger.cpp @@ -591,29 +591,9 @@ Ledger::setup(Config const& config) { bool ret = true; - fees_.base = config.FEE_DEFAULT; - fees_.units = config.TRANSACTION_FEE_BASE; - fees_.reserve = config.FEE_ACCOUNT_RESERVE; - fees_.increment = config.FEE_OWNER_RESERVE; - try { - if (auto const sle = read(keylet::fees())) - { - // VFALCO NOTE Why getFieldIndex and not isFieldPresent? - - if (sle->getFieldIndex(sfBaseFee) != -1) - fees_.base = sle->getFieldU64(sfBaseFee); - - if (sle->getFieldIndex(sfReferenceFeeUnits) != -1) - fees_.units = sle->getFieldU32(sfReferenceFeeUnits); - - if (sle->getFieldIndex(sfReserveBase) != -1) - fees_.reserve = sle->getFieldU32(sfReserveBase); - - if (sle->getFieldIndex(sfReserveIncrement) != -1) - fees_.increment = sle->getFieldU32(sfReserveIncrement); - } + rules_ = makeRulesGivenLedger(*this, config.features); } catch (SHAMapMissingNode const&) { @@ -624,9 +604,56 @@ Ledger::setup(Config const& config) Rethrow(); } + fees_.base = config.FEE_DEFAULT; + fees_.reserve = config.FEE_ACCOUNT_RESERVE; + fees_.increment = config.FEE_OWNER_RESERVE; + try { - rules_ = makeRulesGivenLedger(*this, config.features); + if (auto const sle = read(keylet::fees())) + { + bool oldFees = false; + bool newFees = false; + { + auto const baseFee = sle->at(~sfBaseFee); + auto const reserveBase = sle->at(~sfReserveBase); + auto const reserveIncrement = sle->at(~sfReserveIncrement); + if (baseFee) + fees_.base = *baseFee; + if (reserveBase) + fees_.reserve = *reserveBase; + if (reserveIncrement) + fees_.increment = *reserveIncrement; + oldFees = baseFee || reserveBase || reserveIncrement; + } + { + auto const baseFeeXRP = sle->at(~sfBaseFeeXRP); + auto const reserveBaseXRP = sle->at(~sfReserveBaseXRP); + auto const reserveIncrementXRP = + sle->at(~sfReserveIncrementXRP); + auto assign = [&ret]( + XRPAmount& dest, + std::optional const& src) { + if (src) + { + if (src->native()) + dest = src->xrp(); + else + ret = false; + } + }; + assign(fees_.base, baseFeeXRP); + assign(fees_.reserve, reserveBaseXRP); + assign(fees_.increment, reserveIncrementXRP); + newFees = baseFeeXRP || reserveBaseXRP || reserveIncrementXRP; + } + if (oldFees && newFees) + // Should be all of one or the other, but not both + ret = false; + if (!rules_.enabled(featureXRPFees) && newFees) + // Can't populate the new fees before the amendment is enabled + ret = false; + } } catch (SHAMapMissingNode const&) { diff --git a/src/ripple/app/misc/FeeVote.h b/src/ripple/app/misc/FeeVote.h index d8948a150b3..4fff64f7de3 100644 --- a/src/ripple/app/misc/FeeVote.h +++ b/src/ripple/app/misc/FeeVote.h @@ -42,9 +42,6 @@ class FeeVote /** The cost of a reference transaction in drops. */ XRPAmount reference_fee{10}; - /** The cost of a reference transaction in fee units. */ - static constexpr FeeUnit32 reference_fee_units{10}; - /** The account reserve requirement in drops. */ XRPAmount account_reserve{10 * DROPS_PER_XRP}; @@ -60,7 +57,10 @@ class FeeVote @param baseValidation */ virtual void - doValidation(Fees const& lastFees, STValidation& val) = 0; + doValidation( + Fees const& lastFees, + Rules const& rules, + STValidation& val) = 0; /** Cast our local vote on the fee. diff --git a/src/ripple/app/misc/FeeVoteImpl.cpp b/src/ripple/app/misc/FeeVoteImpl.cpp index faa8a259543..928d0b40b12 100644 --- a/src/ripple/app/misc/FeeVoteImpl.cpp +++ b/src/ripple/app/misc/FeeVoteImpl.cpp @@ -33,44 +33,70 @@ class VotableValue { private: using value_type = XRPAmount; - value_type const mCurrent; // The current setting - value_type const mTarget; // The setting we want - std::map mVoteMap; + value_type const current_; // The current setting + value_type const target_; // The setting we want + std::map voteMap_; + std::optional vote_; public: VotableValue(value_type current, value_type target) - : mCurrent(current), mTarget(target) + : current_(current), target_(target) { // Add our vote - ++mVoteMap[mTarget]; + ++voteMap_[target_]; } void addVote(value_type vote) { - ++mVoteMap[vote]; + ++voteMap_[vote]; } void noVote() { - addVote(mCurrent); + addVote(current_); } + void + setVotes(); + value_type getVotes() const; + + template + Dest + getVotesAs() const; + + bool + voteChange() const + { + return getVotes() != current_; + } }; +void +VotableValue::setVotes() +{ + // Need to explicitly remove any value from vote_, because it will be + // returned from getVotes() if set. + vote_.reset(); + vote_ = getVotes(); +} + auto VotableValue::getVotes() const -> value_type { - value_type ourVote = mCurrent; + if (vote_) + return *vote_; + + value_type ourVote = current_; int weight = 0; - for (auto const& [key, val] : mVoteMap) + for (auto const& [key, val] : voteMap_) { // Take most voted value between current and target, inclusive - if ((key <= std::max(mTarget, mCurrent)) && - (key >= std::min(mTarget, mCurrent)) && (val > weight)) + if ((key <= std::max(target_, current_)) && + (key >= std::min(target_, current_)) && (val > weight)) { ourVote = key; weight = val; @@ -80,6 +106,13 @@ VotableValue::getVotes() const -> value_type return ourVote; } +template +Dest +VotableValue::getVotesAs() const +{ + return getVotes().dropsAs(current_); +} + } // namespace detail //------------------------------------------------------------------------------ @@ -94,7 +127,8 @@ class FeeVoteImpl : public FeeVote FeeVoteImpl(Setup const& setup, beast::Journal journal); void - doValidation(Fees const& lastFees, STValidation& val) override; + doValidation(Fees const& lastFees, Rules const& rules, STValidation& val) + override; void doVoting( @@ -111,7 +145,10 @@ FeeVoteImpl::FeeVoteImpl(Setup const& setup, beast::Journal journal) } void -FeeVoteImpl::doValidation(Fees const& lastFees, STValidation& v) +FeeVoteImpl::doValidation( + Fees const& lastFees, + Rules const& rules, + STValidation& v) { // Values should always be in a valid range (because the voting process // will ignore out-of-range values) but if we detect such a case, we do @@ -121,8 +158,10 @@ FeeVoteImpl::doValidation(Fees const& lastFees, STValidation& v) JLOG(journal_.info()) << "Voting for base fee of " << target_.reference_fee; - if (auto const f = target_.reference_fee.dropsAs()) - v.setFieldU64(sfBaseFee, *f); + if (rules.enabled(featureXRPFees)) + v[sfBaseFeeXRP] = target_.reference_fee; + else if (auto const f = target_.reference_fee.dropsAs()) + v[sfBaseFee] = *f; } if (lastFees.accountReserve(0) != target_.account_reserve) @@ -130,8 +169,11 @@ FeeVoteImpl::doValidation(Fees const& lastFees, STValidation& v) JLOG(journal_.info()) << "Voting for base reserve of " << target_.account_reserve; - if (auto const f = target_.account_reserve.dropsAs()) - v.setFieldU32(sfReserveBase, *f); + if (rules.enabled(featureXRPFees)) + v[sfReserveBaseXRP] = target_.account_reserve; + else if ( + auto const f = target_.account_reserve.dropsAs()) + v[sfReserveBase] = *f; } if (lastFees.increment != target_.owner_reserve) @@ -139,8 +181,10 @@ FeeVoteImpl::doValidation(Fees const& lastFees, STValidation& v) JLOG(journal_.info()) << "Voting for reserve increment of " << target_.owner_reserve; - if (auto const f = target_.owner_reserve.dropsAs()) - v.setFieldU32(sfReserveIncrement, *f); + if (rules.enabled(featureXRPFees)) + v[sfReserveIncrementXRP] = target_.owner_reserve; + else if (auto const f = target_.owner_reserve.dropsAs()) + v[sfReserveIncrement] = *f; } } @@ -151,7 +195,7 @@ FeeVoteImpl::doVoting( std::shared_ptr const& initialPosition) { // LCL must be flag ledger - assert(isFlagLedger(lastClosedLedger->seq())); + assert(lastClosedLedger && isFlagLedger(lastClosedLedger->seq())); detail::VotableValue baseFeeVote( lastClosedLedger->fees().base, target_.reference_fee); @@ -162,79 +206,89 @@ FeeVoteImpl::doVoting( detail::VotableValue incReserveVote( lastClosedLedger->fees().increment, target_.owner_reserve); - for (auto const& val : set) - { - if (val->isTrusted()) + auto const& rules = lastClosedLedger->rules(); + auto doVote = [&rules]( + std::shared_ptr const& val, + detail::VotableValue& value, + SF_AMOUNT const& xrpField, + auto const& valueField) { + if (auto const field = ~val->at(~xrpField); + field && rules.enabled(featureXRPFees) && field->native()) { - if (val->isFieldPresent(sfBaseFee)) - { - using xrptype = XRPAmount::value_type; - auto const vote = val->getFieldU64(sfBaseFee); - if (vote <= std::numeric_limits::max() && - isLegalAmount(XRPAmount{unsafe_cast(vote)})) - baseFeeVote.addVote( - XRPAmount{unsafe_cast(vote)}); - else - // Invalid amounts will be treated as if they're - // not provided. Don't throw because this value is - // provided by an external entity. - baseFeeVote.noVote(); - } - else - { - baseFeeVote.noVote(); - } - - if (val->isFieldPresent(sfReserveBase)) - { - baseReserveVote.addVote( - XRPAmount{val->getFieldU32(sfReserveBase)}); - } + auto const vote = field->xrp(); + if (isLegalAmount(vote)) + value.addVote(vote); else - { - baseReserveVote.noVote(); - } - - if (val->isFieldPresent(sfReserveIncrement)) - { - incReserveVote.addVote( - XRPAmount{val->getFieldU32(sfReserveIncrement)}); - } + value.noVote(); + } + else if (auto const field = val->at(~valueField)) + { + using xrptype = XRPAmount::value_type; + auto const vote = *field; + if (vote <= std::numeric_limits::max() && + isLegalAmount(XRPAmount{unsafe_cast(vote)})) + value.addVote( + XRPAmount{unsafe_cast(vote)}); else - { - incReserveVote.noVote(); - } + // Invalid amounts will be treated as if they're + // not provided. Don't throw because this value is + // provided by an external entity. + value.noVote(); } + else + { + value.noVote(); + } + }; + + for (auto const& val : set) + { + if (!val->isTrusted()) + continue; + doVote(val, baseFeeVote, sfBaseFeeXRP, sfBaseFee); + doVote(val, baseReserveVote, sfReserveBaseXRP, sfReserveBase); + doVote(val, incReserveVote, sfReserveIncrementXRP, sfReserveIncrement); } // choose our positions - // If any of the values are invalid, send the current values. - auto const baseFee = baseFeeVote.getVotes().dropsAs( - lastClosedLedger->fees().base); - auto const baseReserve = baseReserveVote.getVotes().dropsAs( - lastClosedLedger->fees().accountReserve(0)); - auto const incReserve = incReserveVote.getVotes().dropsAs( - lastClosedLedger->fees().increment); - constexpr FeeUnit32 feeUnits = Setup::reference_fee_units; + baseFeeVote.setVotes(); + baseReserveVote.setVotes(); + incReserveVote.setVotes(); + auto const seq = lastClosedLedger->info().seq + 1; // add transactions to our position - if ((baseFee != lastClosedLedger->fees().base) || - (baseReserve != lastClosedLedger->fees().accountReserve(0)) || - (incReserve != lastClosedLedger->fees().increment)) + if ((baseFeeVote.voteChange()) || (baseReserveVote.voteChange()) || + (incReserveVote.voteChange())) { - JLOG(journal_.warn()) << "We are voting for a fee change: " << baseFee - << "/" << baseReserve << "/" << incReserve; + JLOG(journal_.warn()) + << "We are voting for a fee change: " << baseFeeVote.getVotes() + << "/" << baseReserveVote.getVotes() << "/" + << incReserveVote.getVotes(); STTx feeTx( ttFEE, - [seq, baseFee, baseReserve, incReserve, feeUnits](auto& obj) { + [&rules, seq, &baseFeeVote, &baseReserveVote, &incReserveVote]( + auto& obj) { obj[sfAccount] = AccountID(); obj[sfLedgerSequence] = seq; - obj[sfBaseFee] = baseFee; - obj[sfReserveBase] = baseReserve; - obj[sfReserveIncrement] = incReserve; - obj[sfReferenceFeeUnits] = feeUnits.fee(); + if (rules.enabled(featureXRPFees)) + { + obj[sfBaseFeeXRP] = baseFeeVote.getVotes(); + obj[sfReserveBaseXRP] = baseReserveVote.getVotes(); + obj[sfReserveIncrementXRP] = incReserveVote.getVotes(); + } + else + { + // Without the featureXRPFees amendment, these fields are + // required. + obj[sfBaseFee] = baseFeeVote.getVotesAs(); + obj[sfReserveBase] = + baseReserveVote.getVotesAs(); + obj[sfReserveIncrement] = + incReserveVote.getVotesAs(); + obj[sfReferenceFeeUnits] = Config::FEE_UNITS_DEPRECATED; + } }); uint256 txID = feeTx.getTransactionID(); diff --git a/src/ripple/app/misc/LoadFeeTrack.h b/src/ripple/app/misc/LoadFeeTrack.h index 0109468cb99..d670c0b7e11 100644 --- a/src/ripple/app/misc/LoadFeeTrack.h +++ b/src/ripple/app/misc/LoadFeeTrack.h @@ -161,7 +161,7 @@ class LoadFeeTrack final // Scale using load as well as base rate XRPAmount scaleFeeLoad( - FeeUnit64 fee, + XRPAmount fee, LoadFeeTrack const& feeTrack, Fees const& fees, bool bUnlimited); diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 8dff1af7b2b..aac72c46166 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2171,15 +2171,30 @@ NetworkOPsImp::pubValidation(std::shared_ptr const& val) if (auto const loadFee = (*val)[~sfLoadFee]) jvObj[jss::load_fee] = *loadFee; - if (auto const baseFee = (*val)[~sfBaseFee]) + if (auto const baseFee = val->at(~sfBaseFee)) jvObj[jss::base_fee] = static_cast(*baseFee); - if (auto const reserveBase = (*val)[~sfReserveBase]) + if (auto const reserveBase = val->at(~sfReserveBase)) jvObj[jss::reserve_base] = *reserveBase; - if (auto const reserveInc = (*val)[~sfReserveIncrement]) + if (auto const reserveInc = val->at(~sfReserveIncrement)) jvObj[jss::reserve_inc] = *reserveInc; + // (The ~ operator converts the Proxy to a std::optional, which + // simplifies later operations) + if (auto const baseFeeXRP = ~val->at(~sfBaseFeeXRP); + baseFeeXRP && baseFeeXRP->native()) + jvObj[jss::base_fee_drops] = baseFeeXRP->xrp().jsonClipped(); + + if (auto const reserveBaseXRP = ~val->at(~sfReserveBaseXRP); + reserveBaseXRP && reserveBaseXRP->native()) + jvObj[jss::reserve_base_drops] = + reserveBaseXRP->xrp().jsonClipped(); + + if (auto const reserveIncXRP = ~val->at(~sfReserveIncrementXRP); + reserveIncXRP && reserveIncXRP->native()) + jvObj[jss::reserve_inc_drops] = reserveIncXRP->xrp().jsonClipped(); + for (auto i = mStreamMaps[sValidations].begin(); i != mStreamMaps[sValidations].end();) { @@ -2883,7 +2898,8 @@ NetworkOPsImp::pubLedger(std::shared_ptr const& lpAccepted) jvObj[jss::ledger_time] = Json::Value::UInt( lpAccepted->info().closeTime.time_since_epoch().count()); - jvObj[jss::fee_ref] = lpAccepted->fees().units.jsonClipped(); + if (!lpAccepted->rules().enabled(featureXRPFees)) + jvObj[jss::fee_ref] = Config::FEE_UNITS_DEPRECATED; jvObj[jss::fee_base] = lpAccepted->fees().base.jsonClipped(); jvObj[jss::reserve_base] = lpAccepted->fees().accountReserve(0).jsonClipped(); @@ -3889,7 +3905,8 @@ NetworkOPsImp::subLedger(InfoSub::ref isrListener, Json::Value& jvResult) jvResult[jss::ledger_hash] = to_string(lpClosed->info().hash); jvResult[jss::ledger_time] = Json::Value::UInt( lpClosed->info().closeTime.time_since_epoch().count()); - jvResult[jss::fee_ref] = lpClosed->fees().units.jsonClipped(); + if (!lpClosed->rules().enabled(featureXRPFees)) + jvResult[jss::fee_ref] = Config::FEE_UNITS_DEPRECATED; jvResult[jss::fee_base] = lpClosed->fees().base.jsonClipped(); jvResult[jss::reserve_base] = lpClosed->fees().accountReserve(0).jsonClipped(); diff --git a/src/ripple/app/misc/impl/LoadFeeTrack.cpp b/src/ripple/app/misc/impl/LoadFeeTrack.cpp index 01445d4e580..11679c9a66e 100644 --- a/src/ripple/app/misc/impl/LoadFeeTrack.cpp +++ b/src/ripple/app/misc/impl/LoadFeeTrack.cpp @@ -87,30 +87,13 @@ LoadFeeTrack::lowerLocalFee() // Scale using load as well as base rate XRPAmount scaleFeeLoad( - FeeUnit64 fee, + XRPAmount fee, LoadFeeTrack const& feeTrack, Fees const& fees, bool bUnlimited) { if (fee == 0) - return XRPAmount{0}; - - // Normally, types with different units wouldn't be mathematically - // compatible. This function is an exception. - auto lowestTerms = [](auto& a, auto& b) { - auto value = [](auto val) { - if constexpr (std::is_arithmetic_v) - return val; - else - return val.value(); - }; - - if (auto const g = std::gcd(value(a), value(b))) - { - a = value(a) / g; - b = value(b) / g; - } - }; + return fee; // Collect the fee rates auto [feeFactor, uRemFee] = feeTrack.getScalingFactors(); @@ -120,45 +103,12 @@ scaleFeeLoad( if (bUnlimited && (feeFactor > uRemFee) && (feeFactor < (4 * uRemFee))) feeFactor = uRemFee; - XRPAmount baseFee{fees.base}; // Compute: - // fee = fee * baseFee * feeFactor / (fees.units * lftNormalFee); + // fee = fee * feeFactor / (lftNormalFee); // without overflow, and as accurately as possible - // The denominator of the fraction we're trying to compute. - // fees.units and lftNormalFee are both 32 bit, - // so the multiplication can't overflow. - auto den = FeeUnit64{fees.units} * - safe_cast(feeTrack.getLoadBase()); - // Reduce fee * baseFee * feeFactor / (fees.units * lftNormalFee) - // to lowest terms. - lowestTerms(fee, den); - lowestTerms(baseFee, den); - lowestTerms(feeFactor, den); - - // fee and baseFee are 64 bit, feeFactor is 32 bit - // Order fee and baseFee largest first - // Normally, these types wouldn't be comparable or swappable. - // This function is an exception. - if (fee.value() < baseFee.value()) - { - auto tmp = fee.value(); - fee = baseFee.value(); - baseFee = tmp; - } - // double check - assert(fee.value() >= baseFee.value()); - - // If baseFee * feeFactor overflows, the final result will overflow - XRPAmount const baseFeeOverflow{ - std::numeric_limits::max() / feeFactor}; - if (baseFee > baseFeeOverflow) - { - Throw("scaleFeeLoad"); - } - baseFee *= feeFactor; - - auto const result = mulDiv(fee, baseFee, den); + auto const result = mulDiv( + fee, feeFactor, safe_cast(feeTrack.getLoadBase())); if (!result.first) Throw("scaleFeeLoad"); return result.second; diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index 59559cf24c6..c7267565dbd 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -38,7 +38,7 @@ static FeeLevel64 getFeeLevelPaid(ReadView const& view, STTx const& tx) { auto const [baseFee, effectiveFeePaid] = [&view, &tx]() { - XRPAmount baseFee = view.fees().toDrops(calculateBaseFee(view, tx)); + XRPAmount baseFee = calculateBaseFee(view, tx); XRPAmount feePaid = tx[sfFee].xrp(); // If baseFee is 0 then the cost of a basic transaction is free. @@ -1758,7 +1758,7 @@ TxQ::getTxRequiredFeeAndSeq( std::lock_guard lock(mutex_); auto const snapshot = feeMetrics_.getSnapshot(); - auto const baseFee = view.fees().toDrops(calculateBaseFee(view, *tx)); + auto const baseFee = calculateBaseFee(view, *tx); auto const fee = FeeMetrics::scaleFeeLevel(snapshot, view); auto const sle = view.read(keylet::account(account)); diff --git a/src/ripple/app/tx/applySteps.h b/src/ripple/app/tx/applySteps.h index 9993ba0c525..ede7bd8cc09 100644 --- a/src/ripple/app/tx/applySteps.h +++ b/src/ripple/app/tx/applySteps.h @@ -300,7 +300,7 @@ preclaim( @return The base fee. */ -FeeUnit64 +XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx); /** Return the minimum fee that an "ordinary" transaction would pay. diff --git a/src/ripple/app/tx/impl/ApplyContext.cpp b/src/ripple/app/tx/impl/ApplyContext.cpp index eb68fe5d39c..27287241637 100644 --- a/src/ripple/app/tx/impl/ApplyContext.cpp +++ b/src/ripple/app/tx/impl/ApplyContext.cpp @@ -33,7 +33,7 @@ ApplyContext::ApplyContext( OpenView& base, STTx const& tx_, TER preclaimResult_, - FeeUnit64 baseFee_, + XRPAmount baseFee_, ApplyFlags flags, beast::Journal journal_) : app(app_) diff --git a/src/ripple/app/tx/impl/ApplyContext.h b/src/ripple/app/tx/impl/ApplyContext.h index 1a47826610e..415054ef4bb 100644 --- a/src/ripple/app/tx/impl/ApplyContext.h +++ b/src/ripple/app/tx/impl/ApplyContext.h @@ -40,14 +40,14 @@ class ApplyContext OpenView& base, STTx const& tx, TER preclaimResult, - FeeUnit64 baseFee, + XRPAmount baseFee, ApplyFlags flags, beast::Journal = beast::Journal{beast::Journal::getNullSink()}); Application& app; STTx const& tx; TER const preclaimResult; - FeeUnit64 const baseFee; + XRPAmount const baseFee; beast::Journal const journal; ApplyView& diff --git a/src/ripple/app/tx/impl/Change.cpp b/src/ripple/app/tx/impl/Change.cpp index 93ed1a04f92..5a91e172b26 100644 --- a/src/ripple/app/tx/impl/Change.cpp +++ b/src/ripple/app/tx/impl/Change.cpp @@ -90,8 +90,42 @@ Change::preclaim(PreclaimContext const& ctx) switch (ctx.tx.getTxnType()) { - case ttAMENDMENT: case ttFEE: + if (ctx.view.rules().enabled(featureXRPFees)) + { + if (!ctx.tx.isFieldPresent(sfBaseFeeXRP) || + !ctx.tx.isFieldPresent(sfReserveBaseXRP) || + !ctx.tx.isFieldPresent(sfReserveIncrementXRP)) + return temMALFORMED; + // The transaction should only have one set of fields or the + // other. + if (ctx.tx.isFieldPresent(sfBaseFee) || + ctx.tx.isFieldPresent(sfReferenceFeeUnits) || + ctx.tx.isFieldPresent(sfReserveBase) || + ctx.tx.isFieldPresent(sfReserveIncrement)) + return temMALFORMED; + } + else + { + // The transaction format formerly enforced these fields as + // required. With featureXRPFees, those fields were made + // optional with the expectation that they won't be used once + // the feature is enabled. Since the feature hasn't yet + // been enabled, they should all still be here. + if (!ctx.tx.isFieldPresent(sfBaseFee) || + !ctx.tx.isFieldPresent(sfReferenceFeeUnits) || + !ctx.tx.isFieldPresent(sfReserveBase) || + !ctx.tx.isFieldPresent(sfReserveIncrement)) + return temMALFORMED; + // The transaction should only have one or the other. If the new + // fields are present without the amendment, that's bad, too. + if (ctx.tx.isFieldPresent(sfBaseFeeXRP) || + ctx.tx.isFieldPresent(sfReserveBaseXRP) || + ctx.tx.isFieldPresent(sfReserveIncrementXRP)) + return temDISABLED; + } + return tesSUCCESS; + case ttAMENDMENT: case ttUNL_MODIFY: return tesSUCCESS; default: @@ -315,13 +349,27 @@ Change::applyFee() feeObject = std::make_shared(k); view().insert(feeObject); } - - feeObject->setFieldU64(sfBaseFee, ctx_.tx.getFieldU64(sfBaseFee)); - feeObject->setFieldU32( - sfReferenceFeeUnits, ctx_.tx.getFieldU32(sfReferenceFeeUnits)); - feeObject->setFieldU32(sfReserveBase, ctx_.tx.getFieldU32(sfReserveBase)); - feeObject->setFieldU32( - sfReserveIncrement, ctx_.tx.getFieldU32(sfReserveIncrement)); + auto set = [](SLE::pointer& feeObject, STTx const& tx, auto const& field) { + feeObject->at(field) = tx[field]; + }; + if (view().rules().enabled(featureXRPFees)) + { + set(feeObject, ctx_.tx, sfBaseFeeXRP); + set(feeObject, ctx_.tx, sfReserveBaseXRP); + set(feeObject, ctx_.tx, sfReserveIncrementXRP); + // Ensure the old fields are removed + feeObject->makeFieldAbsent(sfBaseFee); + feeObject->makeFieldAbsent(sfReferenceFeeUnits); + feeObject->makeFieldAbsent(sfReserveBase); + feeObject->makeFieldAbsent(sfReserveIncrement); + } + else + { + set(feeObject, ctx_.tx, sfBaseFee); + set(feeObject, ctx_.tx, sfReferenceFeeUnits); + set(feeObject, ctx_.tx, sfReserveBase); + set(feeObject, ctx_.tx, sfReserveIncrement); + } view().update(feeObject); diff --git a/src/ripple/app/tx/impl/Change.h b/src/ripple/app/tx/impl/Change.h index 0ee7067b323..f366a5754ce 100644 --- a/src/ripple/app/tx/impl/Change.h +++ b/src/ripple/app/tx/impl/Change.h @@ -46,10 +46,10 @@ class Change : public Transactor void preCompute() override; - static FeeUnit64 + static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx) { - return FeeUnit64{0}; + return XRPAmount{0}; } static TER diff --git a/src/ripple/app/tx/impl/DeleteAccount.cpp b/src/ripple/app/tx/impl/DeleteAccount.cpp index da2244bca5e..3d9d83c0d35 100644 --- a/src/ripple/app/tx/impl/DeleteAccount.cpp +++ b/src/ripple/app/tx/impl/DeleteAccount.cpp @@ -52,20 +52,11 @@ DeleteAccount::preflight(PreflightContext const& ctx) return preflight2(ctx); } -FeeUnit64 +XRPAmount DeleteAccount::calculateBaseFee(ReadView const& view, STTx const& tx) { - // The fee required for AccountDelete is one owner reserve. But the - // owner reserve is stored in drops. We need to convert it to fee units. - Fees const& fees{view.fees()}; - std::pair const mulDivResult{ - mulDiv(fees.increment, safe_cast(fees.units), fees.base)}; - if (mulDivResult.first) - return mulDivResult.second; - - // If mulDiv returns false then overflow happened. Punt by using the - // standard calculation. - return Transactor::calculateBaseFee(view, tx); + // The fee required for AccountDelete is one owner reserve. + return view.fees().increment; } namespace { diff --git a/src/ripple/app/tx/impl/DeleteAccount.h b/src/ripple/app/tx/impl/DeleteAccount.h index b0dbaa5bc7e..0f298bb8596 100644 --- a/src/ripple/app/tx/impl/DeleteAccount.h +++ b/src/ripple/app/tx/impl/DeleteAccount.h @@ -38,7 +38,7 @@ class DeleteAccount : public Transactor static NotTEC preflight(PreflightContext const& ctx); - static FeeUnit64 + static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx); static TER diff --git a/src/ripple/app/tx/impl/Escrow.cpp b/src/ripple/app/tx/impl/Escrow.cpp index 7486dfaca4b..f8860ae4b56 100644 --- a/src/ripple/app/tx/impl/Escrow.cpp +++ b/src/ripple/app/tx/impl/Escrow.cpp @@ -338,15 +338,14 @@ EscrowFinish::preflight(PreflightContext const& ctx) return tesSUCCESS; } -FeeUnit64 +XRPAmount EscrowFinish::calculateBaseFee(ReadView const& view, STTx const& tx) { - FeeUnit64 extraFee{0}; + XRPAmount extraFee{0}; if (auto const fb = tx[~sfFulfillment]) { - extraFee += - safe_cast(view.fees().units) * (32 + (fb->size() / 16)); + extraFee += view.fees().base * (32 + (fb->size() / 16)); } return Transactor::calculateBaseFee(view, tx) + extraFee; diff --git a/src/ripple/app/tx/impl/Escrow.h b/src/ripple/app/tx/impl/Escrow.h index 20e57c85f67..c0f6333604b 100644 --- a/src/ripple/app/tx/impl/Escrow.h +++ b/src/ripple/app/tx/impl/Escrow.h @@ -57,7 +57,7 @@ class EscrowFinish : public Transactor static NotTEC preflight(PreflightContext const& ctx); - static FeeUnit64 + static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx); TER diff --git a/src/ripple/app/tx/impl/SetRegularKey.cpp b/src/ripple/app/tx/impl/SetRegularKey.cpp index 1b5a3eedea0..34a8b7d238c 100644 --- a/src/ripple/app/tx/impl/SetRegularKey.cpp +++ b/src/ripple/app/tx/impl/SetRegularKey.cpp @@ -24,7 +24,7 @@ namespace ripple { -FeeUnit64 +XRPAmount SetRegularKey::calculateBaseFee(ReadView const& view, STTx const& tx) { auto const id = tx.getAccountID(sfAccount); @@ -39,7 +39,7 @@ SetRegularKey::calculateBaseFee(ReadView const& view, STTx const& tx) if (sle && (!(sle->getFlags() & lsfPasswordSpent))) { // flag is armed and they signed with the right account - return FeeUnit64{0}; + return XRPAmount{0}; } } } diff --git a/src/ripple/app/tx/impl/SetRegularKey.h b/src/ripple/app/tx/impl/SetRegularKey.h index 53d832c17cc..402ee436ed5 100644 --- a/src/ripple/app/tx/impl/SetRegularKey.h +++ b/src/ripple/app/tx/impl/SetRegularKey.h @@ -39,7 +39,7 @@ class SetRegularKey : public Transactor static NotTEC preflight(PreflightContext const& ctx); - static FeeUnit64 + static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx); TER diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 9265d365647..4c1a7e726cd 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -137,7 +137,7 @@ Transactor::Transactor(ApplyContext& ctx) { } -FeeUnit64 +XRPAmount Transactor::calculateBaseFee(ReadView const& view, STTx const& tx) { // Returns the fee in fee units. @@ -145,7 +145,7 @@ Transactor::calculateBaseFee(ReadView const& view, STTx const& tx) // The computation has two parts: // * The base fee, which is the same for most transactions. // * The additional cost of each multisignature on the transaction. - FeeUnit64 const baseFee = safe_cast(view.fees().units); + XRPAmount const baseFee = view.fees().base; // Each signer adds one more baseFee to the minimum required fee // for the transaction. @@ -158,7 +158,7 @@ Transactor::calculateBaseFee(ReadView const& view, STTx const& tx) XRPAmount Transactor::minimumFee( Application& app, - FeeUnit64 baseFee, + XRPAmount baseFee, Fees const& fees, ApplyFlags flags) { @@ -166,7 +166,7 @@ Transactor::minimumFee( } TER -Transactor::checkFee(PreclaimContext const& ctx, FeeUnit64 baseFee) +Transactor::checkFee(PreclaimContext const& ctx, XRPAmount baseFee) { if (!ctx.tx[sfFee].native()) return temBAD_FEE; diff --git a/src/ripple/app/tx/impl/Transactor.h b/src/ripple/app/tx/impl/Transactor.h index c54f5a1a365..cc280e6141d 100644 --- a/src/ripple/app/tx/impl/Transactor.h +++ b/src/ripple/app/tx/impl/Transactor.h @@ -132,13 +132,13 @@ class Transactor checkPriorTxAndLastLedger(PreclaimContext const& ctx); static TER - checkFee(PreclaimContext const& ctx, FeeUnit64 baseFee); + checkFee(PreclaimContext const& ctx, XRPAmount baseFee); static NotTEC checkSign(PreclaimContext const& ctx); // Returns the fee in fee units, not scaled for load. - static FeeUnit64 + static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx); static TER @@ -182,7 +182,7 @@ class Transactor static XRPAmount minimumFee( Application& app, - FeeUnit64 baseFee, + XRPAmount baseFee, Fees const& fees, ApplyFlags flags); diff --git a/src/ripple/app/tx/impl/applySteps.cpp b/src/ripple/app/tx/impl/applySteps.cpp index 581a700cf75..85959862dba 100644 --- a/src/ripple/app/tx/impl/applySteps.cpp +++ b/src/ripple/app/tx/impl/applySteps.cpp @@ -254,7 +254,7 @@ invoke_preclaim(PreclaimContext const& ctx) } } -static FeeUnit64 +static XRPAmount invoke_calculateBaseFee(ReadView const& view, STTx const& tx) { switch (tx.getTxnType()) @@ -313,7 +313,7 @@ invoke_calculateBaseFee(ReadView const& view, STTx const& tx) return NFTokenAcceptOffer::calculateBaseFee(view, tx); default: assert(false); - return FeeUnit64{0}; + return XRPAmount{0}; } } @@ -535,7 +535,7 @@ preclaim( } } -FeeUnit64 +XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx) { return invoke_calculateBaseFee(view, tx); @@ -544,7 +544,7 @@ calculateBaseFee(ReadView const& view, STTx const& tx) XRPAmount calculateDefaultBaseFee(ReadView const& view, STTx const& tx) { - return view.fees().toDrops(Transactor::calculateBaseFee(view, tx)); + return Transactor::calculateBaseFee(view, tx); } std::pair diff --git a/src/ripple/basics/FeeUnits.h b/src/ripple/basics/FeeUnits.h index 90116eed2a1..c74524c7c71 100644 --- a/src/ripple/basics/FeeUnits.h +++ b/src/ripple/basics/FeeUnits.h @@ -454,11 +454,6 @@ mulDivU(Source1 value, Dest mul, Source2 div) } // namespace feeunit -template -using FeeUnit = feeunit::TaggedFee; -using FeeUnit32 = FeeUnit; -using FeeUnit64 = FeeUnit; - template using FeeLevel = feeunit::TaggedFee; using FeeLevel64 = FeeLevel; diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index 2d440a1afd9..1e91f49263b 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -139,8 +139,9 @@ class Config : public BasicConfig // Network parameters - // The number of fee units a reference transaction costs - static constexpr FeeUnit32 TRANSACTION_FEE_BASE{10}; + // DEPRECATED - Fee units for a reference transction. + // Only provided for backwards compatibility in a couple of places + static constexpr std::uint32_t FEE_UNITS_DEPRECATED = 10; // Note: The following parameters do not relate to the UNL or trust at all // Minimum number of nodes to consider the network present diff --git a/src/ripple/ledger/ReadView.h b/src/ripple/ledger/ReadView.h index 714f8dc945d..fb9e37c7458 100644 --- a/src/ripple/ledger/ReadView.h +++ b/src/ripple/ledger/ReadView.h @@ -49,7 +49,6 @@ namespace ripple { struct Fees { XRPAmount base{0}; // Reference tx cost (drops) - FeeUnit32 units{0}; // Reference fee units XRPAmount reserve{0}; // Reserve base (drops) XRPAmount increment{0}; // Reserve increment (drops) @@ -68,15 +67,6 @@ struct Fees { return reserve + ownerCount * increment; } - - XRPAmount - toDrops(FeeUnit64 const& fee) const - { - if (auto const resultPair = mulDiv(base, fee, units); resultPair.first) - return resultPair.second; - - return XRPAmount(STAmount::cMaxNativeN); - } }; //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index d4e65a31af8..b3e1dba78bd 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -74,7 +74,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 54; +static constexpr std::size_t numFeatures = 55; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -341,6 +341,7 @@ extern uint256 const fixTrustLinesToSelf; extern uint256 const fixRemoveNFTokenAutoTrustLine; extern uint256 const featureImmediateOfferKilled; extern uint256 const featureDisallowIncoming; +extern uint256 const featureXRPFees; } // namespace ripple diff --git a/src/ripple/protocol/SField.h b/src/ripple/protocol/SField.h index 253d956408f..0627f9860d9 100644 --- a/src/ripple/protocol/SField.h +++ b/src/ripple/protocol/SField.h @@ -483,6 +483,11 @@ extern SF_AMOUNT const sfRippleEscrow; extern SF_AMOUNT const sfDeliveredAmount; extern SF_AMOUNT const sfNFTokenBrokerFee; +// currency amount (fees) +extern SF_AMOUNT const sfBaseFeeXRP; +extern SF_AMOUNT const sfReserveBaseXRP; +extern SF_AMOUNT const sfReserveIncrementXRP; + // variable length (common) extern SF_VL const sfPublicKey; extern SF_VL const sfMessageKey; diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index 0620f5f66ca..847a3b4a720 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -46,7 +46,7 @@ constexpr XRPAmount INITIAL_XRP{100'000'000'000 * DROPS_PER_XRP}; inline bool isLegalAmount(XRPAmount const& amount) { - return amount <= INITIAL_XRP; + return amount >= -INITIAL_XRP && amount <= INITIAL_XRP; } /* The currency code for the native currency. */ diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 5903603f975..2e141c11fd1 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -451,6 +451,7 @@ REGISTER_FIX (fixTrustLinesToSelf, Supported::yes, DefaultVote::no) REGISTER_FIX (fixRemoveNFTokenAutoTrustLine, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(ImmediateOfferKilled, Supported::yes, DefaultVote::no); REGISTER_FEATURE(DisallowIncoming, Supported::yes, DefaultVote::no); +REGISTER_FEATURE(XRPFees, Supported::yes, DefaultVote::no); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index 7d5cf9d21aa..da7b17f817c 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -146,10 +146,15 @@ LedgerFormats::LedgerFormats() add(jss::FeeSettings, ltFEE_SETTINGS, { - {sfBaseFee, soeREQUIRED}, - {sfReferenceFeeUnits, soeREQUIRED}, - {sfReserveBase, soeREQUIRED}, - {sfReserveIncrement, soeREQUIRED}, + // Old version uses raw numbers + {sfBaseFee, soeOPTIONAL}, + {sfReferenceFeeUnits, soeOPTIONAL}, + {sfReserveBase, soeOPTIONAL}, + {sfReserveIncrement, soeOPTIONAL}, + // New version uses Amounts + {sfBaseFeeXRP, soeOPTIONAL}, + {sfReserveBaseXRP, soeOPTIONAL}, + {sfReserveIncrementXRP, soeOPTIONAL}, }, commonFields); diff --git a/src/ripple/protocol/impl/SField.cpp b/src/ripple/protocol/impl/SField.cpp index 73098319b28..b495bf7dac4 100644 --- a/src/ripple/protocol/impl/SField.cpp +++ b/src/ripple/protocol/impl/SField.cpp @@ -234,6 +234,11 @@ CONSTRUCT_TYPED_SFIELD(sfRippleEscrow, "RippleEscrow", AMOUNT, CONSTRUCT_TYPED_SFIELD(sfDeliveredAmount, "DeliveredAmount", AMOUNT, 18); CONSTRUCT_TYPED_SFIELD(sfNFTokenBrokerFee, "NFTokenBrokerFee", AMOUNT, 19); +// currency amount (fees) +CONSTRUCT_TYPED_SFIELD(sfBaseFeeXRP, "BaseFeeXRP", AMOUNT, 20); +CONSTRUCT_TYPED_SFIELD(sfReserveBaseXRP, "ReserveBaseXRP", AMOUNT, 21); +CONSTRUCT_TYPED_SFIELD(sfReserveIncrementXRP, "ReserveIncrementXRP", AMOUNT, 22); + // variable length (common) CONSTRUCT_TYPED_SFIELD(sfPublicKey, "PublicKey", VL, 1); CONSTRUCT_TYPED_SFIELD(sfMessageKey, "MessageKey", VL, 2); diff --git a/src/ripple/protocol/impl/STValidation.cpp b/src/ripple/protocol/impl/STValidation.cpp index 8d9b3563c35..d83281a85d4 100644 --- a/src/ripple/protocol/impl/STValidation.cpp +++ b/src/ripple/protocol/impl/STValidation.cpp @@ -58,9 +58,14 @@ STValidation::validationFormat() {sfSigningPubKey, soeREQUIRED}, {sfSignature, soeREQUIRED}, {sfConsensusHash, soeOPTIONAL}, + // featureHardenedValidations {sfCookie, soeDEFAULT}, {sfValidatedHash, soeOPTIONAL}, {sfServerVersion, soeOPTIONAL}, + // featureXRPFees + {sfBaseFeeXRP, soeOPTIONAL}, + {sfReserveBaseXRP, soeOPTIONAL}, + {sfReserveIncrementXRP, soeOPTIONAL}, }; // clang-format on diff --git a/src/ripple/protocol/impl/TxFormats.cpp b/src/ripple/protocol/impl/TxFormats.cpp index ce0d5db921f..8e1b9a9ee46 100644 --- a/src/ripple/protocol/impl/TxFormats.cpp +++ b/src/ripple/protocol/impl/TxFormats.cpp @@ -155,10 +155,15 @@ TxFormats::TxFormats() ttFEE, { {sfLedgerSequence, soeOPTIONAL}, - {sfBaseFee, soeREQUIRED}, - {sfReferenceFeeUnits, soeREQUIRED}, - {sfReserveBase, soeREQUIRED}, - {sfReserveIncrement, soeREQUIRED}, + // Old version uses raw numbers + {sfBaseFee, soeOPTIONAL}, + {sfReferenceFeeUnits, soeOPTIONAL}, + {sfReserveBase, soeOPTIONAL}, + {sfReserveIncrement, soeOPTIONAL}, + // New version uses Amounts + {sfBaseFeeXRP, soeOPTIONAL}, + {sfReserveBaseXRP, soeOPTIONAL}, + {sfReserveIncrementXRP, soeOPTIONAL}, }, commonFields); diff --git a/src/ripple/protocol/jss.h b/src/ripple/protocol/jss.h index 1c5bf8463b0..01e30dd9327 100644 --- a/src/ripple/protocol/jss.h +++ b/src/ripple/protocol/jss.h @@ -151,6 +151,7 @@ JSS(balance); // out: AccountLines JSS(balances); // out: GatewayBalances JSS(base); // out: LogLevel JSS(base_fee); // out: NetworkOPs +JSS(base_fee_drops); // out: NetworkOPs JSS(base_fee_xrp); // out: NetworkOPs JSS(bids); // out: Subscribe JSS(binary); // in: AccountTX, LedgerEntry, @@ -495,8 +496,10 @@ JSS(request); // RPC JSS(requested); // out: Manifest JSS(reservations); // out: Reservations JSS(reserve_base); // out: NetworkOPs +JSS(reserve_base_drops); // out: NetworkOPs JSS(reserve_base_xrp); // out: NetworkOPs JSS(reserve_inc); // out: NetworkOPs +JSS(reserve_inc_drops); // out: NetworkOPs JSS(reserve_inc_xrp); // out: NetworkOPs JSS(response); // websocket JSS(result); // RPC diff --git a/src/ripple/rpc/handlers/NoRippleCheck.cpp b/src/ripple/rpc/handlers/NoRippleCheck.cpp index a2af9845fd7..18156ea4247 100644 --- a/src/ripple/rpc/handlers/NoRippleCheck.cpp +++ b/src/ripple/rpc/handlers/NoRippleCheck.cpp @@ -45,7 +45,7 @@ fillTransaction( // Convert the reference transaction cost in fee units to drops // scaled to represent the current fee load. txArray["Fee"] = - scaleFeeLoad(fees.units, context.app.getFeeTrack(), fees, false) + scaleFeeLoad(fees.base, context.app.getFeeTrack(), fees, false) .jsonClipped(); } diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index ca24b68740e..4cf372e6b63 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -720,8 +720,7 @@ checkFee( } } - // Default fee in fee units. - FeeUnit32 const feeDefault = config.TRANSACTION_FEE_BASE; + XRPAmount const feeDefault = config.FEE_DEFAULT; auto ledger = app.openLedger().current(); // Administrative and identified endpoints are exempt from local fees. @@ -738,11 +737,7 @@ checkFee( auto const limit = [&]() { // Scale fee units to drops: - auto const drops = - mulDiv(feeDefault, ledger->fees().base, ledger->fees().units); - if (!drops.first) - Throw("mulDiv"); - auto const result = mulDiv(drops.second, mult, div); + auto const result = mulDiv(feeDefault, mult, div); if (!result.first) Throw("mulDiv"); return result.second; diff --git a/src/test/app/LoadFeeTrack_test.cpp b/src/test/app/LoadFeeTrack_test.cpp index d34531bd7bf..cc0b1c19529 100644 --- a/src/test/app/LoadFeeTrack_test.cpp +++ b/src/test/app/LoadFeeTrack_test.cpp @@ -36,55 +36,52 @@ class LoadFeeTrack_test : public beast::unit_test::suite Fees const fees = [&]() { Fees f; f.base = d.FEE_DEFAULT; - f.units = d.TRANSACTION_FEE_BASE; f.reserve = 200 * DROPS_PER_XRP; f.increment = 50 * DROPS_PER_XRP; return f; }(); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{0}, l, fees, false) == XRPAmount{0}); + scaleFeeLoad(XRPAmount{0}, l, fees, false) == XRPAmount{0}); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{10000}, l, fees, false) == + scaleFeeLoad(XRPAmount{10000}, l, fees, false) == XRPAmount{10000}); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{1}, l, fees, false) == XRPAmount{1}); + scaleFeeLoad(XRPAmount{1}, l, fees, false) == XRPAmount{1}); } { Fees const fees = [&]() { Fees f; f.base = d.FEE_DEFAULT * 10; - f.units = d.TRANSACTION_FEE_BASE; f.reserve = 200 * DROPS_PER_XRP; f.increment = 50 * DROPS_PER_XRP; return f; }(); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{0}, l, fees, false) == XRPAmount{0}); + scaleFeeLoad(XRPAmount{0}, l, fees, false) == XRPAmount{0}); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{10000}, l, fees, false) == - XRPAmount{100000}); + scaleFeeLoad(XRPAmount{10000}, l, fees, false) == + XRPAmount{10000}); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{1}, l, fees, false) == XRPAmount{10}); + scaleFeeLoad(XRPAmount{1}, l, fees, false) == XRPAmount{1}); } { Fees const fees = [&]() { Fees f; f.base = d.FEE_DEFAULT; - f.units = d.TRANSACTION_FEE_BASE * 10; f.reserve = 200 * DROPS_PER_XRP; f.increment = 50 * DROPS_PER_XRP; return f; }(); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{0}, l, fees, false) == XRPAmount{0}); + scaleFeeLoad(XRPAmount{0}, l, fees, false) == XRPAmount{0}); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{10000}, l, fees, false) == - XRPAmount{1000}); + scaleFeeLoad(XRPAmount{10000}, l, fees, false) == + XRPAmount{10000}); BEAST_EXPECT( - scaleFeeLoad(FeeUnit64{1}, l, fees, false) == XRPAmount{0}); + scaleFeeLoad(XRPAmount{1}, l, fees, false) == XRPAmount{1}); } } }; diff --git a/src/test/app/PseudoTx_test.cpp b/src/test/app/PseudoTx_test.cpp index d76b66f0a99..597fb912006 100644 --- a/src/test/app/PseudoTx_test.cpp +++ b/src/test/app/PseudoTx_test.cpp @@ -16,6 +16,7 @@ //============================================================================== #include +#include #include #include #include @@ -27,17 +28,26 @@ namespace test { struct PseudoTx_test : public beast::unit_test::suite { std::vector - getPseudoTxs(std::uint32_t seq) + getPseudoTxs(Rules const& rules, std::uint32_t seq) { std::vector res; res.emplace_back(STTx(ttFEE, [&](auto& obj) { obj[sfAccount] = AccountID(); obj[sfLedgerSequence] = seq; - obj[sfBaseFee] = 0; - obj[sfReserveBase] = 0; - obj[sfReserveIncrement] = 0; - obj[sfReferenceFeeUnits] = 0; + if (rules.enabled(featureXRPFees)) + { + obj[sfBaseFeeXRP] = XRPAmount{0}; + obj[sfReserveBaseXRP] = XRPAmount{0}; + obj[sfReserveIncrementXRP] = XRPAmount{0}; + } + else + { + obj[sfBaseFee] = 0; + obj[sfReserveBase] = 0; + obj[sfReserveIncrement] = 0; + obj[sfReferenceFeeUnits] = 0; + } })); res.emplace_back(STTx(ttAMENDMENT, [&](auto& obj) { @@ -66,12 +76,13 @@ struct PseudoTx_test : public beast::unit_test::suite } void - testPrevented() + testPrevented(FeatureBitset features) { using namespace jtx; - Env env(*this); + Env env(*this, features); - for (auto const& stx : getPseudoTxs(env.closed()->seq() + 1)) + for (auto const& stx : + getPseudoTxs(env.closed()->rules(), env.closed()->seq() + 1)) { std::string reason; BEAST_EXPECT(isPseudoTx(stx)); @@ -101,7 +112,12 @@ struct PseudoTx_test : public beast::unit_test::suite void run() override { - testPrevented(); + using namespace test::jtx; + FeatureBitset const all{supported_amendments()}; + FeatureBitset const xrpFees{featureXRPFees}; + + testPrevented(all - featureXRPFees); + testPrevented(all); testAllowed(); } }; diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index f3170c9a27b..21bcbd42150 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -219,7 +219,6 @@ class TxQ1_test : public beast::unit_test::suite checkMetrics(__LINE__, env, 0, flagMaxQueue, 0, expectedPerLedger, 256); auto const fees = env.current()->fees(); BEAST_EXPECT(fees.base == XRPAmount{base}); - BEAST_EXPECT(fees.units == FeeUnit64{units}); BEAST_EXPECT(fees.reserve == XRPAmount{reserve}); BEAST_EXPECT(fees.increment == XRPAmount{increment}); diff --git a/src/test/basics/FeeUnits_test.cpp b/src/test/basics/FeeUnits_test.cpp index bcb265c36b3..85527423c58 100644 --- a/src/test/basics/FeeUnits_test.cpp +++ b/src/test/basics/FeeUnits_test.cpp @@ -30,6 +30,8 @@ class feeunits_test : public beast::unit_test::suite void testTypes() { + using FeeLevel32 = FeeLevel; + { XRPAmount x{100}; BEAST_EXPECT(x.drops() == 100); @@ -45,8 +47,8 @@ class feeunits_test : public beast::unit_test::suite BEAST_EXPECT( (std::is_same_v)); - FeeUnit32 f{10}; - FeeUnit32 baseFee{100}; + FeeLevel32 f{10}; + FeeLevel32 baseFee{100}; auto drops = mulDiv(baseFee, x, f).second; @@ -65,8 +67,8 @@ class feeunits_test : public beast::unit_test::suite BEAST_EXPECT( (std::is_same_v)); - FeeUnit64 f{10}; - FeeUnit64 baseFee{100}; + FeeLevel64 f{10}; + FeeLevel64 baseFee{100}; auto drops = mulDiv(baseFee, x, f).second; @@ -102,22 +104,24 @@ class feeunits_test : public beast::unit_test::suite testJson() { // Json value functionality + using FeeLevel32 = FeeLevel; + { - FeeUnit32 x{std::numeric_limits::max()}; + FeeLevel32 x{std::numeric_limits::max()}; auto y = x.jsonClipped(); BEAST_EXPECT(y.type() == Json::uintValue); BEAST_EXPECT(y == Json::Value{x.fee()}); } { - FeeUnit32 x{std::numeric_limits::min()}; + FeeLevel32 x{std::numeric_limits::min()}; auto y = x.jsonClipped(); BEAST_EXPECT(y.type() == Json::uintValue); BEAST_EXPECT(y == Json::Value{x.fee()}); } { - FeeUnit64 x{std::numeric_limits::max()}; + FeeLevel64 x{std::numeric_limits::max()}; auto y = x.jsonClipped(); BEAST_EXPECT(y.type() == Json::uintValue); BEAST_EXPECT( @@ -125,7 +129,7 @@ class feeunits_test : public beast::unit_test::suite } { - FeeUnit64 x{std::numeric_limits::min()}; + FeeLevel64 x{std::numeric_limits::min()}; auto y = x.jsonClipped(); BEAST_EXPECT(y.type() == Json::uintValue); BEAST_EXPECT(y == Json::Value{0}); @@ -167,15 +171,17 @@ class feeunits_test : public beast::unit_test::suite { // Explicitly test every defined function for the TaggedFee class // since some of them are templated, but not used anywhere else. + using FeeLevel32 = FeeLevel; + { - auto make = [&](auto x) -> FeeUnit64 { return x; }; - auto explicitmake = [&](auto x) -> FeeUnit64 { - return FeeUnit64{x}; + auto make = [&](auto x) -> FeeLevel64 { return x; }; + auto explicitmake = [&](auto x) -> FeeLevel64 { + return FeeLevel64{x}; }; - FeeUnit64 defaulted; + FeeLevel64 defaulted; (void)defaulted; - FeeUnit64 test{0}; + FeeLevel64 test{0}; BEAST_EXPECT(test.fee() == 0); test = explicitmake(beast::zero); @@ -187,13 +193,13 @@ class feeunits_test : public beast::unit_test::suite test = explicitmake(100u); BEAST_EXPECT(test.fee() == 100); - FeeUnit64 const targetSame{200u}; - FeeUnit32 const targetOther{300u}; + FeeLevel64 const targetSame{200u}; + FeeLevel32 const targetOther{300u}; test = make(targetSame); BEAST_EXPECT(test.fee() == 200); BEAST_EXPECT(test == targetSame); - BEAST_EXPECT(test < FeeUnit64{1000}); - BEAST_EXPECT(test > FeeUnit64{100}); + BEAST_EXPECT(test < FeeLevel64{1000}); + BEAST_EXPECT(test > FeeLevel64{100}); test = make(targetOther); BEAST_EXPECT(test.fee() == 300); BEAST_EXPECT(test == targetOther); diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index f07f3392245..898376bdaf7 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -64,7 +64,7 @@ class Invariants_test : public beast::unit_test::suite ov, tx, tesSUCCESS, - safe_cast(env.current()->fees().units), + env.current()->fees().base, tapNONE, jlog}; diff --git a/src/test/rpc/Subscribe_test.cpp b/src/test/rpc/Subscribe_test.cpp index 1a0773f26ec..783f5eb7e38 100644 --- a/src/test/rpc/Subscribe_test.cpp +++ b/src/test/rpc/Subscribe_test.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -326,11 +327,11 @@ class Subscribe_test : public beast::unit_test::suite } void - testValidations() + testValidations(FeatureBitset features) { using namespace jtx; - Env env{*this, envconfig(validator, "")}; + Env env{*this, envconfig(validator, ""), features}; auto& cfg = env.app().config(); if (!BEAST_EXPECT(cfg.section(SECTION_VALIDATION_SEED).empty())) return; @@ -410,10 +411,25 @@ class Subscribe_test : public beast::unit_test::suite if (jv.isMember(jss::server_version) != isFlagLedger) return false; - if (jv.isMember(jss::reserve_base) != isFlagLedger) + bool xrpFees = env.closed()->rules().enabled(featureXRPFees); + if ((!xrpFees && + jv.isMember(jss::reserve_base) != isFlagLedger) || + (xrpFees && jv.isMember(jss::reserve_base))) return false; - if (jv.isMember(jss::reserve_inc) != isFlagLedger) + if ((!xrpFees && + jv.isMember(jss::reserve_inc) != isFlagLedger) || + (xrpFees && jv.isMember(jss::reserve_inc))) + return false; + + if ((xrpFees && + jv.isMember(jss::reserve_base_drops) != isFlagLedger) || + (!xrpFees && jv.isMember(jss::reserve_base_drops))) + return false; + + if ((xrpFees && + jv.isMember(jss::reserve_inc_drops) != isFlagLedger) || + (!xrpFees && jv.isMember(jss::reserve_inc_drops))) return false; return true; @@ -1140,11 +1156,16 @@ class Subscribe_test : public beast::unit_test::suite void run() override { + using namespace test::jtx; + FeatureBitset const all{supported_amendments()}; + FeatureBitset const xrpFees{featureXRPFees}; + testServer(); testLedger(); testTransactions(); testManifests(); - testValidations(); + testValidations(all - xrpFees); + testValidations(all); testSubErrors(true); testSubErrors(false); testSubByUrl(); From 7ff770669d7e81e689298181d17867dcd665d068 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 5 Jul 2022 21:08:31 -0400 Subject: [PATCH 2/8] Improve handling of 0 drop reference fee with TxQ * For use with CDBC and sidechain projects that do not want to require fees. * Note that fee escalation logic is still in place, which may cause the open ledger fee to rise if the network is busy. 0 drop transactions will still queue, and fee escalation can be effectively disabled by modifying the configuration on all nodes. --- src/ripple/app/misc/TxQ.h | 2 +- src/ripple/app/misc/impl/TxQ.cpp | 60 ++++++++---- src/test/app/TxQ_test.cpp | 154 ++++++++++++++++++++++++++++++- 3 files changed, 193 insertions(+), 23 deletions(-) diff --git a/src/ripple/app/misc/TxQ.h b/src/ripple/app/misc/TxQ.h index 7e004ec7267..69b6d264825 100644 --- a/src/ripple/app/misc/TxQ.h +++ b/src/ripple/app/misc/TxQ.h @@ -860,7 +860,7 @@ setup_TxQ(Config const&); template XRPAmount -toDrops(FeeLevel const& level, XRPAmount const& baseFee) +toDrops(FeeLevel const& level, XRPAmount baseFee) { if (auto const drops = mulDiv(level, baseFee, TxQ::baseLevel); drops.first) return drops.second; diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index c7267565dbd..8424b1d29af 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -41,11 +41,15 @@ getFeeLevelPaid(ReadView const& view, STTx const& tx) XRPAmount baseFee = calculateBaseFee(view, tx); XRPAmount feePaid = tx[sfFee].xrp(); - // If baseFee is 0 then the cost of a basic transaction is free. - XRPAmount const ref = baseFee.signum() > 0 - ? XRPAmount{0} - : calculateDefaultBaseFee(view, tx); - return std::pair{baseFee + ref, feePaid + ref}; + // If baseFee is 0 then the cost of a basic transaction is free, but we + // need the effective fee level to be non-zero. + XRPAmount const mod = [&view, &tx, baseFee]() { + if (baseFee.signum() > 0) + return XRPAmount{0}; + auto def = calculateDefaultBaseFee(view, tx); + return def.signum() == 0 ? XRPAmount{1} : def; + }(); + return std::pair{baseFee + mod, feePaid + mod}; }(); assert(baseFee.signum() > 0); @@ -1072,19 +1076,27 @@ TxQ::apply( LastLedgerSeq and MaybeTx::retriesRemaining. */ auto const balance = (*sleAccount)[sfBalance].xrp(); - /* Get the minimum possible reserve. If fees exceed + /* Get the minimum possible account reserve. If it + is at least 10 * the base fee, and fees exceed this amount, the transaction can't be queued. - Considering that typical fees are several orders + + Currently typical fees are several orders of magnitude smaller than any current or expected - future reserve, this calculation is simpler than + future reserve. This calculation is simpler than trying to figure out the potential changes to the ownerCount that may occur to the account - as a result of these transactions, and removes + as a result of these transactions, and removes any need to account for other transactions that may affect the owner count while these are queued. + + However, in case the account reserve is on a + comparable scale to the base fee, ignore the + reserve. Only check the account balance. */ auto const reserve = view.fees().accountReserve(0); - if (totalFee >= balance || totalFee >= reserve) + auto const base = view.fees().base; + if (totalFee >= balance || + (reserve > 10 * base && totalFee >= reserve)) { // Drop the current transaction JLOG(j_.trace()) << "Ignoring transaction " << transactionID @@ -1104,7 +1116,10 @@ TxQ::apply( // inserted in the middle from fouling up later transactions. auto const potentialTotalSpend = totalFee + std::min(balance - std::min(balance, reserve), potentialSpend); - assert(potentialTotalSpend > XRPAmount{0}); + assert( + potentialTotalSpend > XRPAmount{0} || + (potentialTotalSpend == XRPAmount{0} && + multiTxn->applyView.fees().base == 0)); sleBump->setFieldAmount(sfBalance, balance - potentialTotalSpend); // The transaction's sequence/ticket will be valid when the other // transactions in the queue have been processed. If the tx has a @@ -1834,15 +1849,26 @@ TxQ::doRPC(Application& app) const levels[jss::open_ledger_level] = to_string(metrics.openLedgerFeeLevel); auto const baseFee = view->fees().base; + // If the base fee is 0 drops, but escalation has kicked in, treat the + // base fee as if it is 1 drop, which makes the rest of the math + // work. + auto const effectiveBaseFee = [&baseFee, &metrics]() { + if (!baseFee && metrics.openLedgerFeeLevel != metrics.referenceFeeLevel) + return XRPAmount{1}; + return baseFee; + }(); auto& drops = ret[jss::drops] = Json::Value(); - drops[jss::base_fee] = - to_string(toDrops(metrics.referenceFeeLevel, baseFee)); - drops[jss::minimum_fee] = - to_string(toDrops(metrics.minProcessingFeeLevel, baseFee)); + drops[jss::base_fee] = to_string(baseFee); drops[jss::median_fee] = to_string(toDrops(metrics.medFeeLevel, baseFee)); - drops[jss::open_ledger_fee] = to_string( - toDrops(metrics.openLedgerFeeLevel - FeeLevel64{1}, baseFee) + 1); + drops[jss::minimum_fee] = to_string(toDrops( + metrics.minProcessingFeeLevel, + metrics.txCount >= metrics.txQMaxSize ? effectiveBaseFee : baseFee)); + auto openFee = toDrops(metrics.openLedgerFeeLevel, effectiveBaseFee); + if (effectiveBaseFee && + toFeeLevel(openFee, effectiveBaseFee) < metrics.openLedgerFeeLevel) + openFee += 1; + drops[jss::open_ledger_fee] = to_string(openFee); return ret; } diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 21bcbd42150..e25c9f60de1 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -144,9 +144,15 @@ class TxQ1_test : public beast::unit_test::suite auto const& view = *env.current(); auto metrics = env.app().getTxQ().getMetrics(view); + auto const base = [&view]() { + auto base = view.fees().base; + if (!base) + base += 1; + return base; + }(); // Don't care about the overflow flag - return fee(toDrops(metrics.openLedgerFeeLevel, view.fees().base) + 1); + return fee(toDrops(metrics.openLedgerFeeLevel, base) + 1); } static std::unique_ptr @@ -189,7 +195,6 @@ class TxQ1_test : public beast::unit_test::suite std::size_t expectedPerLedger, std::size_t ledgersInQueue, std::uint32_t base, - std::uint32_t units, std::uint32_t reserve, std::uint32_t increment) { @@ -1094,7 +1099,7 @@ class TxQ1_test : public beast::unit_test::suite checkMetrics(__LINE__, env, 0, std::nullopt, 0, 3, 256); // ledgers in queue is 2 because of makeConfig - auto const initQueueMax = initFee(env, 3, 2, 10, 10, 200, 50); + auto const initQueueMax = initFee(env, 3, 2, 10, 200, 50); // Create several accounts while the fee is cheap so they all apply. env.fund(drops(2000), noripple(alice)); @@ -1741,7 +1746,7 @@ class TxQ1_test : public beast::unit_test::suite auto queued = ter(terQUEUED); // ledgers in queue is 2 because of makeConfig - auto const initQueueMax = initFee(env, 3, 2, 10, 10, 200, 50); + auto const initQueueMax = initFee(env, 3, 2, 10, 200, 50); BEAST_EXPECT(env.current()->fees().base == 10); @@ -2136,7 +2141,7 @@ class TxQ1_test : public beast::unit_test::suite // queued before the open ledger fee approached the reserve, // which would unnecessarily slow down this test. // ledgers in queue is 2 because of makeConfig - auto const initQueueMax = initFee(env, 3, 2, 10, 10, 200, 50); + auto const initQueueMax = initFee(env, 3, 2, 10, 200, 50); auto limit = 3; @@ -4784,6 +4789,144 @@ class TxQ1_test : public beast::unit_test::suite } } + void + testZeroReferenceFee() + { + testcase("Zero reference fee"); + using namespace jtx; + + Account const alice("alice"); + auto const queued = ter(terQUEUED); + + Env env( + *this, + makeConfig( + {{"minimum_txn_in_ledger_standalone", "3"}}, + {{"reference_fee", "0"}, + {"account_reserve", "0"}, + {"owner_reserve", "0"}})); + + BEAST_EXPECT(env.current()->fees().base == 10); + + checkMetrics(__LINE__, env, 0, std::nullopt, 0, 3, 256); + + // ledgers in queue is 2 because of makeConfig + auto const initQueueMax = initFee(env, 3, 2, 0, 0, 0); + + BEAST_EXPECT(env.current()->fees().base == 0); + + { + auto const fee = env.rpc("fee"); + + if (BEAST_EXPECT(fee.isMember(jss::result)) && + BEAST_EXPECT(!RPC::contains_error(fee[jss::result]))) + { + auto const& result = fee[jss::result]; + + BEAST_EXPECT(result.isMember(jss::levels)); + auto const& levels = result[jss::levels]; + BEAST_EXPECT( + levels.isMember(jss::median_level) && + levels[jss::median_level] == "128000"); + BEAST_EXPECT( + levels.isMember(jss::minimum_level) && + levels[jss::minimum_level] == "256"); + BEAST_EXPECT( + levels.isMember(jss::open_ledger_level) && + levels[jss::open_ledger_level] == "256"); + BEAST_EXPECT( + levels.isMember(jss::reference_level) && + levels[jss::reference_level] == "256"); + + auto const& drops = result[jss::drops]; + BEAST_EXPECT( + drops.isMember(jss::base_fee) && + drops[jss::base_fee] == "0"); + BEAST_EXPECT( + drops.isMember(jss::median_fee) && + drops[jss::base_fee] == "0"); + BEAST_EXPECT( + drops.isMember(jss::minimum_fee) && + drops[jss::base_fee] == "0"); + BEAST_EXPECT( + drops.isMember(jss::open_ledger_fee) && + drops[jss::base_fee] == "0"); + } + } + + checkMetrics(__LINE__, env, 0, initQueueMax, 0, 3, 256); + + // The noripple is to reduce the number of transactions required to + // fund the accounts. There is no rippling in this test. + env.fund(XRP(100000), noripple(alice)); + + checkMetrics(__LINE__, env, 0, initQueueMax, 1, 3, 256); + + env.close(); + + checkMetrics(__LINE__, env, 0, 6, 0, 3, 256); + + fillQueue(env, alice); + + checkMetrics(__LINE__, env, 0, 6, 4, 3, 256); + + env(noop(alice), openLedgerFee(env)); + + checkMetrics(__LINE__, env, 0, 6, 5, 3, 256); + + auto aliceSeq = env.seq(alice); + env(noop(alice), queued); + + checkMetrics(__LINE__, env, 1, 6, 5, 3, 256); + + env(noop(alice), seq(aliceSeq + 1), fee(10), queued); + + checkMetrics(__LINE__, env, 2, 6, 5, 3, 256); + + { + auto const fee = env.rpc("fee"); + + if (BEAST_EXPECT(fee.isMember(jss::result)) && + BEAST_EXPECT(!RPC::contains_error(fee[jss::result]))) + { + auto const& result = fee[jss::result]; + + BEAST_EXPECT(result.isMember(jss::levels)); + auto const& levels = result[jss::levels]; + BEAST_EXPECT( + levels.isMember(jss::median_level) && + levels[jss::median_level] == "128000"); + BEAST_EXPECT( + levels.isMember(jss::minimum_level) && + levels[jss::minimum_level] == "256"); + BEAST_EXPECT( + levels.isMember(jss::open_ledger_level) && + levels[jss::open_ledger_level] == "355555"); + BEAST_EXPECT( + levels.isMember(jss::reference_level) && + levels[jss::reference_level] == "256"); + + auto const& drops = result[jss::drops]; + BEAST_EXPECT( + drops.isMember(jss::base_fee) && + drops[jss::base_fee] == "0"); + BEAST_EXPECT( + drops.isMember(jss::median_fee) && + drops[jss::median_fee] == "0"); + BEAST_EXPECT( + drops.isMember(jss::minimum_fee) && + drops[jss::minimum_fee] == "0"); + BEAST_EXPECT( + drops.isMember(jss::open_ledger_fee) && + drops[jss::open_ledger_fee] == "1389"); + } + } + + env.close(); + + checkMetrics(__LINE__, env, 0, 10, 2, 5, 256); + } + void run() override { @@ -4824,6 +4967,7 @@ class TxQ1_test : public beast::unit_test::suite testReexecutePreflight(); testQueueFullDropPenalty(); testCancelQueuedOffers(); + testZeroReferenceFee(); } }; From 8d15137bb48de90bf775a71339a6b54dc64d0d8c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 31 Aug 2022 16:29:53 -0700 Subject: [PATCH 3/8] [FOLD] Review feedback from Nik: * Break featureXRPCheck logic into multiple blocks. * Change default network reserves to match mainnet. * Add a isLegalAmountSigned check --- src/ripple/app/misc/FeeVoteImpl.cpp | 286 ++++++++++++++----------- src/ripple/protocol/SystemParameters.h | 8 + src/test/app/AccountDelete_test.cpp | 5 +- src/test/app/FeeVote_test.cpp | 21 +- src/test/rpc/AccountTx_test.cpp | 5 +- 5 files changed, 183 insertions(+), 142 deletions(-) diff --git a/src/ripple/app/misc/FeeVoteImpl.cpp b/src/ripple/app/misc/FeeVoteImpl.cpp index 928d0b40b12..0d289d151a5 100644 --- a/src/ripple/app/misc/FeeVoteImpl.cpp +++ b/src/ripple/app/misc/FeeVoteImpl.cpp @@ -36,7 +36,6 @@ class VotableValue value_type const current_; // The current setting value_type const target_; // The setting we want std::map voteMap_; - std::optional vote_; public: VotableValue(value_type current, value_type target) @@ -58,38 +57,19 @@ class VotableValue addVote(current_); } - void - setVotes(); - value_type - getVotes() const; - - template - Dest - getVotesAs() const; - - bool - voteChange() const + current() const { - return getVotes() != current_; + return current_; } -}; -void -VotableValue::setVotes() -{ - // Need to explicitly remove any value from vote_, because it will be - // returned from getVotes() if set. - vote_.reset(); - vote_ = getVotes(); -} + std::pair + getVotes() const; +}; auto -VotableValue::getVotes() const -> value_type +VotableValue::getVotes() const -> std::pair { - if (vote_) - return *vote_; - value_type ourVote = current_; int weight = 0; for (auto const& [key, val] : voteMap_) @@ -103,14 +83,7 @@ VotableValue::getVotes() const -> value_type } } - return ourVote; -} - -template -Dest -VotableValue::getVotesAs() const -{ - return getVotes().dropsAs(current_); + return {ourVote, ourVote != current_}; } } // namespace detail @@ -153,38 +126,70 @@ FeeVoteImpl::doValidation( // Values should always be in a valid range (because the voting process // will ignore out-of-range values) but if we detect such a case, we do // not send a value. - if (lastFees.base != target_.reference_fee) - { - JLOG(journal_.info()) - << "Voting for base fee of " << target_.reference_fee; - - if (rules.enabled(featureXRPFees)) - v[sfBaseFeeXRP] = target_.reference_fee; - else if (auto const f = target_.reference_fee.dropsAs()) - v[sfBaseFee] = *f; - } - - if (lastFees.accountReserve(0) != target_.account_reserve) + if (rules.enabled(featureXRPFees)) { - JLOG(journal_.info()) - << "Voting for base reserve of " << target_.account_reserve; - - if (rules.enabled(featureXRPFees)) - v[sfReserveBaseXRP] = target_.account_reserve; - else if ( - auto const f = target_.account_reserve.dropsAs()) - v[sfReserveBase] = *f; + auto vote = [&v, this]( + auto const current, + XRPAmount target, + const char* name, + auto const& sfield) { + if (current != target) + { + JLOG(journal_.info()) + << "Voting for " << name << " of " << target; + + v[sfield] = target; + } + }; + vote(lastFees.base, target_.reference_fee, "base fee", sfBaseFeeXRP); + vote( + lastFees.accountReserve(0), + target_.account_reserve, + "base reserve", + sfReserveBaseXRP); + vote( + lastFees.increment, + target_.owner_reserve, + "reserve increment", + sfReserveIncrementXRP); } - - if (lastFees.increment != target_.owner_reserve) + else { - JLOG(journal_.info()) - << "Voting for reserve increment of " << target_.owner_reserve; - - if (rules.enabled(featureXRPFees)) - v[sfReserveIncrementXRP] = target_.owner_reserve; - else if (auto const f = target_.owner_reserve.dropsAs()) - v[sfReserveIncrement] = *f; + auto to32 = [](XRPAmount target) { + return target.dropsAs(); + }; + auto to64 = [](XRPAmount target) { + return target.dropsAs(); + }; + auto vote = [&v, this]( + auto const current, + XRPAmount target, + auto convertCallback, + const char* name, + auto const& sfield) { + if (current != target) + { + JLOG(journal_.info()) + << "Voting for " << name << " of " << target; + + if (auto const f = convertCallback(target)) + v[sfield] = *f; + } + }; + + vote(lastFees.base, target_.reference_fee, to64, "base fee", sfBaseFee); + vote( + lastFees.accountReserve(0), + target_.account_reserve, + to32, + "base reserve", + sfReserveBase); + vote( + lastFees.increment, + target_.owner_reserve, + to32, + "reserve increment", + sfReserveIncrement); } } @@ -207,89 +212,110 @@ FeeVoteImpl::doVoting( lastClosedLedger->fees().increment, target_.owner_reserve); auto const& rules = lastClosedLedger->rules(); - auto doVote = [&rules]( - std::shared_ptr const& val, - detail::VotableValue& value, - SF_AMOUNT const& xrpField, - auto const& valueField) { - if (auto const field = ~val->at(~xrpField); - field && rules.enabled(featureXRPFees) && field->native()) - { - auto const vote = field->xrp(); - if (isLegalAmount(vote)) - value.addVote(vote); + if (rules.enabled(featureXRPFees)) + { + auto doVote = [](std::shared_ptr const& val, + detail::VotableValue& value, + SF_AMOUNT const& xrpField) { + if (auto const field = ~val->at(~xrpField); + field && field->native()) + { + auto const vote = field->xrp(); + if (isLegalAmountSigned(vote)) + value.addVote(vote); + else + value.noVote(); + } else + { value.noVote(); - } - else if (auto const field = val->at(~valueField)) + } + }; + + for (auto const& val : set) { - using xrptype = XRPAmount::value_type; - auto const vote = *field; - if (vote <= std::numeric_limits::max() && - isLegalAmount(XRPAmount{unsafe_cast(vote)})) - value.addVote( - XRPAmount{unsafe_cast(vote)}); + if (!val->isTrusted()) + continue; + doVote(val, baseFeeVote, sfBaseFeeXRP); + doVote(val, baseReserveVote, sfReserveBaseXRP); + doVote(val, incReserveVote, sfReserveIncrementXRP); + } + } + else + { + auto doVote = [](std::shared_ptr const& val, + detail::VotableValue& value, + auto const& valueField) { + if (auto const field = val->at(~valueField)) + { + using xrptype = XRPAmount::value_type; + auto const vote = *field; + if (vote <= std::numeric_limits::max() && + isLegalAmountSigned(XRPAmount{unsafe_cast(vote)})) + value.addVote( + XRPAmount{unsafe_cast(vote)}); + else + // Invalid amounts will be treated as if they're + // not provided. Don't throw because this value is + // provided by an external entity. + value.noVote(); + } else - // Invalid amounts will be treated as if they're - // not provided. Don't throw because this value is - // provided by an external entity. + { value.noVote(); - } - else + } + }; + + for (auto const& val : set) { - value.noVote(); + if (!val->isTrusted()) + continue; + doVote(val, baseFeeVote, sfBaseFee); + doVote(val, baseReserveVote, sfReserveBase); + doVote(val, incReserveVote, sfReserveIncrement); } - }; - - for (auto const& val : set) - { - if (!val->isTrusted()) - continue; - doVote(val, baseFeeVote, sfBaseFeeXRP, sfBaseFee); - doVote(val, baseReserveVote, sfReserveBaseXRP, sfReserveBase); - doVote(val, incReserveVote, sfReserveIncrementXRP, sfReserveIncrement); } // choose our positions - baseFeeVote.setVotes(); - baseReserveVote.setVotes(); - incReserveVote.setVotes(); + // TODO: Use structured binding once LLVM issue + // https://github.com/llvm/llvm-project/issues/48582 + // is fixed. + auto const baseFee = baseFeeVote.getVotes(); + auto const baseReserve = baseReserveVote.getVotes(); + auto const incReserve = incReserveVote.getVotes(); auto const seq = lastClosedLedger->info().seq + 1; // add transactions to our position - if ((baseFeeVote.voteChange()) || (baseReserveVote.voteChange()) || - (incReserveVote.voteChange())) + if (baseFee.second || baseReserve.second || incReserve.second) { JLOG(journal_.warn()) - << "We are voting for a fee change: " << baseFeeVote.getVotes() - << "/" << baseReserveVote.getVotes() << "/" - << incReserveVote.getVotes(); - - STTx feeTx( - ttFEE, - [&rules, seq, &baseFeeVote, &baseReserveVote, &incReserveVote]( - auto& obj) { - obj[sfAccount] = AccountID(); - obj[sfLedgerSequence] = seq; - if (rules.enabled(featureXRPFees)) - { - obj[sfBaseFeeXRP] = baseFeeVote.getVotes(); - obj[sfReserveBaseXRP] = baseReserveVote.getVotes(); - obj[sfReserveIncrementXRP] = incReserveVote.getVotes(); - } - else - { - // Without the featureXRPFees amendment, these fields are - // required. - obj[sfBaseFee] = baseFeeVote.getVotesAs(); - obj[sfReserveBase] = - baseReserveVote.getVotesAs(); - obj[sfReserveIncrement] = - incReserveVote.getVotesAs(); - obj[sfReferenceFeeUnits] = Config::FEE_UNITS_DEPRECATED; - } - }); + << "We are voting for a fee change: " << baseFee.first << "/" + << baseReserve.first << "/" << incReserve.first; + + STTx feeTx(ttFEE, [=, &rules](auto& obj) { + obj[sfAccount] = AccountID(); + obj[sfLedgerSequence] = seq; + if (rules.enabled(featureXRPFees)) + { + obj[sfBaseFeeDrops] = baseFee.first; + obj[sfReserveBaseDrops] = baseReserve.first; + obj[sfReserveIncrementDrops] = incReserve.first; + } + else + { + // Without the featureXRPFees amendment, these fields are + // required. + obj[sfBaseFee] = + baseFee.first.dropsAs(baseFeeVote.current()); + obj[sfReserveBase] = baseReserve.first.dropsAs( + baseReserveVote.current()); + obj[sfReserveIncrement] = + incReserve.first.dropsAs( + incReserveVote.current()); + obj[sfReferenceFeeUnits] = Config::FEE_UNITS_DEPRECATED; + } + }); uint256 txID = feeTx.getTransactionID(); diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index 847a3b4a720..db0c15dcad7 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -45,6 +45,14 @@ constexpr XRPAmount INITIAL_XRP{100'000'000'000 * DROPS_PER_XRP}; /** Returns true if the amount does not exceed the initial XRP in existence. */ inline bool isLegalAmount(XRPAmount const& amount) +{ + return amount <= INITIAL_XRP; +} + +/** Returns true if the absolute value of the amount does not exceed the initial + * XRP in existence. */ +inline bool +isLegalAmountSigned(XRPAmount const& amount) { return amount >= -INITIAL_XRP && amount <= INITIAL_XRP; } diff --git a/src/test/app/AccountDelete_test.cpp b/src/test/app/AccountDelete_test.cpp index 73a0ccbf9e0..2ec0b876a64 100644 --- a/src/test/app/AccountDelete_test.cpp +++ b/src/test/app/AccountDelete_test.cpp @@ -515,7 +515,10 @@ class AccountDelete_test : public beast::unit_test::suite // All it takes is a large enough XRP payment to resurrect // becky's account. Try too small a payment. - env(pay(alice, becky, XRP(9)), ter(tecNO_DST_INSUF_XRP)); + env(pay(alice, + becky, + drops(env.current()->fees().accountReserve(0)) - XRP(1)), + ter(tecNO_DST_INSUF_XRP)); env.close(); // Actually resurrect becky's account. diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 4c2acf6297d..90dd8fa3dfc 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -29,13 +29,14 @@ class FeeVote_test : public beast::unit_test::suite void testSetup() { + FeeVote::Setup const defaultSetup; { // defaults Section config; auto setup = setup_FeeVote(config); - BEAST_EXPECT(setup.reference_fee == 10); - BEAST_EXPECT(setup.account_reserve == 10 * DROPS_PER_XRP); - BEAST_EXPECT(setup.owner_reserve == 2 * DROPS_PER_XRP); + BEAST_EXPECT(setup.reference_fee == defaultSetup.reference_fee); + BEAST_EXPECT(setup.account_reserve == defaultSetup.account_reserve); + BEAST_EXPECT(setup.owner_reserve == defaultSetup.owner_reserve); } { Section config; @@ -56,9 +57,9 @@ class FeeVote_test : public beast::unit_test::suite "owner_reserve = foo"}); // Illegal values are ignored, and the defaults left unchanged auto setup = setup_FeeVote(config); - BEAST_EXPECT(setup.reference_fee == 10); - BEAST_EXPECT(setup.account_reserve == 10 * DROPS_PER_XRP); - BEAST_EXPECT(setup.owner_reserve == 2 * DROPS_PER_XRP); + BEAST_EXPECT(setup.reference_fee == defaultSetup.reference_fee); + BEAST_EXPECT(setup.account_reserve == defaultSetup.account_reserve); + BEAST_EXPECT(setup.owner_reserve == defaultSetup.owner_reserve); } { Section config; @@ -68,7 +69,7 @@ class FeeVote_test : public beast::unit_test::suite "owner_reserve = -1234"}); // Illegal values are ignored, and the defaults left unchanged auto setup = setup_FeeVote(config); - BEAST_EXPECT(setup.reference_fee == 10); + BEAST_EXPECT(setup.reference_fee == defaultSetup.reference_fee); BEAST_EXPECT( setup.account_reserve == static_cast(-1234567)); BEAST_EXPECT( @@ -86,9 +87,9 @@ class FeeVote_test : public beast::unit_test::suite "owner_reserve = " + big64}); // Illegal values are ignored, and the defaults left unchanged auto setup = setup_FeeVote(config); - BEAST_EXPECT(setup.reference_fee == 10); - BEAST_EXPECT(setup.account_reserve == 10 * DROPS_PER_XRP); - BEAST_EXPECT(setup.owner_reserve == 2 * DROPS_PER_XRP); + BEAST_EXPECT(setup.reference_fee == defaultSetup.reference_fee); + BEAST_EXPECT(setup.account_reserve == defaultSetup.account_reserve); + BEAST_EXPECT(setup.owner_reserve == defaultSetup.owner_reserve); } } diff --git a/src/test/rpc/AccountTx_test.cpp b/src/test/rpc/AccountTx_test.cpp index 1d537d47791..75147875d1a 100644 --- a/src/test/rpc/AccountTx_test.cpp +++ b/src/test/rpc/AccountTx_test.cpp @@ -547,7 +547,10 @@ class AccountTx_test : public beast::unit_test::suite // All it takes is a large enough XRP payment to resurrect // becky's account. Try too small a payment. - env(pay(alice, becky, XRP(9)), ter(tecNO_DST_INSUF_XRP)); + env(pay(alice, + becky, + drops(env.current()->fees().accountReserve(0)) - XRP(1)), + ter(tecNO_DST_INSUF_XRP)); env.close(); // Actually resurrect becky's account. From ebe3a1cc455f825753f10b0a32c43c7bbba26596 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 7 Sep 2022 16:03:38 -0700 Subject: [PATCH 4/8] [FOLD, maybe] Rename the new SFields from *XRP to *Drops --- src/ripple/app/ledger/Ledger.cpp | 6 +++--- src/ripple/app/misc/FeeVoteImpl.cpp | 12 ++++++------ src/ripple/app/misc/NetworkOPs.cpp | 6 +++--- src/ripple/app/tx/impl/Change.cpp | 18 +++++++++--------- src/ripple/protocol/SField.h | 6 +++--- src/ripple/protocol/impl/LedgerFormats.cpp | 6 +++--- src/ripple/protocol/impl/SField.cpp | 6 +++--- src/ripple/protocol/impl/STValidation.cpp | 6 +++--- src/ripple/protocol/impl/TxFormats.cpp | 6 +++--- src/test/app/PseudoTx_test.cpp | 6 +++--- 10 files changed, 39 insertions(+), 39 deletions(-) diff --git a/src/ripple/app/ledger/Ledger.cpp b/src/ripple/app/ledger/Ledger.cpp index f82a26c8275..7757dac53bf 100644 --- a/src/ripple/app/ledger/Ledger.cpp +++ b/src/ripple/app/ledger/Ledger.cpp @@ -627,10 +627,10 @@ Ledger::setup(Config const& config) oldFees = baseFee || reserveBase || reserveIncrement; } { - auto const baseFeeXRP = sle->at(~sfBaseFeeXRP); - auto const reserveBaseXRP = sle->at(~sfReserveBaseXRP); + auto const baseFeeXRP = sle->at(~sfBaseFeeDrops); + auto const reserveBaseXRP = sle->at(~sfReserveBaseDrops); auto const reserveIncrementXRP = - sle->at(~sfReserveIncrementXRP); + sle->at(~sfReserveIncrementDrops); auto assign = [&ret]( XRPAmount& dest, std::optional const& src) { diff --git a/src/ripple/app/misc/FeeVoteImpl.cpp b/src/ripple/app/misc/FeeVoteImpl.cpp index 0d289d151a5..570d16ba8da 100644 --- a/src/ripple/app/misc/FeeVoteImpl.cpp +++ b/src/ripple/app/misc/FeeVoteImpl.cpp @@ -141,17 +141,17 @@ FeeVoteImpl::doValidation( v[sfield] = target; } }; - vote(lastFees.base, target_.reference_fee, "base fee", sfBaseFeeXRP); + vote(lastFees.base, target_.reference_fee, "base fee", sfBaseFeeDrops); vote( lastFees.accountReserve(0), target_.account_reserve, "base reserve", - sfReserveBaseXRP); + sfReserveBaseDrops); vote( lastFees.increment, target_.owner_reserve, "reserve increment", - sfReserveIncrementXRP); + sfReserveIncrementDrops); } else { @@ -236,9 +236,9 @@ FeeVoteImpl::doVoting( { if (!val->isTrusted()) continue; - doVote(val, baseFeeVote, sfBaseFeeXRP); - doVote(val, baseReserveVote, sfReserveBaseXRP); - doVote(val, incReserveVote, sfReserveIncrementXRP); + doVote(val, baseFeeVote, sfBaseFeeDrops); + doVote(val, baseReserveVote, sfReserveBaseDrops); + doVote(val, incReserveVote, sfReserveIncrementDrops); } } else diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index aac72c46166..610b8e71adc 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2182,16 +2182,16 @@ NetworkOPsImp::pubValidation(std::shared_ptr const& val) // (The ~ operator converts the Proxy to a std::optional, which // simplifies later operations) - if (auto const baseFeeXRP = ~val->at(~sfBaseFeeXRP); + if (auto const baseFeeXRP = ~val->at(~sfBaseFeeDrops); baseFeeXRP && baseFeeXRP->native()) jvObj[jss::base_fee_drops] = baseFeeXRP->xrp().jsonClipped(); - if (auto const reserveBaseXRP = ~val->at(~sfReserveBaseXRP); + if (auto const reserveBaseXRP = ~val->at(~sfReserveBaseDrops); reserveBaseXRP && reserveBaseXRP->native()) jvObj[jss::reserve_base_drops] = reserveBaseXRP->xrp().jsonClipped(); - if (auto const reserveIncXRP = ~val->at(~sfReserveIncrementXRP); + if (auto const reserveIncXRP = ~val->at(~sfReserveIncrementDrops); reserveIncXRP && reserveIncXRP->native()) jvObj[jss::reserve_inc_drops] = reserveIncXRP->xrp().jsonClipped(); diff --git a/src/ripple/app/tx/impl/Change.cpp b/src/ripple/app/tx/impl/Change.cpp index 5a91e172b26..ec5ff2ab2f0 100644 --- a/src/ripple/app/tx/impl/Change.cpp +++ b/src/ripple/app/tx/impl/Change.cpp @@ -93,9 +93,9 @@ Change::preclaim(PreclaimContext const& ctx) case ttFEE: if (ctx.view.rules().enabled(featureXRPFees)) { - if (!ctx.tx.isFieldPresent(sfBaseFeeXRP) || - !ctx.tx.isFieldPresent(sfReserveBaseXRP) || - !ctx.tx.isFieldPresent(sfReserveIncrementXRP)) + if (!ctx.tx.isFieldPresent(sfBaseFeeDrops) || + !ctx.tx.isFieldPresent(sfReserveBaseDrops) || + !ctx.tx.isFieldPresent(sfReserveIncrementDrops)) return temMALFORMED; // The transaction should only have one set of fields or the // other. @@ -119,9 +119,9 @@ Change::preclaim(PreclaimContext const& ctx) return temMALFORMED; // The transaction should only have one or the other. If the new // fields are present without the amendment, that's bad, too. - if (ctx.tx.isFieldPresent(sfBaseFeeXRP) || - ctx.tx.isFieldPresent(sfReserveBaseXRP) || - ctx.tx.isFieldPresent(sfReserveIncrementXRP)) + if (ctx.tx.isFieldPresent(sfBaseFeeDrops) || + ctx.tx.isFieldPresent(sfReserveBaseDrops) || + ctx.tx.isFieldPresent(sfReserveIncrementDrops)) return temDISABLED; } return tesSUCCESS; @@ -354,9 +354,9 @@ Change::applyFee() }; if (view().rules().enabled(featureXRPFees)) { - set(feeObject, ctx_.tx, sfBaseFeeXRP); - set(feeObject, ctx_.tx, sfReserveBaseXRP); - set(feeObject, ctx_.tx, sfReserveIncrementXRP); + set(feeObject, ctx_.tx, sfBaseFeeDrops); + set(feeObject, ctx_.tx, sfReserveBaseDrops); + set(feeObject, ctx_.tx, sfReserveIncrementDrops); // Ensure the old fields are removed feeObject->makeFieldAbsent(sfBaseFee); feeObject->makeFieldAbsent(sfReferenceFeeUnits); diff --git a/src/ripple/protocol/SField.h b/src/ripple/protocol/SField.h index 0627f9860d9..694eeef5cbb 100644 --- a/src/ripple/protocol/SField.h +++ b/src/ripple/protocol/SField.h @@ -484,9 +484,9 @@ extern SF_AMOUNT const sfDeliveredAmount; extern SF_AMOUNT const sfNFTokenBrokerFee; // currency amount (fees) -extern SF_AMOUNT const sfBaseFeeXRP; -extern SF_AMOUNT const sfReserveBaseXRP; -extern SF_AMOUNT const sfReserveIncrementXRP; +extern SF_AMOUNT const sfBaseFeeDrops; +extern SF_AMOUNT const sfReserveBaseDrops; +extern SF_AMOUNT const sfReserveIncrementDrops; // variable length (common) extern SF_VL const sfPublicKey; diff --git a/src/ripple/protocol/impl/LedgerFormats.cpp b/src/ripple/protocol/impl/LedgerFormats.cpp index da7b17f817c..a540a5d80c0 100644 --- a/src/ripple/protocol/impl/LedgerFormats.cpp +++ b/src/ripple/protocol/impl/LedgerFormats.cpp @@ -152,9 +152,9 @@ LedgerFormats::LedgerFormats() {sfReserveBase, soeOPTIONAL}, {sfReserveIncrement, soeOPTIONAL}, // New version uses Amounts - {sfBaseFeeXRP, soeOPTIONAL}, - {sfReserveBaseXRP, soeOPTIONAL}, - {sfReserveIncrementXRP, soeOPTIONAL}, + {sfBaseFeeDrops, soeOPTIONAL}, + {sfReserveBaseDrops, soeOPTIONAL}, + {sfReserveIncrementDrops, soeOPTIONAL}, }, commonFields); diff --git a/src/ripple/protocol/impl/SField.cpp b/src/ripple/protocol/impl/SField.cpp index b495bf7dac4..b954d64c214 100644 --- a/src/ripple/protocol/impl/SField.cpp +++ b/src/ripple/protocol/impl/SField.cpp @@ -235,9 +235,9 @@ CONSTRUCT_TYPED_SFIELD(sfDeliveredAmount, "DeliveredAmount", AMOUNT, CONSTRUCT_TYPED_SFIELD(sfNFTokenBrokerFee, "NFTokenBrokerFee", AMOUNT, 19); // currency amount (fees) -CONSTRUCT_TYPED_SFIELD(sfBaseFeeXRP, "BaseFeeXRP", AMOUNT, 20); -CONSTRUCT_TYPED_SFIELD(sfReserveBaseXRP, "ReserveBaseXRP", AMOUNT, 21); -CONSTRUCT_TYPED_SFIELD(sfReserveIncrementXRP, "ReserveIncrementXRP", AMOUNT, 22); +CONSTRUCT_TYPED_SFIELD(sfBaseFeeDrops, "BaseFeeDrops", AMOUNT, 20); +CONSTRUCT_TYPED_SFIELD(sfReserveBaseDrops, "ReserveBaseDrops", AMOUNT, 21); +CONSTRUCT_TYPED_SFIELD(sfReserveIncrementDrops, "ReserveIncrementDrops", AMOUNT, 22); // variable length (common) CONSTRUCT_TYPED_SFIELD(sfPublicKey, "PublicKey", VL, 1); diff --git a/src/ripple/protocol/impl/STValidation.cpp b/src/ripple/protocol/impl/STValidation.cpp index d83281a85d4..e62a81733bd 100644 --- a/src/ripple/protocol/impl/STValidation.cpp +++ b/src/ripple/protocol/impl/STValidation.cpp @@ -63,9 +63,9 @@ STValidation::validationFormat() {sfValidatedHash, soeOPTIONAL}, {sfServerVersion, soeOPTIONAL}, // featureXRPFees - {sfBaseFeeXRP, soeOPTIONAL}, - {sfReserveBaseXRP, soeOPTIONAL}, - {sfReserveIncrementXRP, soeOPTIONAL}, + {sfBaseFeeDrops, soeOPTIONAL}, + {sfReserveBaseDrops, soeOPTIONAL}, + {sfReserveIncrementDrops, soeOPTIONAL}, }; // clang-format on diff --git a/src/ripple/protocol/impl/TxFormats.cpp b/src/ripple/protocol/impl/TxFormats.cpp index 8e1b9a9ee46..fe42fd53e3c 100644 --- a/src/ripple/protocol/impl/TxFormats.cpp +++ b/src/ripple/protocol/impl/TxFormats.cpp @@ -161,9 +161,9 @@ TxFormats::TxFormats() {sfReserveBase, soeOPTIONAL}, {sfReserveIncrement, soeOPTIONAL}, // New version uses Amounts - {sfBaseFeeXRP, soeOPTIONAL}, - {sfReserveBaseXRP, soeOPTIONAL}, - {sfReserveIncrementXRP, soeOPTIONAL}, + {sfBaseFeeDrops, soeOPTIONAL}, + {sfReserveBaseDrops, soeOPTIONAL}, + {sfReserveIncrementDrops, soeOPTIONAL}, }, commonFields); diff --git a/src/test/app/PseudoTx_test.cpp b/src/test/app/PseudoTx_test.cpp index 597fb912006..78ca7cc05b1 100644 --- a/src/test/app/PseudoTx_test.cpp +++ b/src/test/app/PseudoTx_test.cpp @@ -37,9 +37,9 @@ struct PseudoTx_test : public beast::unit_test::suite obj[sfLedgerSequence] = seq; if (rules.enabled(featureXRPFees)) { - obj[sfBaseFeeXRP] = XRPAmount{0}; - obj[sfReserveBaseXRP] = XRPAmount{0}; - obj[sfReserveIncrementXRP] = XRPAmount{0}; + obj[sfBaseFeeDrops] = XRPAmount{0}; + obj[sfReserveBaseDrops] = XRPAmount{0}; + obj[sfReserveIncrementDrops] = XRPAmount{0}; } else { From fdff84d99899285ed440e8887a9c5e1bdd5abeb3 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 4 Jan 2023 12:28:18 -0500 Subject: [PATCH 5/8] [FOLD] Reserve SField IDs for Hooks, renumber fee fields --- src/ripple/protocol/impl/SField.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/ripple/protocol/impl/SField.cpp b/src/ripple/protocol/impl/SField.cpp index b954d64c214..f458c2dfe54 100644 --- a/src/ripple/protocol/impl/SField.cpp +++ b/src/ripple/protocol/impl/SField.cpp @@ -234,10 +234,12 @@ CONSTRUCT_TYPED_SFIELD(sfRippleEscrow, "RippleEscrow", AMOUNT, CONSTRUCT_TYPED_SFIELD(sfDeliveredAmount, "DeliveredAmount", AMOUNT, 18); CONSTRUCT_TYPED_SFIELD(sfNFTokenBrokerFee, "NFTokenBrokerFee", AMOUNT, 19); +// Reserve 20 & 21 for Hooks + // currency amount (fees) -CONSTRUCT_TYPED_SFIELD(sfBaseFeeDrops, "BaseFeeDrops", AMOUNT, 20); -CONSTRUCT_TYPED_SFIELD(sfReserveBaseDrops, "ReserveBaseDrops", AMOUNT, 21); -CONSTRUCT_TYPED_SFIELD(sfReserveIncrementDrops, "ReserveIncrementDrops", AMOUNT, 22); +CONSTRUCT_TYPED_SFIELD(sfBaseFeeDrops, "BaseFeeDrops", AMOUNT, 22); +CONSTRUCT_TYPED_SFIELD(sfReserveBaseDrops, "ReserveBaseDrops", AMOUNT, 23); +CONSTRUCT_TYPED_SFIELD(sfReserveIncrementDrops, "ReserveIncrementDrops", AMOUNT, 24); // variable length (common) CONSTRUCT_TYPED_SFIELD(sfPublicKey, "PublicKey", VL, 1); From a92f9473f9ce959a3522eb422ba72f6a33719dc2 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 9 Sep 2022 13:55:29 -0700 Subject: [PATCH 6/8] Always create the FeeSettings object in genesis ledger: * Initialize with default values from the config. Removes the need to pass a Config down into the Ledger initialization functions, including setup(). * Because old mainnet ledgers (prior to 562177 - yes, I looked it up), don't have FeeSettings, some of the other ctors will default them to the config values before setup() tries to load the object. * Update default Config fee values to match mainnet. * Fix unit tests: * Updated fees: Some tests are converted to use computed values of fee object, but the default Env config was also updated to fix the rest. * Unit tests that check the structure of the ledger have updated hashes and counts. --- src/ripple/app/ledger/Ledger.cpp | 67 ++++++++++---- src/ripple/app/ledger/Ledger.h | 10 ++- src/ripple/app/ledger/impl/BuildLedger.cpp | 6 +- src/ripple/app/ledger/impl/InboundLedger.cpp | 15 +++- src/ripple/app/main/Application.cpp | 17 +++- src/ripple/app/reporting/ReportingETL.cpp | 5 +- src/ripple/core/Config.h | 4 +- src/ripple/ledger/ReadView.h | 3 + src/ripple/ledger/impl/ReadView.cpp | 6 ++ src/ripple/nodestore/impl/Shard.cpp | 5 +- src/ripple/protocol/Rules.h | 8 ++ src/ripple/protocol/SystemParameters.h | 4 + src/ripple/protocol/impl/Rules.cpp | 13 +++ src/test/app/LedgerHistory_test.cpp | 3 +- src/test/app/LedgerLoad_test.cpp | 4 +- src/test/app/NFToken_test.cpp | 13 +-- src/test/app/Offer_test.cpp | 9 +- src/test/app/RCLValidations_test.cpp | 3 +- src/test/app/TxQ_test.cpp | 18 ++-- src/test/jtx/impl/envconfig.cpp | 11 ++- src/test/rpc/LedgerClosed_test.cpp | 4 +- src/test/rpc/LedgerData_test.cpp | 2 +- src/test/rpc/LedgerRPC_test.cpp | 30 +++---- src/test/rpc/LedgerRequestRPC_test.cpp | 92 ++++++++------------ src/test/rpc/ReportingETL_test.cpp | 2 +- 25 files changed, 223 insertions(+), 131 deletions(-) diff --git a/src/ripple/app/ledger/Ledger.cpp b/src/ripple/app/ledger/Ledger.cpp index 7757dac53bf..1e213d1f499 100644 --- a/src/ripple/app/ledger/Ledger.cpp +++ b/src/ripple/app/ledger/Ledger.cpp @@ -209,8 +209,33 @@ Ledger::Ledger( rawInsert(sle); } + { + auto sle = std::make_shared(keylet::fees()); + // Whether featureXRPFees is supported will depend on startup options. + if (std::find(amendments.begin(), amendments.end(), featureXRPFees) != + amendments.end()) + { + sle->at(sfBaseFeeDrops) = config.FEE_DEFAULT; + sle->at(sfReserveBaseDrops) = config.FEE_ACCOUNT_RESERVE; + sle->at(sfReserveIncrementDrops) = config.FEE_OWNER_RESERVE; + } + else + { + if (auto const f = config.FEE_DEFAULT.dropsAs()) + sle->at(sfBaseFee) = *f; + if (auto const f = + config.FEE_ACCOUNT_RESERVE.dropsAs()) + sle->at(sfReserveBase) = *f; + if (auto const f = + config.FEE_OWNER_RESERVE.dropsAs()) + sle->at(sfReserveIncrement) = *f; + sle->at(sfReferenceFeeUnits) = Config::FEE_UNITS_DEPRECATED; + } + rawInsert(sle); + } + stateMap_->flushDirty(hotACCOUNT_NODE); - setImmutable(config); + setImmutable(); } Ledger::Ledger( @@ -259,7 +284,8 @@ Ledger::Ledger( txMap_->setImmutable(); stateMap_->setImmutable(); - if (!setup(config)) + defaultFees(config); + if (!setup()) loaded = false; if (!loaded) @@ -329,11 +355,12 @@ Ledger::Ledger( info_.seq = ledgerSeq; info_.closeTime = closeTime; info_.closeTimeResolution = ledgerDefaultTimeResolution; - setup(config); + defaultFees(config); + setup(); } void -Ledger::setImmutable(Config const& config, bool rehash) +Ledger::setImmutable(bool rehash) { // Force update, since this is the only // place the hash transitions to valid @@ -349,15 +376,14 @@ Ledger::setImmutable(Config const& config, bool rehash) mImmutable = true; txMap_->setImmutable(); stateMap_->setImmutable(); - setup(config); + setup(); } void Ledger::setAccepted( NetClock::time_point closeTime, NetClock::duration closeResolution, - bool correctCloseTime, - Config const& config) + bool correctCloseTime) { // Used when we witnessed the consensus. assert(!open()); @@ -365,7 +391,7 @@ Ledger::setAccepted( info_.closeTime = closeTime; info_.closeTimeResolution = closeResolution; info_.closeFlags = correctCloseTime ? 0 : sLCF_NoConsensusTime; - setImmutable(config); + setImmutable(); } bool @@ -587,13 +613,13 @@ Ledger::rawTxInsertWithHash( } bool -Ledger::setup(Config const& config) +Ledger::setup() { bool ret = true; try { - rules_ = makeRulesGivenLedger(*this, config.features); + rules_ = makeRulesGivenLedger(*this, rules_); } catch (SHAMapMissingNode const&) { @@ -604,10 +630,6 @@ Ledger::setup(Config const& config) Rethrow(); } - fees_.base = config.FEE_DEFAULT; - fees_.reserve = config.FEE_ACCOUNT_RESERVE; - fees_.increment = config.FEE_OWNER_RESERVE; - try { if (auto const sle = read(keylet::fees())) @@ -667,6 +689,18 @@ Ledger::setup(Config const& config) return ret; } +void +Ledger::defaultFees(Config const& config) +{ + assert(fees_.base == 0 && fees_.reserve == 0 && fees_.increment == 0); + if (fees_.base == 0) + fees_.base = config.FEE_DEFAULT; + if (fees_.reserve == 0) + fees_.reserve = config.FEE_ACCOUNT_RESERVE; + if (fees_.increment == 0) + fees_.increment = config.FEE_OWNER_RESERVE; +} + std::shared_ptr Ledger::peek(Keylet const& k) const { @@ -1071,7 +1105,10 @@ finishLoadByIndexOrHash( if (!ledger) return; - ledger->setImmutable(config); + assert( + ledger->info().seq < XRP_LEDGER_EARLIEST_FEES || + ledger->read(keylet::fees())); + ledger->setImmutable(); JLOG(j.trace()) << "Loaded ledger: " << to_string(ledger->info().hash); diff --git a/src/ripple/app/ledger/Ledger.h b/src/ripple/app/ledger/Ledger.h index caf68b3eac8..0b0830df9d9 100644 --- a/src/ripple/app/ledger/Ledger.h +++ b/src/ripple/app/ledger/Ledger.h @@ -266,11 +266,10 @@ class Ledger final : public std::enable_shared_from_this, setAccepted( NetClock::time_point closeTime, NetClock::duration closeResolution, - bool correctCloseTime, - Config const& config); + bool correctCloseTime); void - setImmutable(Config const& config, bool rehash = true); + setImmutable(bool rehash = true); bool isImmutable() const @@ -395,7 +394,10 @@ class Ledger final : public std::enable_shared_from_this, class txs_iter_impl; bool - setup(Config const& config); + setup(); + + void + defaultFees(Config const& config); bool mImmutable; diff --git a/src/ripple/app/ledger/impl/BuildLedger.cpp b/src/ripple/app/ledger/impl/BuildLedger.cpp index f70b754ab7c..87960d16eac 100644 --- a/src/ripple/app/ledger/impl/BuildLedger.cpp +++ b/src/ripple/app/ledger/impl/BuildLedger.cpp @@ -75,8 +75,10 @@ buildLedgerImpl( built->unshare(); // Accept ledger - built->setAccepted( - closeTime, closeResolution, closeTimeCorrect, app.config()); + assert( + built->info().seq < XRP_LEDGER_EARLIEST_FEES || + built->read(keylet::fees())); + built->setAccepted(closeTime, closeResolution, closeTimeCorrect); return built; } diff --git a/src/ripple/app/ledger/impl/InboundLedger.cpp b/src/ripple/app/ledger/impl/InboundLedger.cpp index 3ecba97b199..af3ba8a7a9b 100644 --- a/src/ripple/app/ledger/impl/InboundLedger.cpp +++ b/src/ripple/app/ledger/impl/InboundLedger.cpp @@ -155,7 +155,10 @@ InboundLedger::init(ScopedLockType& collectionLock) JLOG(journal_.debug()) << "Acquiring ledger we already have in " << " local store. " << hash_; - mLedger->setImmutable(app_.config()); + assert( + mLedger->info().seq < XRP_LEDGER_EARLIEST_FEES || + mLedger->read(keylet::fees())); + mLedger->setImmutable(); if (mReason == Reason::HISTORY || mReason == Reason::SHARD) return; @@ -416,7 +419,10 @@ InboundLedger::tryDB(NodeStore::Database& srcDB) { JLOG(journal_.debug()) << "Had everything locally"; complete_ = true; - mLedger->setImmutable(app_.config()); + assert( + mLedger->info().seq < XRP_LEDGER_EARLIEST_FEES || + mLedger->read(keylet::fees())); + mLedger->setImmutable(); } } @@ -513,7 +519,10 @@ InboundLedger::done() if (complete_ && !failed_ && mLedger) { - mLedger->setImmutable(app_.config()); + assert( + mLedger->info().seq < XRP_LEDGER_EARLIEST_FEES || + mLedger->read(keylet::fees())); + mLedger->setImmutable(); switch (mReason) { case Reason::SHARD: diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index dce11bc38f0..16781ac09d4 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1699,7 +1699,7 @@ ApplicationImp::fdRequired() const void ApplicationImp::startGenesisLedger() { - std::vector initialAmendments = + std::vector const initialAmendments = (config_->START_UP == Config::FRESH) ? m_amendmentTable->getDesired() : std::vector{}; @@ -1710,7 +1710,10 @@ ApplicationImp::startGenesisLedger() auto const next = std::make_shared(*genesis, timeKeeper().closeTime()); next->updateSkipList(); - next->setImmutable(*config_); + assert( + next->info().seq < XRP_LEDGER_EARLIEST_FEES || + next->read(keylet::fees())); + next->setImmutable(); openLedger_.emplace(next, cachedSLEs_, logs_->journal("OpenLedger")); m_ledgerMaster->storeLedger(next); m_ledgerMaster->switchLCL(next); @@ -1728,7 +1731,10 @@ ApplicationImp::getLastFullLedger() if (!ledger) return ledger; - ledger->setImmutable(*config_); + assert( + ledger->info().seq < XRP_LEDGER_EARLIEST_FEES || + ledger->read(keylet::fees())); + ledger->setImmutable(); if (getLedgerMaster().haveLedger(seq)) ledger->setValidated(); @@ -1879,8 +1885,11 @@ ApplicationImp::loadLedgerFromFile(std::string const& name) loadLedger->stateMap().flushDirty(hotACCOUNT_NODE); + assert( + loadLedger->info().seq < XRP_LEDGER_EARLIEST_FEES || + loadLedger->read(keylet::fees())); loadLedger->setAccepted( - closeTime, closeTimeResolution, !closeTimeEstimated, *config_); + closeTime, closeTimeResolution, !closeTimeEstimated); return loadLedger; } diff --git a/src/ripple/app/reporting/ReportingETL.cpp b/src/ripple/app/reporting/ReportingETL.cpp index 7e15d242a72..d8d6af36881 100644 --- a/src/ripple/app/reporting/ReportingETL.cpp +++ b/src/ripple/app/reporting/ReportingETL.cpp @@ -189,7 +189,10 @@ ReportingETL::flushLedger(std::shared_ptr& ledger) auto& txHash = ledger->info().txHash; auto& ledgerHash = ledger->info().hash; - ledger->setImmutable(app_.config(), false); + assert( + ledger->info().seq < XRP_LEDGER_EARLIEST_FEES || + ledger->read(keylet::fees())); + ledger->setImmutable(false); auto start = std::chrono::system_clock::now(); auto numFlushed = ledger->stateMap().flushDirty(hotACCOUNT_NODE); diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index 1e91f49263b..0db0290f505 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -186,8 +186,8 @@ class Config : public BasicConfig VALIDATION_QUORUM; // validations to consider ledger authoritative XRPAmount FEE_DEFAULT{10}; - XRPAmount FEE_ACCOUNT_RESERVE{200 * DROPS_PER_XRP}; - XRPAmount FEE_OWNER_RESERVE{50 * DROPS_PER_XRP}; + XRPAmount FEE_ACCOUNT_RESERVE{10 * DROPS_PER_XRP}; + XRPAmount FEE_OWNER_RESERVE{2 * DROPS_PER_XRP}; // Node storage configuration std::uint32_t LEDGER_HISTORY = 256; diff --git a/src/ripple/ledger/ReadView.h b/src/ripple/ledger/ReadView.h index fb9e37c7458..e019d602f07 100644 --- a/src/ripple/ledger/ReadView.h +++ b/src/ripple/ledger/ReadView.h @@ -356,6 +356,9 @@ getCloseAgree(LedgerInfo const& info) void addRaw(LedgerInfo const&, Serializer&, bool includeHash = false); +Rules +makeRulesGivenLedger(DigestAwareReadView const& ledger, Rules const& current); + Rules makeRulesGivenLedger( DigestAwareReadView const& ledger, diff --git a/src/ripple/ledger/impl/ReadView.cpp b/src/ripple/ledger/impl/ReadView.cpp index 57af008b47c..1ce21777297 100644 --- a/src/ripple/ledger/impl/ReadView.cpp +++ b/src/ripple/ledger/impl/ReadView.cpp @@ -65,6 +65,12 @@ ReadView::txs_type::end() const -> iterator return iterator(view_, view_->txsEnd()); } +Rules +makeRulesGivenLedger(DigestAwareReadView const& ledger, Rules const& current) +{ + return makeRulesGivenLedger(ledger, current.presets()); +} + Rules makeRulesGivenLedger( DigestAwareReadView const& ledger, diff --git a/src/ripple/nodestore/impl/Shard.cpp b/src/ripple/nodestore/impl/Shard.cpp index 030fbf4aa12..14bfe487303 100644 --- a/src/ripple/nodestore/impl/Shard.cpp +++ b/src/ripple/nodestore/impl/Shard.cpp @@ -688,7 +688,10 @@ Shard::finalize(bool writeSQLite, std::optional const& referenceHash) ledger->stateMap().setLedgerSeq(ledgerSeq); ledger->txMap().setLedgerSeq(ledgerSeq); - ledger->setImmutable(config); + assert( + ledger->info().seq < XRP_LEDGER_EARLIEST_FEES || + ledger->read(keylet::fees())); + ledger->setImmutable(); if (!ledger->stateMap().fetchRoot( SHAMapHash{ledger->info().accountHash}, nullptr)) { diff --git a/src/ripple/protocol/Rules.h b/src/ripple/protocol/Rules.h index d8190e86a71..52d01c6f589 100644 --- a/src/ripple/protocol/Rules.h +++ b/src/ripple/protocol/Rules.h @@ -56,6 +56,11 @@ class Rules private: // Allow a friend function to construct Rules. + friend Rules + makeRulesGivenLedger( + DigestAwareReadView const& ledger, + Rules const& current); + friend Rules makeRulesGivenLedger( DigestAwareReadView const& ledger, @@ -66,6 +71,9 @@ class Rules std::optional const& digest, STVector256 const& amendments); + std::unordered_set> const& + presets() const; + public: /** Returns `true` if a feature is enabled. */ bool diff --git a/src/ripple/protocol/SystemParameters.h b/src/ripple/protocol/SystemParameters.h index db0c15dcad7..bc2f7136ff9 100644 --- a/src/ripple/protocol/SystemParameters.h +++ b/src/ripple/protocol/SystemParameters.h @@ -68,6 +68,10 @@ systemCurrencyCode() /** The XRP ledger network's earliest allowed sequence */ static constexpr std::uint32_t XRP_LEDGER_EARLIEST_SEQ{32570u}; +/** The XRP Ledger mainnet's earliest ledger with a FeeSettings object. Only + * used in asserts and tests. */ +static constexpr std::uint32_t XRP_LEDGER_EARLIEST_FEES{562177u}; + /** The number of ledgers in a shard */ static constexpr std::uint32_t DEFAULT_LEDGERS_PER_SHARD{16384u}; diff --git a/src/ripple/protocol/impl/Rules.cpp b/src/ripple/protocol/impl/Rules.cpp index 35a09b85658..c8f4720bd6c 100644 --- a/src/ripple/protocol/impl/Rules.cpp +++ b/src/ripple/protocol/impl/Rules.cpp @@ -45,6 +45,12 @@ class Rules::Impl set_.insert(amendments.begin(), amendments.end()); } + std::unordered_set> const& + presets() const + { + return presets_; + } + bool enabled(uint256 const& feature) const { @@ -60,6 +66,7 @@ class Rules::Impl return true; if (!digest_ || !other.digest_) return false; + assert(presets_ == other.presets_); return *digest_ == *other.digest_; } }; @@ -77,6 +84,12 @@ Rules::Rules( { } +std::unordered_set> const& +Rules::presets() const +{ + return impl_->presets(); +} + bool Rules::enabled(uint256 const& feature) const { diff --git a/src/test/app/LedgerHistory_test.cpp b/src/test/app/LedgerHistory_test.cpp index ba4faa9da05..880cbea5980 100644 --- a/src/test/app/LedgerHistory_test.cpp +++ b/src/test/app/LedgerHistory_test.cpp @@ -81,8 +81,7 @@ class LedgerHistory_test : public beast::unit_test::suite res->setAccepted( res->info().closeTime, res->info().closeTimeResolution, - true /* close time correct*/, - env.app().config()); + true /* close time correct*/); lh.insert(res, false); return res; } diff --git a/src/test/app/LedgerLoad_test.cpp b/src/test/app/LedgerLoad_test.cpp index d78d25ea05a..2685014e474 100644 --- a/src/test/app/LedgerLoad_test.cpp +++ b/src/test/app/LedgerLoad_test.cpp @@ -82,7 +82,7 @@ class LedgerLoad_test : public beast::unit_test::suite retval.ledger = env.rpc("ledger", "current", "full")[jss::result]; BEAST_EXPECT( - retval.ledger[jss::ledger][jss::accountState].size() == 101); + retval.ledger[jss::ledger][jss::accountState].size() == 102); retval.hashes = [&] { for (auto const& it : retval.ledger[jss::ledger][jss::accountState]) @@ -193,7 +193,7 @@ class LedgerLoad_test : public beast::unit_test::suite nullptr, beast::severities::kDisabled); auto jrb = env.rpc("ledger", "current", "full")[jss::result]; - BEAST_EXPECT(jrb[jss::ledger][jss::accountState].size() == 97); + BEAST_EXPECT(jrb[jss::ledger][jss::accountState].size() == 98); BEAST_EXPECT( jrb[jss::ledger][jss::accountState].size() <= sd.ledger[jss::ledger][jss::accountState].size()); diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 42a6eb4d3ce..a466573f8c2 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -215,12 +215,13 @@ class NFToken_test : public beast::unit_test::suite Account const minter{"minter"}; // Fund alice and minter enough to exist, but not enough to meet - // the reserve for creating their first NFT. Account reserve for unit - // tests is 200 XRP, not 20. - env.fund(XRP(200), alice, minter); + // the reserve for creating their first NFT. + auto const acctReserve = env.current()->fees().accountReserve(0); + auto const incReserve = env.current()->fees().increment; + env.fund(acctReserve, alice, minter); env.close(); - BEAST_EXPECT(env.balance(alice) == XRP(200)); - BEAST_EXPECT(env.balance(minter) == XRP(200)); + BEAST_EXPECT(env.balance(alice) == acctReserve); + BEAST_EXPECT(env.balance(minter) == acctReserve); BEAST_EXPECT(ownerCount(env, alice) == 0); BEAST_EXPECT(ownerCount(env, minter) == 0); @@ -232,7 +233,7 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(burnedCount(env, alice) == 0); // Pay alice almost enough to make the reserve for an NFT page. - env(pay(env.master, alice, XRP(50) + drops(9))); + env(pay(env.master, alice, incReserve + drops(9))); env.close(); // A lambda that checks alice's ownerCount, mintedCount, and diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 9f6e165bc16..194626965a8 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -2079,7 +2079,8 @@ class Offer_test : public beast::unit_test::suite BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "100"); jrr = ledgerEntryRoot(env, alice); BEAST_EXPECT( - jrr[jss::node][sfBalance.fieldName] == XRP(350).value().getText()); + jrr[jss::node][sfBalance.fieldName] == + STAmount(env.current()->fees().accountReserve(3)).getText()); jrr = ledgerEntryState(env, bob, gw1, "USD"); BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "-400"); @@ -2160,7 +2161,8 @@ class Offer_test : public beast::unit_test::suite BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "-100"); jrr = ledgerEntryRoot(env, alice); BEAST_EXPECT( - jrr[jss::node][sfBalance.fieldName] == XRP(250).value().getText()); + jrr[jss::node][sfBalance.fieldName] == + STAmount(env.current()->fees().accountReserve(1)).getText()); jrr = ledgerEntryState(env, bob, gw, "USD"); BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "-400"); @@ -2203,7 +2205,8 @@ class Offer_test : public beast::unit_test::suite BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "-200"); jrr = ledgerEntryRoot(env, alice); BEAST_EXPECT( - jrr[jss::node][sfBalance.fieldName] == XRP(250).value().getText()); + jrr[jss::node][sfBalance.fieldName] == + STAmount(env.current()->fees().accountReserve(1)).getText()); jrr = ledgerEntryState(env, bob, gw, "USD"); BEAST_EXPECT(jrr[jss::node][sfBalance.fieldName][jss::value] == "-300"); diff --git a/src/test/app/RCLValidations_test.cpp b/src/test/app/RCLValidations_test.cpp index 14a54a1492f..ffb468c1cf1 100644 --- a/src/test/app/RCLValidations_test.cpp +++ b/src/test/app/RCLValidations_test.cpp @@ -108,7 +108,8 @@ class RCLValidations_test : public beast::unit_test::suite next->updateSkipList(); if (forceHash) { - next->setImmutable(config); + BEAST_EXPECT(next->read(keylet::fees())); + next->setImmutable(); forceHash = false; } diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index e25c9f60de1..9acbd5493b4 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -1448,7 +1448,7 @@ class TxQ1_test : public beast::unit_test::suite // These tests may change if TxQ ordering is changed using namespace std::string_literals; BEAST_EXPECTS( - aliceSeq + 1 == env.seq(alice), + aliceSeq == env.seq(alice), "alice: "s + std::to_string(aliceSeq) + ", " + std::to_string(env.seq(alice))); BEAST_EXPECTS( @@ -1460,7 +1460,7 @@ class TxQ1_test : public beast::unit_test::suite "charlie: "s + std::to_string(charlieSeq) + ", " + std::to_string(env.seq(charlie))); BEAST_EXPECTS( - dariaSeq == env.seq(daria), + dariaSeq + 1 == env.seq(daria), "daria: "s + std::to_string(dariaSeq) + ", " + std::to_string(env.seq(daria))); BEAST_EXPECTS( @@ -1472,24 +1472,24 @@ class TxQ1_test : public beast::unit_test::suite "fred: "s + std::to_string(fredSeq) + ", " + std::to_string(env.seq(fred))); BEAST_EXPECTS( - gwenSeq + 1 == env.seq(gwen), + gwenSeq == env.seq(gwen), "gwen: "s + std::to_string(gwenSeq) + ", " + std::to_string(env.seq(gwen))); BEAST_EXPECTS( - hankSeq == env.seq(hank), + hankSeq + 1 == env.seq(hank), "hank: "s + std::to_string(hankSeq) + ", " + std::to_string(env.seq(hank))); // Which sequences get incremented may change if TxQ ordering is // changed - ++aliceSeq; + //++aliceSeq; ++bobSeq; ++(++charlieSeq); - // ++dariaSeq; + ++dariaSeq; ++elmoSeq; // ++fredSeq; - ++gwenSeq; - // ++hankSeq; + //++gwenSeq; + ++hankSeq; auto getTxsQueued = [&]() { auto const txs = env.app().getTxQ().getTxs(); @@ -2944,7 +2944,7 @@ class TxQ1_test : public beast::unit_test::suite // we only see a reduction by 5. env.close(); checkMetrics(__LINE__, env, 9, 50, 6, 5, 256); - BEAST_EXPECT(env.seq(alice) == aliceSeq + 15); + BEAST_EXPECT(env.seq(alice) == aliceSeq + 13); // Close ledger 7. That should remove 7 more of alice's transactions. env.close(); diff --git a/src/test/jtx/impl/envconfig.cpp b/src/test/jtx/impl/envconfig.cpp index f34a444fa5f..598b8a96f04 100644 --- a/src/test/jtx/impl/envconfig.cpp +++ b/src/test/jtx/impl/envconfig.cpp @@ -17,9 +17,11 @@ */ //============================================================================== +#include + #include #include -#include +#include namespace ripple { namespace test { @@ -40,6 +42,13 @@ setupConfigForUnitTests(Config& cfg) std::string port_rpc = std::to_string(port_base + 1); std::string port_ws = std::to_string(port_base + 2); + using namespace jtx; + // Default fees to old values, so tests don't have to worry about changes in + // Config.h + cfg.FEE_DEFAULT = 10; + cfg.FEE_ACCOUNT_RESERVE = XRP(200).value().xrp().drops(); + cfg.FEE_OWNER_RESERVE = XRP(50).value().xrp().drops(); + cfg.overwrite(ConfigSection::nodeDatabase(), "type", "memory"); cfg.overwrite(ConfigSection::nodeDatabase(), "path", "main"); cfg.deprecatedClearSection(ConfigSection::importNodeDatabase()); diff --git a/src/test/rpc/LedgerClosed_test.cpp b/src/test/rpc/LedgerClosed_test.cpp index c5073db328e..2f81031f85c 100644 --- a/src/test/rpc/LedgerClosed_test.cpp +++ b/src/test/rpc/LedgerClosed_test.cpp @@ -37,7 +37,7 @@ class LedgerClosed_test : public beast::unit_test::suite auto lc_result = env.rpc("ledger_closed")[jss::result]; BEAST_EXPECT( lc_result[jss::ledger_hash] == - "A15F7FBE0B06286915D971BF9802C9431CD7DE40E2AC7D07C409EDB1C0715C60"); + "CCC3B3E88CCAC17F1BE6B4A648A55999411F19E3FE55EB721960EB0DF28EDDA5"); BEAST_EXPECT(lc_result[jss::ledger_index] == 2); env.close(); @@ -52,7 +52,7 @@ class LedgerClosed_test : public beast::unit_test::suite lc_result = env.rpc("ledger_closed")[jss::result]; BEAST_EXPECT( lc_result[jss::ledger_hash] == - "2E81FC6EC0DD943197E0C7E3FBE9AE307F2775F2F7485BB37307984C3C0F2340"); + "E86DE7F3D7A4D9CE17EF7C8BA08A8F4D8F643B9552F0D895A31CDA78F541DE4E"); BEAST_EXPECT(lc_result[jss::ledger_index] == 3); } diff --git a/src/test/rpc/LedgerData_test.cpp b/src/test/rpc/LedgerData_test.cpp index 1c55e907062..ab520181c05 100644 --- a/src/test/rpc/LedgerData_test.cpp +++ b/src/test/rpc/LedgerData_test.cpp @@ -123,7 +123,7 @@ class LedgerData_test : public beast::unit_test::suite jrr[jss::ledger_current_index].isIntegral() && jrr[jss::ledger_current_index].asInt() > 0); BEAST_EXPECT(!jrr.isMember(jss::marker)); - BEAST_EXPECT(checkArraySize(jrr[jss::state], num_accounts + 3)); + BEAST_EXPECT(checkArraySize(jrr[jss::state], num_accounts + 4)); } void diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 5494a81da63..6644e15e959 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -237,7 +237,7 @@ class LedgerRPC_test : public beast::unit_test::suite env.rpc("json", "ledger", to_string(jvParams))[jss::result]; BEAST_EXPECT(jrr[jss::ledger].isMember(jss::accountState)); BEAST_EXPECT(jrr[jss::ledger][jss::accountState].isArray()); - BEAST_EXPECT(jrr[jss::ledger][jss::accountState].size() == 2u); + BEAST_EXPECT(jrr[jss::ledger][jss::accountState].size() == 3u); } void @@ -276,7 +276,7 @@ class LedgerRPC_test : public beast::unit_test::suite env.rpc("json", "ledger", to_string(jvParams))[jss::result]; BEAST_EXPECT(jrr[jss::ledger].isMember(jss::accountState)); BEAST_EXPECT(jrr[jss::ledger][jss::accountState].isArray()); - BEAST_EXPECT(jrr[jss::ledger][jss::accountState].size() == 2u); + BEAST_EXPECT(jrr[jss::ledger][jss::accountState].size() == 3u); } void @@ -1315,11 +1315,12 @@ class LedgerRPC_test : public beast::unit_test::suite } { + std::string const hash3{ + "E86DE7F3D7A4D9CE17EF7C8BA08A8F4D" + "8F643B9552F0D895A31CDA78F541DE4E"}; // access via the ledger_hash field Json::Value jvParams; - jvParams[jss::ledger_hash] = - "2E81FC6EC0DD943197E0C7E3FBE9AE30" - "7F2775F2F7485BB37307984C3C0F2340"; + jvParams[jss::ledger_hash] = hash3; auto jrr = env.rpc( "json", "ledger", @@ -1328,11 +1329,8 @@ class LedgerRPC_test : public beast::unit_test::suite BEAST_EXPECT(jrr.isMember(jss::ledger_hash)); BEAST_EXPECT(jrr[jss::ledger][jss::ledger_index] == "3"); - // extra leading hex chars in hash will be ignored - jvParams[jss::ledger_hash] = - "DEADBEEF" - "2E81FC6EC0DD943197E0C7E3FBE9AE30" - "7F2775F2F7485BB37307984C3C0F2340"; + // extra leading hex chars in hash are not allowed + jvParams[jss::ledger_hash] = "DEADBEEF" + hash3; jrr = env.rpc( "json", "ledger", @@ -1535,7 +1533,7 @@ class LedgerRPC_test : public beast::unit_test::suite env.close(); jrr = env.rpc("json", "ledger", to_string(jv))[jss::result]; - const std::string txid1 = [&]() { + const std::string txid0 = [&]() { auto const& parentHash = env.current()->info().parentHash; if (BEAST_EXPECT(jrr[jss::queue_data].size() == 2)) { @@ -1579,22 +1577,22 @@ class LedgerRPC_test : public beast::unit_test::suite if (BEAST_EXPECT(jrr[jss::queue_data].size() == 2)) { auto const& parentHash = env.current()->info().parentHash; - auto const txid0 = [&]() { - auto const& txj = jrr[jss::queue_data][0u]; + auto const txid1 = [&]() { + auto const& txj = jrr[jss::queue_data][1u]; BEAST_EXPECT(txj[jss::account] == alice.human()); BEAST_EXPECT(txj[jss::fee_level] == "256"); BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); BEAST_EXPECT(txj.isMember(jss::tx)); return txj[jss::tx].asString(); }(); - auto const& txj = jrr[jss::queue_data][1u]; + auto const& txj = jrr[jss::queue_data][0u]; BEAST_EXPECT(txj[jss::account] == alice.human()); BEAST_EXPECT(txj[jss::fee_level] == "256"); BEAST_EXPECT(txj["preflight_result"] == "tesSUCCESS"); BEAST_EXPECT(txj["retries_remaining"] == 9); BEAST_EXPECT(txj["last_result"] == "terPRE_SEQ"); BEAST_EXPECT(txj.isMember(jss::tx)); - BEAST_EXPECT(txj[jss::tx] == txid1); + BEAST_EXPECT(txj[jss::tx] == txid0); uint256 tx0, tx1; BEAST_EXPECT(tx0.parseHex(txid0)); BEAST_EXPECT(tx1.parseHex(txid1)); @@ -1647,7 +1645,7 @@ class LedgerRPC_test : public beast::unit_test::suite BEAST_EXPECT(txj["retries_remaining"] == 1); BEAST_EXPECT(txj["last_result"] == "terPRE_SEQ"); BEAST_EXPECT(txj.isMember(jss::tx)); - BEAST_EXPECT(txj[jss::tx] != txid1); + BEAST_EXPECT(txj[jss::tx] != txid0); return txj[jss::tx].asString(); } return std::string{}; diff --git a/src/test/rpc/LedgerRequestRPC_test.cpp b/src/test/rpc/LedgerRequestRPC_test.cpp index de2ddeff8e4..14d06e9234b 100644 --- a/src/test/rpc/LedgerRequestRPC_test.cpp +++ b/src/test/rpc/LedgerRequestRPC_test.cpp @@ -30,6 +30,14 @@ namespace RPC { class LedgerRequestRPC_test : public beast::unit_test::suite { + static constexpr char const* hash1 = + "3020EB9E7BE24EF7D7A060CB051583EC117384636D1781AFB5B87F3E348DA489"; + static constexpr char const* accounthash1 = + "BD8A3D72CA73DDE887AD63666EC2BAD07875CBA997A102579B5B95ECDFFEAED8"; + + static constexpr char const* zerohash = + "0000000000000000000000000000000000000000000000000000000000000000"; + public: void testLedgerRequest() @@ -181,87 +189,69 @@ class LedgerRequestRPC_test : public beast::unit_test::suite BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "100000000000000000"); BEAST_EXPECT(result[jss::ledger][jss::closed] == true); - BEAST_EXPECT( - result[jss::ledger][jss::ledger_hash] == - "E9BB323980D202EC7E51BAB2AA8E35353F9C7BDAB59BF17378EADD4D0486EF9F"); - BEAST_EXPECT( - result[jss::ledger][jss::parent_hash] == - "0000000000000000000000000000000000000000000000000000000000000000"); - BEAST_EXPECT( - result[jss::ledger][jss::account_hash] == - "A21ED30C04C88046FC61DB9DC19375EEDBD365FD8C17286F27127DF804E9CAA6"); - BEAST_EXPECT( - result[jss::ledger][jss::transaction_hash] == - "0000000000000000000000000000000000000000000000000000000000000000"); + BEAST_EXPECT(result[jss::ledger][jss::ledger_hash] == hash1); + BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == zerohash); + BEAST_EXPECT(result[jss::ledger][jss::account_hash] == accounthash1); + BEAST_EXPECT(result[jss::ledger][jss::transaction_hash] == zerohash); result = env.rpc("ledger_request", "2")[jss::result]; + constexpr char const* hash2 = + "CCC3B3E88CCAC17F1BE6B4A648A55999411F19E3FE55EB721960EB0DF28EDDA5"; BEAST_EXPECT(result[jss::ledger][jss::ledger_index] == "2"); BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "100000000000000000"); BEAST_EXPECT(result[jss::ledger][jss::closed] == true); - BEAST_EXPECT( - result[jss::ledger][jss::ledger_hash] == - "A15F7FBE0B06286915D971BF9802C9431CD7DE40E2AC7D07C409EDB1C0715C60"); - BEAST_EXPECT( - result[jss::ledger][jss::parent_hash] == - "E9BB323980D202EC7E51BAB2AA8E35353F9C7BDAB59BF17378EADD4D0486EF9F"); + BEAST_EXPECT(result[jss::ledger][jss::ledger_hash] == hash2); + BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == hash1); BEAST_EXPECT( result[jss::ledger][jss::account_hash] == - "CB07F3CA0398BE969A5B88F874629D4DBB6E103DE7C6DB8037281A89E51AA8C6"); - BEAST_EXPECT( - result[jss::ledger][jss::transaction_hash] == - "0000000000000000000000000000000000000000000000000000000000000000"); + "3C834285F7F464FBE99AFEB84D354A968EB2CAA24523FF26797A973D906A3D29"); + BEAST_EXPECT(result[jss::ledger][jss::transaction_hash] == zerohash); result = env.rpc("ledger_request", "3")[jss::result]; + constexpr char const* hash3 = + "8D631B20BC989AF568FBA97375290544B0703A5ADC1CF9E9053580461690C9EE"; BEAST_EXPECT(result[jss::ledger][jss::ledger_index] == "3"); BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "99999999999999980"); BEAST_EXPECT(result[jss::ledger][jss::closed] == true); - BEAST_EXPECT( - result[jss::ledger][jss::ledger_hash] == - "9BCA8AE5FD41D223D82E1B8288961D693EB1B2EFA10F51827A641AD4B12111D7"); - BEAST_EXPECT( - result[jss::ledger][jss::parent_hash] == - "A15F7FBE0B06286915D971BF9802C9431CD7DE40E2AC7D07C409EDB1C0715C60"); + BEAST_EXPECT(result[jss::ledger][jss::ledger_hash] == hash3); + BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == hash2); BEAST_EXPECT( result[jss::ledger][jss::account_hash] == - "5B793533909906D15CE27D1A423732D113160AB166188D89A2DFD8737CBDCBD5"); + "BC9EF2A16BFF80BCFABA6FA84688D858D33BD0FA0435CAA9DF6DA4105A39A29E"); BEAST_EXPECT( result[jss::ledger][jss::transaction_hash] == "0213EC486C058B3942FBE3DAC6839949A5C5B02B8B4244C8998EFDF04DBD8222"); result = env.rpc("ledger_request", "4")[jss::result]; + constexpr char const* hash4 = + "1A8E7098B23597E73094DADA58C9D62F3AB93A12C6F7666D56CA85A6CFDE530F"; BEAST_EXPECT(result[jss::ledger][jss::ledger_index] == "4"); BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "99999999999999960"); BEAST_EXPECT(result[jss::ledger][jss::closed] == true); - BEAST_EXPECT( - result[jss::ledger][jss::ledger_hash] == - "433D1E42F2735F926BF594E4F3DFC70AE3E74F51464156ED83A33D0FF121D136"); - BEAST_EXPECT( - result[jss::ledger][jss::parent_hash] == - "9BCA8AE5FD41D223D82E1B8288961D693EB1B2EFA10F51827A641AD4B12111D7"); + BEAST_EXPECT(result[jss::ledger][jss::ledger_hash] == hash4); + BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == hash3); BEAST_EXPECT( result[jss::ledger][jss::account_hash] == - "39C91E2227ACECD057AFDC64AE8FEFF5A0E07CF26ED29D1AECC55B0385F3EFDE"); + "C690188F123C91355ADA8BDF4AC5B5C927076D3590C215096868A5255264C6DD"); BEAST_EXPECT( result[jss::ledger][jss::transaction_hash] == "3CBDB8F42E04333E1642166BFB93AC9A7E1C6C067092CD5D881D6F3AB3D67E76"); result = env.rpc("ledger_request", "5")[jss::result]; + constexpr char const* hash5 = + "C6A222D71AE65D7B4F240009EAD5DEB20D7EEDE5A4064F28BBDBFEEB6FBE48E5"; BEAST_EXPECT(result[jss::ledger][jss::ledger_index] == "5"); BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "99999999999999940"); BEAST_EXPECT(result[jss::ledger][jss::closed] == true); - BEAST_EXPECT( - result[jss::ledger][jss::ledger_hash] == - "9ED4D0C397810980904AF3FC08583D23B09C3C7CCF835D2A4768145A8BAC1175"); - BEAST_EXPECT( - result[jss::ledger][jss::parent_hash] == - "433D1E42F2735F926BF594E4F3DFC70AE3E74F51464156ED83A33D0FF121D136"); + BEAST_EXPECT(result[jss::ledger][jss::ledger_hash] == hash5); + BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == hash4); BEAST_EXPECT( result[jss::ledger][jss::account_hash] == - "8F047B6A0D2083DF4F69C17F7CC9AE997B0D59020A43D9799A31D22F55837147"); + "EA81CD9D36740736F00CB747E0D0E32D3C10B695823D961F0FB9A1CE7133DD4D"); BEAST_EXPECT( result[jss::ledger][jss::transaction_hash] == "C3D086CD6BDB9E97AD1D513B2C049EF2840BD21D0B3E22D84EBBB89B6D2EF59D"); @@ -340,18 +330,10 @@ class LedgerRequestRPC_test : public beast::unit_test::suite BEAST_EXPECT( result[jss::ledger][jss::total_coins] == "100000000000000000"); BEAST_EXPECT(result[jss::ledger][jss::closed] == true); - BEAST_EXPECT( - result[jss::ledger][jss::ledger_hash] == - "E9BB323980D202EC7E51BAB2AA8E35353F9C7BDAB59BF17378EADD4D0486EF9F"); - BEAST_EXPECT( - result[jss::ledger][jss::parent_hash] == - "0000000000000000000000000000000000000000000000000000000000000000"); - BEAST_EXPECT( - result[jss::ledger][jss::account_hash] == - "A21ED30C04C88046FC61DB9DC19375EEDBD365FD8C17286F27127DF804E9CAA6"); - BEAST_EXPECT( - result[jss::ledger][jss::transaction_hash] == - "0000000000000000000000000000000000000000000000000000000000000000"); + BEAST_EXPECT(result[jss::ledger][jss::ledger_hash] == hash1); + BEAST_EXPECT(result[jss::ledger][jss::parent_hash] == zerohash); + BEAST_EXPECT(result[jss::ledger][jss::account_hash] == accounthash1); + BEAST_EXPECT(result[jss::ledger][jss::transaction_hash] == zerohash); } void diff --git a/src/test/rpc/ReportingETL_test.cpp b/src/test/rpc/ReportingETL_test.cpp index d8e6fc684fd..77284dd776d 100644 --- a/src/test/rpc/ReportingETL_test.cpp +++ b/src/test/rpc/ReportingETL_test.cpp @@ -532,7 +532,7 @@ class ReportingETL_test : public beast::unit_test::suite BEAST_EXPECT(status.ok()); BEAST_EXPECT( - reply.ledger_objects().objects_size() == num_accounts + 3); + reply.ledger_objects().objects_size() == num_accounts + 4); BEAST_EXPECT(reply.marker().size() == 0); auto ledger = env.closed(); size_t idx = 0; From f7a58449692889462694359a82bed4efb1ac9f44 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 23 Feb 2023 17:08:31 -0800 Subject: [PATCH 7/8] Drop the hidden fee config settings in favor of [voting] --- cfg/rippled-example.cfg | 4 ++-- cfg/rippled-reporting.cfg | 4 ++-- src/ripple/app/ledger/Ledger.cpp | 19 ++++++++------- src/ripple/app/misc/FeeVote.h | 24 ++----------------- src/ripple/app/misc/FeeVoteImpl.cpp | 28 ++++------------------ src/ripple/core/Config.h | 27 ++++++++++++++++++--- src/ripple/core/ConfigSections.h | 2 -- src/ripple/core/impl/Config.cpp | 32 +++++++++++++++++++------ src/ripple/rpc/impl/TransactionSign.cpp | 2 +- src/test/app/FeeVote_test.cpp | 2 +- src/test/app/LoadFeeTrack_test.cpp | 6 ++--- src/test/jtx/impl/envconfig.cpp | 6 ++--- 12 files changed, 77 insertions(+), 79 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index 02ef02e920f..0a669313066 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -1441,7 +1441,7 @@ # default. Don't change this without understanding the consequences. # # Example: -# account_reserve = 20000000 # 20 XRP +# account_reserve = 10000000 # 10 XRP # # owner_reserve = # @@ -1453,7 +1453,7 @@ # default. Don't change this without understanding the consequences. # # Example: -# owner_reserve = 5000000 # 5 XRP +# owner_reserve = 2000000 # 2 XRP # #------------------------------------------------------------------------------- # diff --git a/cfg/rippled-reporting.cfg b/cfg/rippled-reporting.cfg index 7e69d76f4f5..dbafdd497fa 100644 --- a/cfg/rippled-reporting.cfg +++ b/cfg/rippled-reporting.cfg @@ -1401,7 +1401,7 @@ # default. Don't change this without understanding the consequences. # # Example: -# account_reserve = 20000000 # 20 XRP +# account_reserve = 10000000 # 10 XRP # # owner_reserve = # @@ -1413,7 +1413,7 @@ # default. Don't change this without understanding the consequences. # # Example: -# owner_reserve = 5000000 # 5 XRP +# owner_reserve = 2000000 # 2 XRP # #------------------------------------------------------------------------------- # diff --git a/src/ripple/app/ledger/Ledger.cpp b/src/ripple/app/ledger/Ledger.cpp index 1e213d1f499..b2658c9c2f3 100644 --- a/src/ripple/app/ledger/Ledger.cpp +++ b/src/ripple/app/ledger/Ledger.cpp @@ -215,19 +215,20 @@ Ledger::Ledger( if (std::find(amendments.begin(), amendments.end(), featureXRPFees) != amendments.end()) { - sle->at(sfBaseFeeDrops) = config.FEE_DEFAULT; - sle->at(sfReserveBaseDrops) = config.FEE_ACCOUNT_RESERVE; - sle->at(sfReserveIncrementDrops) = config.FEE_OWNER_RESERVE; + sle->at(sfBaseFeeDrops) = config.FEES.reference_fee; + sle->at(sfReserveBaseDrops) = config.FEES.account_reserve; + sle->at(sfReserveIncrementDrops) = config.FEES.owner_reserve; } else { - if (auto const f = config.FEE_DEFAULT.dropsAs()) + if (auto const f = + config.FEES.reference_fee.dropsAs()) sle->at(sfBaseFee) = *f; if (auto const f = - config.FEE_ACCOUNT_RESERVE.dropsAs()) + config.FEES.account_reserve.dropsAs()) sle->at(sfReserveBase) = *f; if (auto const f = - config.FEE_OWNER_RESERVE.dropsAs()) + config.FEES.owner_reserve.dropsAs()) sle->at(sfReserveIncrement) = *f; sle->at(sfReferenceFeeUnits) = Config::FEE_UNITS_DEPRECATED; } @@ -694,11 +695,11 @@ Ledger::defaultFees(Config const& config) { assert(fees_.base == 0 && fees_.reserve == 0 && fees_.increment == 0); if (fees_.base == 0) - fees_.base = config.FEE_DEFAULT; + fees_.base = config.FEES.reference_fee; if (fees_.reserve == 0) - fees_.reserve = config.FEE_ACCOUNT_RESERVE; + fees_.reserve = config.FEES.account_reserve; if (fees_.increment == 0) - fees_.increment = config.FEE_OWNER_RESERVE; + fees_.increment = config.FEES.owner_reserve; } std::shared_ptr diff --git a/src/ripple/app/misc/FeeVote.h b/src/ripple/app/misc/FeeVote.h index 4fff64f7de3..a90f82efb35 100644 --- a/src/ripple/app/misc/FeeVote.h +++ b/src/ripple/app/misc/FeeVote.h @@ -32,23 +32,6 @@ namespace ripple { class FeeVote { public: - /** Fee schedule to vote for. - During voting ledgers, the FeeVote logic will try to move towards - these values when injecting fee-setting transactions. - A default-constructed Setup contains recommended values. - */ - struct Setup - { - /** The cost of a reference transaction in drops. */ - XRPAmount reference_fee{10}; - - /** The account reserve requirement in drops. */ - XRPAmount account_reserve{10 * DROPS_PER_XRP}; - - /** The per-owned item reserve requirement in drops. */ - XRPAmount owner_reserve{2 * DROPS_PER_XRP}; - }; - virtual ~FeeVote() = default; /** Add local fee preference to validation. @@ -74,16 +57,13 @@ class FeeVote std::shared_ptr const& initialPosition) = 0; }; -/** Build FeeVote::Setup from a config section. */ -FeeVote::Setup -setup_FeeVote(Section const& section); - +struct FeeSetup; /** Create an instance of the FeeVote logic. @param setup The fee schedule to vote for. @param journal Where to log. */ std::unique_ptr -make_FeeVote(FeeVote::Setup const& setup, beast::Journal journal); +make_FeeVote(FeeSetup const& setup, beast::Journal journal); } // namespace ripple diff --git a/src/ripple/app/misc/FeeVoteImpl.cpp b/src/ripple/app/misc/FeeVoteImpl.cpp index 73d98fbd58a..048f5a3fc61 100644 --- a/src/ripple/app/misc/FeeVoteImpl.cpp +++ b/src/ripple/app/misc/FeeVoteImpl.cpp @@ -93,11 +93,11 @@ VotableValue::getVotes() const -> std::pair class FeeVoteImpl : public FeeVote { private: - Setup target_; + FeeSetup target_; beast::Journal const journal_; public: - FeeVoteImpl(Setup const& setup, beast::Journal journal); + FeeVoteImpl(FeeSetup const& setup, beast::Journal journal); void doValidation(Fees const& lastFees, Rules const& rules, STValidation& val) @@ -112,7 +112,7 @@ class FeeVoteImpl : public FeeVote //-------------------------------------------------------------------------- -FeeVoteImpl::FeeVoteImpl(Setup const& setup, beast::Journal journal) +FeeVoteImpl::FeeVoteImpl(FeeSetup const& setup, beast::Journal journal) : target_(setup), journal_(journal) { } @@ -335,28 +335,8 @@ FeeVoteImpl::doVoting( //------------------------------------------------------------------------------ -FeeVote::Setup -setup_FeeVote(Section const& section) -{ - FeeVote::Setup setup; - { - std::uint64_t temp; - if (set(temp, "reference_fee", section) && - temp <= std::numeric_limits::max()) - setup.reference_fee = temp; - } - { - std::uint32_t temp; - if (set(temp, "account_reserve", section)) - setup.account_reserve = temp; - if (set(temp, "owner_reserve", section)) - setup.owner_reserve = temp; - } - return setup; -} - std::unique_ptr -make_FeeVote(FeeVote::Setup const& setup, beast::Journal journal) +make_FeeVote(FeeSetup const& setup, beast::Journal journal) { return std::make_unique(setup, journal); } diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index 0db0290f505..e805faad989 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -61,6 +61,26 @@ enum class SizedItem : std::size_t { accountIdCacheSize, }; +/** Fee schedule for startup / standalone, and to vote for. +During voting ledgers, the FeeVote logic will try to move towards +these values when injecting fee-setting transactions. +A default-constructed Setup contains recommended values. +*/ +struct FeeSetup +{ + /** The cost of a reference transaction in drops. */ + XRPAmount reference_fee{10}; + + /** The account reserve requirement in drops. */ + XRPAmount account_reserve{10 * DROPS_PER_XRP}; + + /** The per-owned item reserve requirement in drops. */ + XRPAmount owner_reserve{2 * DROPS_PER_XRP}; + + /* (Remember to update the example cfg files when changing any of these + * values.) */ +}; + // This entire derived class is deprecated. // For new config information use the style implied // in the base class. For existing config information @@ -185,9 +205,7 @@ class Config : public BasicConfig std::optional VALIDATION_QUORUM; // validations to consider ledger authoritative - XRPAmount FEE_DEFAULT{10}; - XRPAmount FEE_ACCOUNT_RESERVE{10 * DROPS_PER_XRP}; - XRPAmount FEE_OWNER_RESERVE{2 * DROPS_PER_XRP}; + FeeSetup FEES; // Node storage configuration std::uint32_t LEDGER_HISTORY = 256; @@ -366,6 +384,9 @@ class Config : public BasicConfig const; }; +FeeSetup +setup_FeeVote(Section const& section); + } // namespace ripple #endif diff --git a/src/ripple/core/ConfigSections.h b/src/ripple/core/ConfigSections.h index ba0f209c0e9..6fd59680d8b 100644 --- a/src/ripple/core/ConfigSections.h +++ b/src/ripple/core/ConfigSections.h @@ -54,8 +54,6 @@ struct ConfigSection #define SECTION_DEBUG_LOGFILE "debug_logfile" #define SECTION_ELB_SUPPORT "elb_support" #define SECTION_FEE_DEFAULT "fee_default" -#define SECTION_FEE_ACCOUNT_RESERVE "fee_account_reserve" -#define SECTION_FEE_OWNER_RESERVE "fee_owner_reserve" #define SECTION_FETCH_DEPTH "fetch_depth" #define SECTION_HISTORICAL_SHARD_PATHS "historical_shard_paths" #define SECTION_INSIGHT "insight" diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index c2cfb14d21d..4946881ab45 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -588,14 +588,12 @@ Config::loadFromString(std::string const& fileContents) if (getSingleSection(secConfig, SECTION_NETWORK_QUORUM, strTemp, j_)) NETWORK_QUORUM = beast::lexicalCastThrow(strTemp); - if (getSingleSection(secConfig, SECTION_FEE_ACCOUNT_RESERVE, strTemp, j_)) - FEE_ACCOUNT_RESERVE = beast::lexicalCastThrow(strTemp); - - if (getSingleSection(secConfig, SECTION_FEE_OWNER_RESERVE, strTemp, j_)) - FEE_OWNER_RESERVE = beast::lexicalCastThrow(strTemp); - + FEES = setup_FeeVote(section("voting")); + /* [fee_default] is documented in the example config files as useful for + * things like offline transaction signing. Until that's completely + * deprecated, allow it to override the [voting] section. */ if (getSingleSection(secConfig, SECTION_FEE_DEFAULT, strTemp, j_)) - FEE_DEFAULT = beast::lexicalCastThrow(strTemp); + FEES.reference_fee = beast::lexicalCastThrow(strTemp); if (getSingleSection(secConfig, SECTION_LEDGER_HISTORY, strTemp, j_)) { @@ -994,4 +992,24 @@ Config::getValueFor(SizedItem item, std::optional node) const return sizedItems.at(index).second.at(node.value_or(NODE_SIZE)); } +FeeSetup +setup_FeeVote(Section const& section) +{ + FeeSetup setup; + { + std::uint64_t temp; + if (set(temp, "reference_fee", section) && + temp <= std::numeric_limits::max()) + setup.reference_fee = temp; + } + { + std::uint32_t temp; + if (set(temp, "account_reserve", section)) + setup.account_reserve = temp; + if (set(temp, "owner_reserve", section)) + setup.owner_reserve = temp; + } + return setup; +} + } // namespace ripple diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 4cf372e6b63..c903c26f8e3 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -720,7 +720,7 @@ checkFee( } } - XRPAmount const feeDefault = config.FEE_DEFAULT; + XRPAmount const feeDefault = config.FEES.reference_fee; auto ledger = app.openLedger().current(); // Administrative and identified endpoints are exempt from local fees. diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 90dd8fa3dfc..ad38aefb20a 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -29,7 +29,7 @@ class FeeVote_test : public beast::unit_test::suite void testSetup() { - FeeVote::Setup const defaultSetup; + FeeSetup const defaultSetup; { // defaults Section config; diff --git a/src/test/app/LoadFeeTrack_test.cpp b/src/test/app/LoadFeeTrack_test.cpp index cc0b1c19529..f8e73cebd16 100644 --- a/src/test/app/LoadFeeTrack_test.cpp +++ b/src/test/app/LoadFeeTrack_test.cpp @@ -35,7 +35,7 @@ class LoadFeeTrack_test : public beast::unit_test::suite { Fees const fees = [&]() { Fees f; - f.base = d.FEE_DEFAULT; + f.base = d.FEES.reference_fee; f.reserve = 200 * DROPS_PER_XRP; f.increment = 50 * DROPS_PER_XRP; return f; @@ -52,7 +52,7 @@ class LoadFeeTrack_test : public beast::unit_test::suite { Fees const fees = [&]() { Fees f; - f.base = d.FEE_DEFAULT * 10; + f.base = d.FEES.reference_fee * 10; f.reserve = 200 * DROPS_PER_XRP; f.increment = 50 * DROPS_PER_XRP; return f; @@ -69,7 +69,7 @@ class LoadFeeTrack_test : public beast::unit_test::suite { Fees const fees = [&]() { Fees f; - f.base = d.FEE_DEFAULT; + f.base = d.FEES.reference_fee; f.reserve = 200 * DROPS_PER_XRP; f.increment = 50 * DROPS_PER_XRP; return f; diff --git a/src/test/jtx/impl/envconfig.cpp b/src/test/jtx/impl/envconfig.cpp index 598b8a96f04..77c43f5e40a 100644 --- a/src/test/jtx/impl/envconfig.cpp +++ b/src/test/jtx/impl/envconfig.cpp @@ -45,9 +45,9 @@ setupConfigForUnitTests(Config& cfg) using namespace jtx; // Default fees to old values, so tests don't have to worry about changes in // Config.h - cfg.FEE_DEFAULT = 10; - cfg.FEE_ACCOUNT_RESERVE = XRP(200).value().xrp().drops(); - cfg.FEE_OWNER_RESERVE = XRP(50).value().xrp().drops(); + cfg.FEES.reference_fee = 10; + cfg.FEES.account_reserve = XRP(200).value().xrp().drops(); + cfg.FEES.owner_reserve = XRP(50).value().xrp().drops(); cfg.overwrite(ConfigSection::nodeDatabase(), "type", "memory"); cfg.overwrite(ConfigSection::nodeDatabase(), "path", "main"); From f2742cf549dd82016297c518940dc326b5a94276 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 24 Mar 2023 12:42:47 -0400 Subject: [PATCH 8/8] [FOLD] Review feedback from @gregtatcam: ** Do not include this message in the squashed commit message * Move one of the tests and update a comment. --- src/test/app/RCLValidations_test.cpp | 2 +- src/test/app/TxQ_test.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/app/RCLValidations_test.cpp b/src/test/app/RCLValidations_test.cpp index ffb468c1cf1..0380795a8ae 100644 --- a/src/test/app/RCLValidations_test.cpp +++ b/src/test/app/RCLValidations_test.cpp @@ -106,9 +106,9 @@ class RCLValidations_test : public beast::unit_test::suite *prev, env.app().timeKeeper().closeTime()); // Force a different hash on the first iteration next->updateSkipList(); + BEAST_EXPECT(next->read(keylet::fees())); if (forceHash) { - BEAST_EXPECT(next->read(keylet::fees())); next->setImmutable(); forceHash = false; } diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 9acbd5493b4..8bf359e101c 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -2939,9 +2939,9 @@ class TxQ1_test : public beast::unit_test::suite // Verify that nothing can be added now that the gap is filled. env(noop(alice), seq(aliceSeq + 20), ter(telCAN_NOT_QUEUE_FULL)); - // Close ledger 6. That removes 6 of alice's transactions, - // but alice adds one more transaction at seq(aliceSeq + 20) so - // we only see a reduction by 5. + // Close ledger 6. That removes some of alice's transactions, + // but alice adds some more transaction(s) so expectedCount + // may not reduce to 8. env.close(); checkMetrics(__LINE__, env, 9, 50, 6, 5, 256); BEAST_EXPECT(env.seq(alice) == aliceSeq + 13);