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(txpool): function to return the next free nonce #11744

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

kien6034
Copy link
Contributor

Closes #11739

Verified

This commit was signed with the committer’s verified signature.
klkvr Arsenii Kulikov
@kien6034 kien6034 requested a review from mattsse as a code owner October 15, 2024 10:12
@kien6034 kien6034 marked this pull request as draft October 15, 2024 10:12
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.

we also need a simple test for this

@mattsse mattsse added the A-tx-pool Related to the transaction mempool label Oct 15, 2024

Verified

This commit was signed with the committer’s verified signature.
klkvr Arsenii Kulikov
@kien6034 kien6034 force-pushed the feat/find-next-free-nonce branch from 1244355 to 6334854 Compare October 15, 2024 10:39
@@ -356,6 +356,9 @@ pub trait TransactionPool: Send + Sync + Clone {
sender: Address,
) -> Option<Arc<ValidPoolTransaction<Self::Transaction>>>;

/// Returns the next free nonce for the given sender.
fn get_next_valid_transaction_by_sender(&self, sender: Address, on_chain_nonce: u64) -> Option<Arc<ValidPoolTransaction<Self::Transaction>>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should call this get_highest_consecutive_transaction_by_sender

@@ -356,6 +356,9 @@ pub trait TransactionPool: Send + Sync + Clone {
sender: Address,
) -> Option<Arc<ValidPoolTransaction<Self::Transaction>>>;

/// Returns the next free nonce for the given sender.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Returns the next free nonce for the given sender.
/// Returns the transaction with the highest nonce that is a direct ancestor of the on chain nonce without a nonce gap.

Comment on lines 122 to 130

if tx_nonce == current_nonce + 1 {
// This transaction is connected to the previous one
last_connected_tx = Some(Arc::clone(&tx.transaction));
current_nonce = tx_nonce;
} else if tx_nonce > current_nonce + 1 {
// We've found a gap, so we stop here
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can make this slightly nicer by incrementing the next consecutive nonce early

@mattsse mattsse marked this pull request as ready for review October 15, 2024 11:27
sender: SenderId,
on_chain_nonce: u64,
) -> Option<Arc<ValidPoolTransaction<T::Transaction>>> {
let mut next_expected_nonce = on_chain_nonce + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm not sure this is entirely correct. if the on chain nonce for an account is 1, then the next transaction sent should have nonce 1, not nonce 2.

@@ -108,6 +108,32 @@ impl<T: TransactionOrdering> TxPool<T> {
self.all().txs_iter(sender).last().map(|(_, tx)| Arc::clone(&tx.transaction))
}

/// Returns the next valid transaction for the given sender.
Copy link
Collaborator

Choose a reason for hiding this comment

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

going by the test, this returns the highest consecutive nonce in the pool, right? so the consumer would need to +1 on the value returned by this fn. this is slightly different behavior than e.g. the nonce RPC API, as this actually returns the exact value you need (i.e. no need to +1)

@kien6034
Copy link
Contributor Author

kien6034 commented Oct 15, 2024

@onbjerg tks, that makes sense. I updated the code.

@kien6034 kien6034 requested review from onbjerg and mattsse October 15, 2024 16:00
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.

lgtm

I made one simplification by leveraging one special API function

@mattsse mattsse added this pull request to the merge queue Oct 16, 2024
Merged via the queue into paradigmxyz:main with commit b814770 Oct 16, 2024
39 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add txpool function to find the next free nonce
3 participants