Skip to content

feat: Add EIP7702 tx handle logic for txpool #15312

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

Merged
merged 10 commits into from
May 9, 2025

Conversation

Pana
Copy link
Contributor

@Pana Pana commented Mar 27, 2025

This PR is a mirror of Geth's relative PR ethereum/go-ethereum#31073

Which add two new rules for 7702 TX:

  1. In addition to tracking transactions, the pool also tracks a set of pending SetCode
    authorizations (EIP7702). As a standard rule, any account with a deployed
    delegation or an in-flight authorization to deploy a delegation will only be allowed a
    single transaction slot instead of the standard number. This is due to the possibility
    of the account being sweeped by an unrelated account.

  2. In case the pool is tracking a pending / queued transaction from a specific account, it
    will reject new transactions with delegations from that account with standard in-flight
    transactions.

@Pana
Copy link
Contributor Author

Pana commented Mar 27, 2025

Not sure the AllTransactions.auths clean logic is added correctly, please help check it

@emhane emhane added A-tx-pool Related to the transaction mempool E-prague Related to the prague network upgrade labels Mar 27, 2025
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.

I think this looks great already tysm, sorry for the delayed review.

the only real concern I have with this is that we're doing recovery inside the pool.

I see that we need to track auths, in the pool itself,
I can think of two ways to improve this, effectively the auths are additional metadata that we return as part of TransactionValidationOutcome::Valid, and we could track a list of auth sender ids in here:

pub struct ValidPoolTransaction<T: PoolTransaction> {
/// The transaction
pub transaction: T,
/// The identifier for this transaction.
pub transaction_id: TransactionId,

then we don't need to deal with addresses and we can avoid moving the sender id map and don't need to do recovery in the pool, if the job of the validator is to recover those addresses and then we can map those to sender ids when we create and freeze the ValidPoolTransaction here:

let tx = ValidPoolTransaction {

just like we do with the sender id

wdyt @Rjected

@github-project-automation github-project-automation bot moved this from Todo to In Progress in Reth Tracker Mar 31, 2025
@Pana Pana force-pushed the feat/txpool7702-v2 branch 2 times, most recently from 07eb459 to 7b9ffda Compare April 1, 2025 05:25
@Pana
Copy link
Contributor Author

Pana commented Apr 1, 2025

Thanks for your advice @mattsse

@Pana
Copy link
Contributor Author

Pana commented Apr 1, 2025

This PR solve issue #15079

@Pana Pana force-pushed the feat/txpool7702-v2 branch from 7b9ffda to 77b06b5 Compare April 1, 2025 06:03
@Pana Pana requested a review from mattsse April 1, 2025 06:17
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,

structurally this is now exactly how it should be done,

need some help from @Rjected reviewing the auth checks

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

This makes sense, I only have some doc / style nits, could not find anything wrong with the checks. Some tests however for the new validations would be appreciated, although this could be done in a followup

@Pana Pana requested a review from mattsse April 2, 2025 02:45
@Pana
Copy link
Contributor Author

Pana commented Apr 2, 2025

This makes sense, I only have some doc / style nits, could not find anything wrong with the checks. Some tests however for the new validations would be appreciated, although this could be done in a followup

will add some tests in next pr

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

I have just one comment on docs, otherwise this looks good to me

@Pana Pana force-pushed the feat/txpool7702-v2 branch from c5aa1cc to 10352c7 Compare April 5, 2025 07:07
@Pana Pana requested a review from Rjected April 5, 2025 07:14
@Pana
Copy link
Contributor Author

Pana commented Apr 18, 2025

Hey @mattsse is there anything to do with this PR

@mattsse mattsse changed the title Add EIP7702 tx handle logic for txpool feat: Add EIP7702 tx handle logic for txpool Apr 28, 2025
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.

this looks complete now,, tysm

just need to test a few things before we can merge this

pending @Rjected

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@Rjected
Copy link
Member

Rjected commented Apr 28, 2025

just needs clippy / compile fixes, trust that they are just from the api changes here

@Pana Pana requested a review from onbjerg as a code owner May 8, 2025 06:15
@Pana Pana force-pushed the feat/txpool7702-v2 branch 2 times, most recently from 3ef1f6d to 1ee2ee8 Compare May 8, 2025 07:14
@Pana Pana closed this May 8, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker May 8, 2025
@Pana
Copy link
Contributor Author

Pana commented May 8, 2025

It's wired, the github action failure related code is different with my local code, seems there are some cache in somewhere. I will create a new PR for this change

@mattsse
Copy link
Collaborator

mattsse commented May 8, 2025

@Pana I think this is just some cargo.lock issue

@mattsse mattsse reopened this May 8, 2025
@github-project-automation github-project-automation bot moved this from Done to In Progress in Reth Tracker May 8, 2025
@mattsse
Copy link
Collaborator

mattsse commented May 8, 2025

could you perhaps try after rebasing?

@Pana Pana force-pushed the feat/txpool7702-v2 branch from 1ee2ee8 to 2d34a0c Compare May 8, 2025 10:40
@Pana
Copy link
Contributor Author

Pana commented May 8, 2025

Thanks for your advice, some new added tests need to be sync and update

@mattsse mattsse added this pull request to the merge queue May 9, 2025
Merged via the queue into paradigmxyz:main with commit 448e909 May 9, 2025
45 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker May 9, 2025
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 E-prague Related to the prague network upgrade
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants