Address elevated transaction fees. #4022

ximinez · 2021-12-09T23:31:24Z

High Level Overview of Change

Two main changes:

Revert a subset of the changes from ximinez@62127d7 which would defer transactions from one ledger to the next into the transaction queue (TxQ). A transaction that is in the open ledger but doesn't get validated should stay in the open ledger so that it can be proposed in the next transaction set.
Order the transaction queue deterministically, first be fee level descending, then by transaction ID / hash ascending. This will improve the overlap of initial proposals from different ledgers because they will be more likely to be pulling the same set of transactions out of their queues into their open ledgers.

Also added and changed some logging and made some minor optimizations.

Context of Change

Between about 14:00 and 17:00 UTC on December 1st, transaction submission rates went up 10-fold. This caused some unexpected side effects. The most observable effect was that transaction queues were full on most nodes in the network, but that was just a symptom. Most of those transactions had a 12 drop fee, but very few of them were getting validated. Transactions paying more would typically land in the open ledger or towards the front of the queues, and were thus being validated. This caused much consternation. The root cause of the problem was that transactions in the proposals for one ledger were being deferred to the next ledger, but the deferral process dropped them entirely because they could not be put back into the now-full queue. tl;dr they should never have attempted to be put back into the queue, but this flaw was not apparent until the current conditions presented themselves.

The problem was further exacerbated by the TxQ policy of ordering transactions with the same fee level by arrival time - basically first come, first served. Because the rate at which transactions are being submitted is so high, most nodes saw most transactions in a drastically different order. Validators would proposed some of these transactions, but very few nodes proposed the same set of transactions. Using a definitive ordering will help increase the overlap between those initial proposals between validators, and the deferral fix will ensure that any differences will be resolved in the next ledger.

Once a majority of UNL validators have this fix, the number of transactions validated should increase dramatically until either they catch up to the backlog, or the new load level becomes the "new normal". Once either of those things happens, the fee escalation logic should have a more accurate view of what the network is capable of and lower baseline fees accordingly.

Note to operators: until a majority of UNL validators are updated, node operators running this change can expect to see 500-1500 transactions in the open ledger, and the transaction queue near or at capacity. This will show a correspondingly large open ledger fee.

Type of Change

[X ] Bug fix (non-breaking change which fixes an issue)
[X ] Refactor (non-breaking change that only restructures code)

* Log load fee values (at debug) received from validations. * Log remote and cluster fee values (at trace) when changed. * Refactor JobQueue::isOverloaded to return sooner if overloaded. * Refactor Transactor::checkFee to only compute fee if ledger is open.

thejohnfreeman

I love that this fix works by removing so much code!

src/ripple/core/impl/JobQueue.cpp

mDuo13 · 2021-12-10T02:13:46Z

A question on the intended design of deterministic queuing: if the queue is full, can a new transaction kick a previous transaction with the same fee, but lower hash, out of the queue? If so, "hash mining" is a theoretical way to game the system, but likely irrelevant since paying even 1 drop more also prioritizes the transaction, and for the foreseeable future 1 drop will often be less valuable than the effort to mine a lower hash.

If a same-fee-lower-hash transaction can't evict a previous transaction, then isn't it still possible for validators to end up with very different—potentially even non-overlapping—queues? Suppose that, for each validator, there is a spammer sending numerous minimum-fee transactions from a node that is near the validator in the network topology. Each spammer could claim all the minimum-fee slots in their nearest validator's queue, resulting in no overlap possible between validators' choices of minimum-fee transactions.

It would be preferable to actually validate as many of each spammer's transactions as possible, to burn the fees, and I'd argue this is more important than preventing someone mining hashes to save 0.000001 XRP, so I think the answer should be "Yes, a same-fee-lower-hash transaction evicts another transaction from the queue if it is full."

donovanhide · 2021-12-10T07:47:52Z

src/ripple/app/misc/TxQ.h

        bool
        operator()(const MaybeTx& lhs, const MaybeTx& rhs) const
        {
+            if (lhs.feeLevel == rhs.feeLevel)
+                return lhs.txID < rhs.txID;


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

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?).

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

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.

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.

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.

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.

@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.

MarkusTeufelberger · 2021-12-10T08:39:52Z

