Skip to content

Commit

Permalink
[FOLD] Review feedback from Nik:
Browse files Browse the repository at this point in the history 
* Break featureXRPCheck logic into multiple blocks.
* Change default network reserves to match mainnet.
* Add a isLegalAmountSigned check
  • Loading branch information
@ximinez
ximinez committed Oct 17, 2022
1 parent 8474973 commit ac080f2
Show file tree
Hide file tree
Showing 6 changed files with 181 additions and 145 deletions.
4 changes: 2 additions & 2 deletions src/ripple/app/misc/FeeVote.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ class FeeVote
XRPAmount reference_fee{10};

/** The account reserve requirement in drops. */
XRPAmount account_reserve{20 * DROPS_PER_XRP};
XRPAmount account_reserve{10 * DROPS_PER_XRP};

/** The per-owned item reserve requirement in drops. */
XRPAmount owner_reserve{5 * DROPS_PER_XRP};
XRPAmount owner_reserve{2 * DROPS_PER_XRP};
};

virtual ~FeeVote() = default;
Expand Down
283 changes: 152 additions & 131 deletions src/ripple/app/misc/FeeVoteImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ class VotableValue
value_type const current_; // The current setting
value_type const target_; // The setting we want
std::map<value_type, int> voteMap_;
std::optional<value_type> vote_;

public:
VotableValue(value_type current, value_type target)
Expand All @@ -58,38 +57,19 @@ class VotableValue
addVote(current_);
}

void
setVotes();

value_type
getVotes() const;

template <class Dest>
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<value_type, bool>
getVotes() const;
};

auto
VotableValue::getVotes() const -> value_type
VotableValue::getVotes() const -> std::pair<value_type, bool>
{
if (vote_)
return *vote_;

value_type ourVote = current_;
int weight = 0;
for (auto const& [key, val] : voteMap_)
Expand All @@ -103,14 +83,7 @@ VotableValue::getVotes() const -> value_type
}
}

return ourVote;
}

template <class Dest>
Dest
VotableValue::getVotesAs() const
{
return getVotes().dropsAs<Dest>(current_);
return {ourVote, ourVote != current_};
}

} // namespace detail
Expand Down Expand Up @@ -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<std::uint64_t>())
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<std::uint32_t>())
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<std::uint32_t>())
v[sfReserveIncrement] = *f;
auto to32 = [](XRPAmount target) {
return target.dropsAs<std::uint32_t>();
};
auto to64 = [](XRPAmount target) {
return target.dropsAs<std::uint64_t>();
};
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);
}
}

Expand All @@ -207,89 +212,105 @@ FeeVoteImpl::doVoting(
lastClosedLedger->fees().increment, target_.owner_reserve);

auto const& rules = lastClosedLedger->rules();
auto doVote = [&rules](
std::shared_ptr<STValidation> 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<STValidation> 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<xrptype>::max() &&
isLegalAmount(XRPAmount{unsafe_cast<xrptype>(vote)}))
value.addVote(
XRPAmount{unsafe_cast<XRPAmount::value_type>(vote)});
if (!val->isTrusted())
continue;
doVote(val, baseFeeVote, sfBaseFeeXRP);
doVote(val, baseReserveVote, sfReserveBaseXRP);
doVote(val, incReserveVote, sfReserveIncrementXRP);
}
}
else
{
auto doVote = [](std::shared_ptr<STValidation> 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<xrptype>::max() &&
isLegalAmountSigned(XRPAmount{unsafe_cast<xrptype>(vote)}))
value.addVote(
XRPAmount{unsafe_cast<XRPAmount::value_type>(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();
auto const [baseFee, baseFeeChanged] = baseFeeVote.getVotes();
auto const [baseReserve, baseReserveChanged] = baseReserveVote.getVotes();
auto const [incReserve, incReserveChanged] = incReserveVote.getVotes();

auto const seq = lastClosedLedger->info().seq + 1;

// add transactions to our position
if ((baseFeeVote.voteChange()) || (baseReserveVote.voteChange()) ||
(incReserveVote.voteChange()))
if (baseFeeChanged || baseReserveChanged || incReserveChanged)
{
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<std::uint64_t>();
obj[sfReserveBase] =
baseReserveVote.getVotesAs<std::uint32_t>();
obj[sfReserveIncrement] =
incReserveVote.getVotesAs<std::uint32_t>();
obj[sfReferenceFeeUnits] = Config::FEE_UNITS_DEPRECATED;
}
});
JLOG(journal_.warn()) << "We are voting for a fee change: " << baseFee
<< "/" << baseReserve << "/" << incReserve;

STTx feeTx(ttFEE, [&](auto& obj) {
obj[sfAccount] = AccountID();
obj[sfLedgerSequence] = seq;
if (rules.enabled(featureXRPFees))
{
obj[sfBaseFeeXRP] = baseFee;
obj[sfReserveBaseXRP] = baseReserve;
obj[sfReserveIncrementXRP] = incReserve;
}
else
{
// Without the featureXRPFees amendment, these fields are
// required.
obj[sfBaseFee] =
baseFee.dropsAs<std::uint64_t>(baseFeeVote.current());
obj[sfReserveBase] = baseReserve.dropsAs<std::uint32_t>(
baseReserveVote.current());
obj[sfReserveIncrement] =
incReserve.dropsAs<std::uint32_t>(incReserveVote.current());
obj[sfReferenceFeeUnits] = Config::FEE_UNITS_DEPRECATED;
}
});

uint256 txID = feeTx.getTransactionID();

Expand Down
8 changes: 8 additions & 0 deletions src/ripple/protocol/SystemParameters.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
5 changes: 4 additions & 1 deletion src/test/app/AccountDelete_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(19)), 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.
Expand Down
Loading

0 comments on commit ac080f2

Please sign in to comment.