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.

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

feat: discard txs by tx_count of sender #4520

Merged
merged 58 commits into from
Nov 29, 2023
Merged

Conversation

chirag-bgh
Copy link
Contributor

Closes #4269

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #4520 (00eed10) into main (4dc15c3) will increase coverage by 41.81%.
Report is 312 commits behind head on main.
The diff coverage is 0.00%.

❗ Current head 00eed10 differs from pull request most recent head c60e831. Consider uploading reports for the commit c60e831 to get more accurate results

Additional details and impacted files

Impacted file tree graph

Files Coverage Δ
crates/transaction-pool/src/pool/txpool.rs 74.81% <ø> (+39.88%) ⬆️
crates/transaction-pool/src/pool/parked.rs 60.88% <0.00%> (+38.54%) ⬆️
crates/transaction-pool/src/pool/pending.rs 61.68% <0.00%> (+13.78%) ⬆️

... and 442 files with indirect coverage changes

Flag Coverage Δ
integration-tests 17.02% <0.00%> (-9.05%) ⬇️
unit-tests 62.03% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 30.85% <ø> (+5.07%) ⬆️
blockchain tree 80.73% <ø> (+52.26%) ⬆️
pipeline 88.36% <ø> (+83.32%) ⬆️
storage (db) 74.59% <ø> (+44.62%) ⬆️
trie 94.96% <ø> (+72.43%) ⬆️
txpool 54.00% <0.00%> (+12.62%) ⬆️
networking 78.18% <ø> (+47.28%) ⬆️
rpc 57.97% <ø> (+31.49%) ⬆️
consensus 62.82% <ø> (+37.75%) ⬆️
revm 24.19% <ø> (+14.33%) ⬆️
payload builder 7.95% <ø> (-6.21%) ⬇️
primitives 84.77% <ø> (+55.59%) ⬆️

@chirag-bgh chirag-bgh changed the title (WIP)feat: discard txs by tx_count of sender feat: discard txs by tx_count of sender Sep 8, 2023
@chirag-bgh chirag-bgh marked this pull request as ready for review September 8, 2023 22:53
@github-actions github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Oct 12, 2023
@github-actions github-actions bot closed this Oct 19, 2023
@Rjected Rjected reopened this Oct 19, 2023
@Rjected
Copy link
Member

Rjected commented Oct 19, 2023

reviewing this and will probably be touching some of it up soon!

@Rjected Rjected added M-prevent-stale Prevents old inactive issues/PRs from being closed due to inactivity and removed S-stale This issue/PR is stale and will close with no further activity labels Oct 19, 2023
@rkrasiuk rkrasiuk added C-enhancement New feature or request A-tx-pool Related to the transaction mempool labels Oct 19, 2023
@Rjected Rjected self-assigned this Oct 20, 2023
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice, I think we're almost there.

making them pub should be fine, I can see the pool structs being useful

crates/transaction-pool/src/pool/parked.rs Outdated Show resolved Hide resolved
crates/transaction-pool/src/pool/parked.rs Outdated Show resolved Hide resolved
crates/transaction-pool/src/pool/parked.rs Outdated Show resolved Hide resolved
crates/transaction-pool/src/pool/parked.rs Outdated Show resolved Hide resolved
crates/transaction-pool/src/pool/pending.rs Outdated Show resolved Hide resolved
@@ -243,6 +246,23 @@ impl<T: TransactionOrdering> PendingPool<T> {
removed
}

/// Updates the independent transaction and independent descendants set, assuming the given
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs reference "highest nonce" instead

Copy link
Member

Choose a reason for hiding this comment

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

added

let mut removed = Vec::new();

// penalize non-local txs if limit is still exceeded
while self.size() > limit.max_size || self.len() > limit.max_txs {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the inverse of L360 and L380?

I think we can rewrite this as loop {} and only keep a break

let mut any_non_local = true;

// use descendants, so we pop a single tx per sender
for tx in self.highest_nonces.clone().iter() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

even though this only includes unique senders and we expect that the outer loop is really a single iterator.
I think we get rid of the clone if we instead push all txs ids that need to be removed to a tmp vec, keep track of the amount and sizes and only remove them once we have collected enough, then do the same again without locals check if necessary

Copy link
Member

Choose a reason for hiding this comment

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

tried to implement this with transactions_to_remove

@Rjected Rjected requested a review from mattsse November 23, 2023 05:05
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

great,

I only have one suggestion left for the pending eviction routine

crates/transaction-pool/src/pool/pending.rs Outdated Show resolved Hide resolved
crates/transaction-pool/src/pool/pending.rs Outdated Show resolved Hide resolved

// index of first tx to check
let mut removed_idx = 0;
'outer: loop {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not immediately obvious why we need this and this looks a bit complex.

I guess we need this because removing 1 tx per unique sender might not be sufficient? But the highest nonce is only updated once we remove the transactions?

I think this can be simplified by putting L372-L397 in a loop that actually removes the recorded ids of removed so we don't need the loop below?

If the function takes a &mut Vec then the transactions can be pushed directly to this, for both (locals =true|false) calls

Under real circumstances this will only evict 1 to a few transactions so having a tmp buffer for the removed ids isn't too bad.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

great,
only cleanup then I believe this is done

total_size += tx.transaction.size();
removed.push(*tx.transaction.id());
}
// we can re-use the temp array
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

/// If the `remove_locals` flag is unset, then only non-local transactions will be removed.
pub(crate) fn transactions_to_remove(
&self,
pub(crate) fn limit_pool(
Copy link
Collaborator

Choose a reason for hiding this comment

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

name should convey that this removes

Copy link
Member

Choose a reason for hiding this comment

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

renamed to remove_to_limit

removed.push(tx);
}
}
self.limit_pool(&limit, true, &mut removed);

removed.shrink_to_fit();
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to shrink anything here?

Copy link
Member

Choose a reason for hiding this comment

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

nope, was left over from when the map was created with_capacity, removed

@Rjected Rjected requested a review from mattsse November 27, 2023 20:18
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

great.
I'm very happy with how this turned out

Comment on lines +131 to +133
.into_iter()
// sort by submission id
.collect::<BinaryHeap<_>>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

not exactly sure how this works internally but I assume this into_iter collect is optimized so there are no allocs,

but I think this could be written as a loop over an iterator that pushes unique senders directly into a binaryheap

@Rjected Rjected added this pull request to the merge queue Nov 29, 2023
Merged via the queue into paradigmxyz:main with commit 15992d9 Nov 29, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tx-pool Related to the transaction mempool C-enhancement New feature or request M-prevent-stale Prevents old inactive issues/PRs from being closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evict transactions based on transaction accounts by sender
4 participants