I think I recall somewhere that "fee level" is not the same as "fee amount"? If so, it might be worth considering sorting by the actual amount, so AccountDelete transactions (which are mostly beneficial in the longer run for validators - fewer objects in the ledger is better!) get heavily prioritized. This would also solve #4016

mDuo13 · 2021-12-10T21:11:08Z

I think I recall somewhere that "fee level" is not the same as "fee amount"? If so, it might be worth considering sorting by the actual amount, so AccountDelete transactions (which are mostly beneficial in the longer run for validators - fewer objects in the ledger is better!) get heavily prioritized. This would also solve #4016

Correct, a "fee level" is, essentially, "what percentage of the minimum for this particular transaction are you paying?" They were invented for the queue; otherwise, transactions that involve extra work—like escrows with fulfillments, or multi-signed transactions—wouldn't need to pay elevated costs to kick out other, simpler transactions that were paying their respective minimums.

It's only with AccountDelete that it gets weird because the "1 owner reserve" amount is so out of scale with the other transaction types' costs.

scottschurr

Looks good as far as I can see. I left a couple of notes about where I was surprised, so a couple more comments might be called for. But I leave those to your discretion.

I am hoping to see another pull request from you (soon) to address concerns raised by @mDuo13, @donovanhide, and @MarkusTeufelberger. But I don't think those concerns need to be handled in this particular pull request.

scottschurr · 2021-12-10T00:56:30Z

src/ripple/app/misc/TxQ.h

        bool
        operator()(const MaybeTx& lhs, const MaybeTx& rhs) const
        {
+            if (lhs.feeLevel == rhs.feeLevel)
+                return lhs.txID < rhs.txID;


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.

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

scottschurr · 2021-12-10T01:17:25Z

src/ripple/app/misc/impl/TxQ.cpp

-            JLOG(j_.warn())
+            ++lastRIter;
+        }
+        if (lastRIter == byFee_.rend())


It looks like, for this condition to be met, the entire contents of byFee_ would have to contain transactions only from this account. And byFee_ is at capacity at this point. But the TxQ limits the number of transactions from an individual account to maximumTxnPerAccount, which is presumably smaller than the total byFee_ size. That said, I'm glad you're checking lastRIter == byFee_.rend().

Maybe it's worth a comment that says we never expect this to happen?

Good point. It does happen in a unit test where the limits are much smaller, and I suppose it would be possible if someone set their config to some unusual values.

scottschurr · 2021-12-10T20:38:40Z

