Unify fee-based transaction comparisions #3508
Conversation
| releaseAssert(iter != mTxStackSet.end()); | ||
| auto const& evictTxStack = *iter; | ||
| auto const& evictTx = *evictTxStack->getTopTx(); | ||
| auto evictOps = evictTxStack->getNumOperations(); |
There was a problem hiding this comment.
I think this method (canFitWithEviction) should not be in this class, or should be written differently:
what that code does is use the top tx evictTx to decide on evicting the entire stack (and the code in , which does not make sense in the general case, and only in the context of TxQueueLimiter that right now uses SingleTxStack.
If we want to keep it in here, the challenge (I think) is that you can't really iterate over txs in stacks without popping.
There was a problem hiding this comment.
what that code does is use the top tx evictTx to decide on evicting the entire stack
Well, that's the invariant of this method and it happens to work for TxQueueLimiter. I can't say it's incorrect for seq num based stack implementations, as the same invariant would be maintained (whether it's useful or not is up to the caller; currently I guess we know we don't need to reuse it).
I cannot say I'm 100% happy with this class, however I think it's a decent option. The set of supported operations is indeed bespoke and isn't too abstract. This is due to the fact that we have quite specific use cases and there is only so much in common between them. In order to support the upcoming functionality I wanted to encapsulate as much logic as possible in these methods and let as little internals to be exposed as possible. The implementation with lanes is more involved in terms of using the internal structures.
I guess we could make the queue to be 'low-level' data structure that exposes the internals and then a wrapper data structure to be used in TxQueueLimiter with SingleTxStack only. However, I'm not sure how much value does that provide, given that the complexity still lies in the iteration methods. I'd also still rather keep the logic in SurgePricingUtils so that there is a single source for all surge-pricing related stuff.
There was a problem hiding this comment.
We talked offline about potentially getting rid of the "transaction chains" per account in the future, so I think it's fine to keep the code as is. As right now we have no use cases for this method outside of the SingleTxStack, and to avoid surprises if we were to use it in a different context, I think we should just add an assert that there is only one tx in the stack:
auto const& evictTx = *evictTxStack->getTopTx();
auto evictOps = evictTxStack->getNumOperations();
releaseAssert(evictOps == evictTx.getNumOperations());
what do you think?
There was a problem hiding this comment.
SG, added the assertion.
…ons. The priority queue allows ranking transactions/transaction stacks (seq num ordered) by fee rate and limit the amount of operations. This replaces 3 existing implementations of surge-pricing-based comparisons and has special operations to support their use cases: - Building the `TxSetFrame` from transactions via selecting highest fee rate transactions - Limiting the transactions in `TxQueueLimiter` via evicting lowest fee rate transactions - Broadcasting the transactions in 'TransactionQueue` via selecting highest fee rate transactions The respective user changes are in the followup commits.
dmkozh
force-pushed
the
surge_pricing_queue
branch
from
de46078
to
54f2f93
Compare
Noticed this while checking the refactored code.
…QueueLimiter`. Instead of explicitly looking at the limiter size, verify how many operations were evicted (this also generally improves coverage). Instead of explicitly looking at the minimum fee stored in limiter, make sure that we can (or can not) add transaction at the fee threshold.
…aching the `TxQueueLimiter` limit.
dmkozh
force-pushed
the
surge_pricing_queue
branch
from
54f2f93
to
9d3ce58
Compare
I've rebased & squashed, just need to wait for the checks to pass. |
|
r+ 9d3ce58 |
Description
This introduces a common data structure (
SurgePricingPriorityQueue) that orders the transactions by the fee rate and is aware of operation limits. This data structure is then used in the 3 places that do fee-based transaction comparisons:TransactionQueue,TxQueueLimiterandTxSetFrame.The refactoring is functionally a no-op and serves as a preparation for introducing more complex transaction comparison logic.
Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)