Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address elevated transaction fees. #4022

Closed
wants to merge 5 commits into from
Closed

Address elevated transaction fees. #4022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d6fdd4c
Logging & minor optimizations:
ximinez Dec 2, 2021
ef635e7
Consensus transaction recovery / deferral completely ignores the TxQ
ximinez Dec 7, 2021
fc6015b
Make transaction queue order deterministic:
ximinez Dec 8, 2021
010f0cf
Reduce TxQ logging severity in several places
ximinez Dec 9, 2021
aa20afa
Increase TxQ default minimum and target sizes
ximinez Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 2 additions & 13 deletions src/ripple/app/ledger/OpenLedger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ class OpenLedger
FwdRange const& txs,
OrderedTxs& retries,
ApplyFlags flags,
std::map<uint256, bool>& shouldRecover,
beast::Journal j);

enum Result { success, failure, retry };
Expand All @@ -200,7 +199,6 @@ class OpenLedger
std::shared_ptr<STTx const> const& tx,
bool retry,
ApplyFlags flags,
bool shouldRecover,
beast::Journal j);
};

Expand All @@ -215,7 +213,6 @@ OpenLedger::apply(
FwdRange const& txs,
OrderedTxs& retries,
ApplyFlags flags,
std::map<uint256, bool>& shouldRecover,
beast::Journal j)
{
for (auto iter = txs.begin(); iter != txs.end(); ++iter)
Expand All @@ -227,8 +224,7 @@ OpenLedger::apply(
auto const txId = tx->getTransactionID();
if (check.txExists(txId))
continue;
auto const result =
apply_one(app, view, tx, true, flags, shouldRecover[txId], j);
auto const result = apply_one(app, view, tx, true, flags, j);
if (result == Result::retry)
retries.insert(tx);
}
Expand All @@ -245,14 +241,7 @@ OpenLedger::apply(
auto iter = retries.begin();
while (iter != retries.end())
{
switch (apply_one(
app,
view,
iter->second,
retry,
flags,
shouldRecover[iter->second->getTransactionID()],
j))
switch (apply_one(app, view, iter->second, retry, flags, j))
{
case Result::success:
++changes;
Expand Down
10 changes: 10 additions & 0 deletions src/ripple/app/ledger/impl/LedgerMaster.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,6 +1080,16 @@ LedgerMaster::checkAccept(std::shared_ptr<Ledger const> const& ledger)
if (!fees.empty())
{
std::sort(fees.begin(), fees.end());
if (auto stream = m_journal.debug())
{
std::stringstream s;
s << "Received fees from validations: (" << fees.size() << ") ";
for (auto const fee1 : fees)
{
s << " " << fee1;
}
stream << s.str();
}
fee = fees[fees.size() / 2]; // median
}
else
Expand Down
33 changes: 3 additions & 30 deletions src/ripple/app/ledger/impl/OpenLedger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,11 @@ OpenLedger::accept(
{
JLOG(j_.trace()) << "accept ledger " << ledger->seq() << " " << suffix;
auto next = create(rules, ledger);
std::map<uint256, bool> shouldRecover;
if (retriesFirst)
{
for (auto const& tx : retries)
{
auto const txID = tx.second->getTransactionID();
shouldRecover[txID] = app.getHashRouter().shouldRecover(txID);
}
// Handle disputed tx, outside lock
using empty = std::vector<std::shared_ptr<STTx const>>;
apply(app, *next, *ledger, empty{}, retries, flags, shouldRecover, j_);
apply(app, *next, *ledger, empty{}, retries, flags, j_);
}
// Block calls to modify, otherwise
// new tx going into the open ledger
Expand All @@ -100,17 +94,6 @@ OpenLedger::accept(
// Apply tx from the current open view
if (!current_->txs.empty())
{
for (auto const& tx : current_->txs)
{
auto const txID = tx.first->getTransactionID();
auto iter = shouldRecover.lower_bound(txID);
if (iter != shouldRecover.end() && iter->first == txID)
// already had a chance via disputes
iter->second = false;
else
shouldRecover.emplace_hint(
iter, txID, app.getHashRouter().shouldRecover(txID));
}
apply(
app,
*next,
Expand All @@ -124,7 +107,6 @@ OpenLedger::accept(
}),
retries,
flags,
shouldRecover,
j_);
}
// Call the modifier
Expand Down Expand Up @@ -179,21 +161,12 @@ OpenLedger::apply_one(
std::shared_ptr<STTx const> const& tx,
bool retry,
ApplyFlags flags,
bool shouldRecover,
beast::Journal j) -> Result
{
if (retry)
flags = flags | tapRETRY;
auto const result = [&] {
auto const queueResult =
app.getTxQ().apply(app, view, tx, flags | tapPREFER_QUEUE, j);
// If the transaction can't get into the queue for intrinsic
// reasons, and it can still be recovered, try to put it
// directly into the open ledger, else drop it.
if (queueResult.first == telCAN_NOT_QUEUE && shouldRecover)
return ripple::apply(app, view, *tx, flags, j);
return queueResult;
}();
// If it's in anybody's proposed set, try to keep it in the ledger
auto const result = ripple::apply(app, view, *tx, flags, j);
if (result.second || result.first == terQUEUED)
return Result::success;
if (isTefFailure(result.first) || isTemMalformed(result.first) ||
Expand Down
3 changes: 1 addition & 2 deletions src/ripple/app/main/Application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,8 +443,7 @@ class ApplicationImp : public Application, public BasicApp

, hashRouter_(std::make_unique<HashRouter>(
stopwatch(),
HashRouter::getDefaultHoldTime(),
HashRouter::getDefaultRecoverLimit()))
HashRouter::getDefaultHoldTime()))

, mValidations(
ValidationParms(),
Expand Down
10 changes: 0 additions & 10 deletions src/ripple/app/misc/HashRouter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,4 @@ HashRouter::shouldRelay(uint256 const& key)
return s.releasePeerSet();
}

bool
HashRouter::shouldRecover(uint256 const& key)
{
std::lock_guard lock(mutex_);

auto& s = emplace(key).first;

return s.shouldRecover(recoverLimit_);
}

} // namespace ripple
41 changes: 2 additions & 39 deletions src/ripple/app/misc/HashRouter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,20 +116,6 @@ class HashRouter
return true;
}

/** Determines if this item should be recovered from the open ledger.

Counts the number of times the item has been recovered.
Every `limit` times the function is called, return false.
Else return true.

@note The limit must be > 0
*/
bool
shouldRecover(std::uint32_t limit)
{
return ++recoveries_ % limit != 0;
}

bool
shouldProcess(Stopwatch::time_point now, std::chrono::seconds interval)
{
Expand All @@ -146,7 +132,6 @@ class HashRouter
// than one flag needs to expire independently.
std::optional<Stopwatch::time_point> relayed_;
std::optional<Stopwatch::time_point> processed_;
std::uint32_t recoveries_ = 0;
};

public:
Expand All @@ -158,19 +143,8 @@ class HashRouter
return 300s;
}

static inline std::uint32_t
getDefaultRecoverLimit()
{
return 1;
}

HashRouter(
Stopwatch& clock,
std::chrono::seconds entryHoldTimeInSeconds,
std::uint32_t recoverLimit)
: suppressionMap_(clock)
, holdTime_(entryHoldTimeInSeconds)
, recoverLimit_(recoverLimit + 1u)
HashRouter(Stopwatch& clock, std::chrono::seconds entryHoldTimeInSeconds)
: suppressionMap_(clock), holdTime_(entryHoldTimeInSeconds)
{
}

Expand Down Expand Up @@ -231,15 +205,6 @@ class HashRouter
std::optional<std::set<PeerShortID>>
shouldRelay(uint256 const& key);

/** Determines whether the hashed item should be recovered
from the open ledger into the next open ledger or the transaction
queue.

@return `bool` indicates whether the item should be recovered
*/
bool
shouldRecover(uint256 const& key);

private:
// pair.second indicates whether the entry was created
std::pair<Entry&, bool>
Expand All @@ -256,8 +221,6 @@ class HashRouter
suppressionMap_;

std::chrono::seconds const holdTime_;

std::uint32_t const recoverLimit_;
};

} // namespace ripple
Expand Down
3 changes: 3 additions & 0 deletions src/ripple/app/misc/LoadFeeTrack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#define RIPPLE_CORE_LOADFEETRACK_H_INCLUDED

#include <ripple/basics/FeeUnits.h>
#include <ripple/basics/Log.h>
#include <ripple/beast/utility/Journal.h>
#include <ripple/json/json_value.h>
#include <algorithm>
Expand Down Expand Up @@ -58,6 +59,7 @@ class LoadFeeTrack final
void
setRemoteFee(std::uint32_t f)
{
JLOG(j_.trace()) << "setRemoteFee: " << f;
std::lock_guard sl(lock_);
remoteTxnLoadFee_ = f;
}
Expand Down Expand Up @@ -110,6 +112,7 @@ class LoadFeeTrack final
void
setClusterFee(std::uint32_t fee)
{
JLOG(j_.trace()) << "setClusterFee: " << fee;
std::lock_guard sl(lock_);
clusterTxnLoadFee_ = fee;
}
Expand Down
29 changes: 21 additions & 8 deletions src/ripple/app/misc/TxQ.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ class TxQ
FeeLevel64 minimumEscalationMultiplier = baseLevel * 500;
/// Minimum number of transactions to allow into the ledger
/// before escalation, regardless of the prior ledger's size.
std::uint32_t minimumTxnInLedger = 5;
std::uint32_t minimumTxnInLedger = 32;
/// Like @ref minimumTxnInLedger for standalone mode.
/// Primarily so that tests don't need to worry about queuing.
std::uint32_t minimumTxnInLedgerSA = 1000;
/// Number of transactions per ledger that fee escalation "works
/// towards".
std::uint32_t targetTxnInLedger = 50;
std::uint32_t targetTxnInLedger = 256;
/** Optional maximum allowed value of transactions per ledger before
fee escalation kicks in. By default, the maximum is an emergent
property of network, validator, and consensus performance. This
Expand Down Expand Up @@ -613,17 +613,30 @@ class TxQ
}
};

/// Used for sorting @ref MaybeTx by `feeLevel`
class GreaterFee
/// Used for sorting @ref MaybeTx
class OrderCandidates
{
public:
/// Default constructor
explicit GreaterFee() = default;

/// Is the fee level of `lhs` greater than the fee level of `rhs`?
explicit OrderCandidates() = default;

/** Sort @ref MaybeTx by `feeLevel` descending, then by
* transaction ID ascending
*
* The transaction queue is ordered such that transactions
* paying a higher fee are in front of transactions paying
* a lower fee, giving them an opportunity to be processed into
* the open ledger first. Within transactions paying the same
* fee, order by the arbitrary but consistent transaction ID.
* This allows validators to build similar queues in the same
* order, and thus have more similar initial proposals.
*
*/
bool
operator()(const MaybeTx& lhs, const MaybeTx& rhs) const
{
if (lhs.feeLevel == rhs.feeLevel)
return lhs.txID < rhs.txID;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was initially surprised to see the comparison operator direction swapped between txID and feeLevel. Feels like it might be worth a comment that we sort with the smallest txID at the front, and the largest feeLevel at the front.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left such a comment at the top of the class. I can move it to the function, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LIke @mDuo13 said in the comments, this is mineable, but if the variation of fees is random enough, shouldn't matter. If you wanted to be fancy, you could use the ledger sequence, or even better, the previous ledger hash, as a salt for a non-cryptographic hash of the transaction id, and order by that

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please consider using a similar method to transaction processing ordering also for queue sorting. It might not be an issue now, but it seems like it could be used to gain an unfair advantage. Even worse - the unfair advantage might be used to cause increasing load on the network, since I don't see much downside to push every incrementally "better" transaction immediately out instead of waiting for a "winner" and just let the remaining ones become invalid (e.g. through using the same sequence number?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe could reuse some of the logic and thinking from CanonicalTxSet, in a reusable class?
https://github.com/ripple/rippled/blob/9d89d4c1885e13e3c0aa7cdb4a389c6fbfd3790a/src/ripple/app/misc/CanonicalTXSet.h#L173-L174
https://github.com/ripple/rippled/blob/9d89d4c1885e13e3c0aa7cdb4a389c6fbfd3790a/src/ripple/app/misc/CanonicalTXSet.h#L128-L133
https://github.com/ripple/rippled/blob/9d89d4c1885e13e3c0aa7cdb4a389c6fbfd3790a/src/ripple/app/misc/CanonicalTXSet.cpp#L24-L49

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding mining for a low txID, I agree with @MarkusTeufelberger and @donovanhide that this is an issue. But I think it's not a top priority, so I don't personally believe it needs to be addressed in this pull particular request (which I think we'd hope to see on the network fairly soon).

My recollection of boost::intrusive::multiset (the container type for byFee_) is foggy. But my best guess is that an efficient way to randomize access in the container, while still staying within a fixed fee range, would involve reorganizing the container. That may take a bit of time. I encourage you to take that time (soon) so these legitimate concerns will be addressed. But, again, I don't think that change needs to be in this particular pull request.

Copy link
Contributor

@nbougalis nbougalis Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we were discussing this fix, @thejohnfreeman suggested using the parent ledger's hash, just as you did @donovanhide. I think it makes sense and it's solution I would prefer too.

But I'm of two minds here: one the one hand, I agree that adding this sort of "anti-mining" support makes sense and it's fairly easy; on the other hand, given that we're all moving fast to try and get this fix done on an expedited basis, I'd rather minimize the number of changes.

So I'm fine with leaving this as a "TODO" but if others feels it's important to have it added now and @ximinez feels he can add it with minimal risk, I'm supportive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, @mDuo13 , @MarkusTeufelberger , and @donovanhide , thank you very much for the feedback! I've already got anti-mining on the radar, and as @scottschurr mentioned, we've got some ideas for it.

However, to emphasize what @nbougalis said, the priority for this PR is to keep the changes as small, simple, and easy to reason about as possible.

I also think that the risk right now even just using the unmodified TX id is small for a few reasons.

  • If you want to jump ahead of someone specifically for some reason, it's probably a lot cheaper to pay one drop more than it is to pre-mine a TX with a predetermined fee.
  • Once your transaction is out of the queue, now your transaction has to contend with the sorted transaction set, which does use that parent ledger hash.
  • The only meaningful attack that I can figure out is to get your transaction into the open ledger / proposed set and prevent another transaction from getting in. That's going to be really hard to time perfectly over and above mining the hash. Partly because you won't know for sure how many other txs are in the ledger or the queue. And you have to do it on a majority of validators.

So yes, there's a risk, and I'd like to address it in another minor release soon, but it seems like it's a very small risk.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this issue should not be addressed in this PR. However, just wanted to highlight the nuances of one particular fictional example motive for mining:
I'm a market maker A who has competitor market maker B. I know he or she doesn't appear to monitor their bot very closely. They do not use a dynamic fee, just 12 drops. I want to stop B's offers getting into the ledger so people consume my offers instead. I pay one more drop for my offers and stuff the queue with 12 drop pointless AccountSet transactions with mined Transaction Ids that are likely to be before most, if not all of B's offers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@donovanhide Yeah, I can see how that might be a viable attack. It's still an open question whether it would still be cheaper to just have those noop transactions pay 13 drops vs. pre-mining.

return lhs.feeLevel > rhs.feeLevel;
}
};
Expand Down Expand Up @@ -722,7 +735,7 @@ class TxQ
&MaybeTx::byFeeListHook>;

using FeeMultiSet = boost::intrusive::
multiset<MaybeTx, FeeHook, boost::intrusive::compare<GreaterFee>>;
multiset<MaybeTx, FeeHook, boost::intrusive::compare<OrderCandidates>>;

using AccountMap = std::map<AccountID, TxQAccount>;

Expand Down
Loading