src/ripple/app/misc/TxQ.h

        bool
        operator()(const MaybeTx& lhs, const MaybeTx& rhs) const
        {
+            if (lhs.feeLevel == rhs.feeLevel)
+                return lhs.txID < rhs.txID;


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.

src/ripple/core/impl/JobQueue.cpp

nbougalis

I'm fine with this as-is. @donovanhide and @thejohnfreeman have both discussed mitigations to gaming the ordering and I think that's great to add but, for me, it's not a requirement with this change.

The good thing about this change is that it is not transaction-breaking and, so, it doesn't require an amendment. Good job.

src/ripple/core/impl/JobQueue.cpp

nbougalis · 2021-12-10T21:34:07Z

src/ripple/app/misc/TxQ.h

        bool
        operator()(const MaybeTx& lhs, const MaybeTx& rhs) const
        {
+            if (lhs.feeLevel == rhs.feeLevel)
+                return lhs.txID < rhs.txID;


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.

ximinez · 2021-12-10T22:51:21Z

I think I recall somewhere that "fee level" is not the same as "fee amount"? If so, it might be worth considering sorting by the actual amount, so AccountDelete transactions (which are mostly beneficial in the longer run for validators - fewer objects in the ledger is better!) get heavily prioritized. This would also solve #4016

I think any solution to this issue is going to be more complicated that I think would be safe for this small,.simple, easy to reason about fix. Also, I would like to see what the consequences / fall out of this fix are before we jump too far into this issue. It's reasonably possible that once the dust settles, things will clear up enough that Account Delete transactions will succeed paying only their base fee.

ximinez · 2021-12-11T20:10:17Z

I just pushed two commits:

Change the some of the default queue sizes. These are a tad more conservative than what I think some of the validators are currently using.
Address @thejohnfreeman and @scottschurr 's suggestions.

scottschurr

👍 Looks great. Thanks!

* Sort by fee level (which is the current behavior) then by transaction ID (hash). * Edge case when the account at the end of the queue submits a higher paying transaction to walk backwards and compare against the cheapest transaction from a different account. * Use std::if_any to simplify the JobQueue::isOverloaded loop.

ximinez · 2021-12-14T18:59:49Z

Squashed the fold commit. No other changes.

ximinez added 2 commits December 9, 2021 18:09

Consensus transaction recovery / deferral completely ignores the TxQ

ef635e7

thejohnfreeman self-requested a review December 9, 2021 23:33

scottschurr self-requested a review December 9, 2021 23:38

scottschurr assigned thejohnfreeman and scottschurr Dec 9, 2021

thejohnfreeman approved these changes Dec 10, 2021

View reviewed changes

src/ripple/core/impl/JobQueue.cpp Outdated Show resolved Hide resolved

donovanhide reviewed Dec 10, 2021

View reviewed changes

scottschurr approved these changes Dec 10, 2021

View reviewed changes

nbougalis approved these changes Dec 10, 2021

View reviewed changes

ximinez force-pushed the deterministic-txq branch 2 times, most recently from c0d1859 to 5923a8d Compare December 11, 2021 20:15

scottschurr approved these changes Dec 13, 2021

View reviewed changes

thejohnfreeman mentioned this pull request Dec 13, 2021

Transaction queue eviction policy #4026

Open

ximinez added 3 commits December 14, 2021 13:58

Reduce TxQ logging severity in several places

010f0cf

Increase TxQ default minimum and target sizes

aa20afa

ximinez force-pushed the deterministic-txq branch from 5923a8d to aa20afa Compare December 14, 2021 18:59

This was referenced Dec 15, 2021

Proposed 1.8.2-rc1 #4039

Closed

Proposed 1.8.2-rc1 #4040

Merged

manojsdoshi closed this in #4040 Dec 17, 2021

ximinez deleted the deterministic-txq branch December 17, 2021 19:42

ximinez mentioned this pull request Jan 19, 2022

Improve deterministic transaction sorting in TxQ #4077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address elevated transaction fees. #4022

Address elevated transaction fees. #4022

ximinez commented Dec 9, 2021 •

edited

Loading

thejohnfreeman left a comment

mDuo13 commented Dec 10, 2021 •

edited

Loading

donovanhide Dec 10, 2021

MarkusTeufelberger Dec 10, 2021

donovanhide Dec 10, 2021

scottschurr Dec 10, 2021

nbougalis Dec 10, 2021 •

edited

Loading

ximinez Dec 10, 2021

donovanhide Dec 11, 2021

ximinez Dec 11, 2021

MarkusTeufelberger commented Dec 10, 2021

mDuo13 commented Dec 10, 2021

scottschurr left a comment

scottschurr Dec 10, 2021

ximinez Dec 11, 2021

ximinez Dec 11, 2021

scottschurr Dec 10, 2021

ximinez Dec 11, 2021 •

edited

Loading

ximinez Dec 11, 2021

scottschurr Dec 10, 2021

nbougalis left a comment

nbougalis Dec 10, 2021 •

edited

Loading

ximinez commented Dec 10, 2021 •

edited

Loading

ximinez commented Dec 11, 2021

scottschurr left a comment

ximinez commented Dec 14, 2021

Address elevated transaction fees. #4022

Address elevated transaction fees. #4022

Conversation

ximinez commented Dec 9, 2021 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

thejohnfreeman left a comment

Choose a reason for hiding this comment

mDuo13 commented Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusTeufelberger commented Dec 10, 2021

mDuo13 commented Dec 10, 2021

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximinez Dec 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

nbougalis Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

ximinez commented Dec 10, 2021 • edited Loading

ximinez commented Dec 11, 2021

scottschurr left a comment

Choose a reason for hiding this comment

ximinez commented Dec 14, 2021

ximinez commented Dec 9, 2021 •

edited

Loading

mDuo13 commented Dec 10, 2021 •

edited

Loading

nbougalis Dec 10, 2021 •

edited

Loading

ximinez Dec 11, 2021 •

edited

Loading

nbougalis Dec 10, 2021 •

edited

Loading

ximinez commented Dec 10, 2021 •

edited

Loading