fix(txpool): evict AA2dPool transactions by priority instead of address to prevent DoS#2216
fix(txpool): evict AA2dPool transactions by priority instead of address to prevent DoS#2216
Conversation
0xrusowsky
left a comment
There was a problem hiding this comment.
the approach LGTM, but i left a cmnt for a small optimization (untested)
mattsse
left a comment
There was a problem hiding this comment.
this isnt a trivial problem to solve, hence the todo (hehe)
I think a more efficient and fair way to achieve this would be going by submission id which we can also use to clear older txs that we failed to evict based on some dynamic behaviour.
ideally we also track queued and pending separately so that we clear queued txs first.
so we should try to
a) evict queued txs first
b) only then start evicting pending ones
since we have const basefee the priority is also const, so we maybe order them by struct prio,submissionid in a separate map
|
yeah i think it should be cheap enough to maintain an extra index for transactions that can be evicted and that would also be properly sorted |
720d797 to
3593797
Compare
@mattsse indeed, makes total sense. i've pushed an impl for this, using |
mattsse
left a comment
There was a problem hiding this comment.
I find this a bit hard to review, because this now requires that we keep track if pending vs queued separately for eviction but we dont do this for the total set
maybe we should take a step back here and reconsider the current design instead of patching a bunch of stuff that makes is quite hard to reason about
3593797 to
aba4c2d
Compare
|
@mattsse all review comments should be addressed, ptal |
mattsse
left a comment
There was a problem hiding this comment.
the atomic is technically not needed so we could optimize this away but relaxed also seems fine
| /// Uses `Arc<AtomicBool>` so we can mutate this flag without removing/reinserting | ||
| /// the transaction from the eviction set. This allows a single eviction set for | ||
| /// all transactions, with pending/queued filtering done at eviction time. | ||
| is_pending: Arc<AtomicBool>, |
There was a problem hiding this comment.
this doesnt technically need to be an atomic and can be rc refcell because this only ever accessed by the pool via mut anyway
| struct EvictionKey { | ||
| /// The transaction's priority (from `CoinbaseTipOrdering`). | ||
| priority: Priority<u128>, | ||
| /// The submission ID when the transaction was added to the pool. | ||
| /// This is unique per transaction, so it's sufficient for ordering. | ||
| submission_id: u64, | ||
| /// The transaction's unique identifier (for lookup during eviction). | ||
| tx_id: AA2dTransactionId, | ||
| /// Shared pending state for filtering without lookup. | ||
| is_pending: Arc<AtomicBool>, | ||
| } |
There was a problem hiding this comment.
we could technically just wrap arc AA2dInternalTransaction for this but this doesnt really matter here
b624bf4 to
e521651
Compare
| .values() | ||
| .filter(move |tx| tx.is_pending && tx.inner.transaction.origin == origin) | ||
| .filter(move |tx| { | ||
| tx.is_pending.load(Ordering::Relaxed) && tx.inner.transaction.origin == origin |
There was a problem hiding this comment.
last suggestion, would be nicer if this were a fn is_pending() then we can change this more easily later
| } | ||
| std::cmp::Ordering::Equal => { | ||
| existing_tx.is_pending = true; | ||
| *existing_tx.is_pending.borrow_mut() = true; |
There was a problem hiding this comment.
would be nicer if we had fn is_pending and set_pending, then all of the borrow stuff is hidden and we can change this more easily
…ss to prevent DoS
…or faster eviction lookups
…ily at eviction time
…stead of duplicating fields
e521651 to
08f16ff
Compare
Closes CHAIN-449
Fixes a DoS vulnerability in
AA2dPool::discard()where transactions were evicted lexicographically by address instead of by priority. Attackers with vanity addresses (many leading zeroes like0x0000...) could persist in the pool indefinitely while legitimate transactions from normal addresses were evicted first. This allowed low-cost DoS attacks on the transaction pool.Changed
discard()to sort transactions byCoinbaseTipOrderingpriority and evict lowest-priority transactions first, regardless of sender address.