-
Notifications
You must be signed in to change notification settings - Fork 20.5k
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
txpool: add support for set code transactions #31073
base: master
Are you sure you want to change the base?
Conversation
77301e7
to
b2ece7b
Compare
core/txpool/legacypool/legacypool.go
Outdated
beats map[common.Address]time.Time // Last heartbeat from each known account | ||
all *lookup // All transactions to allow lookups | ||
priced *pricedList // All transactions sorted by price | ||
auths map[common.Address][]*types.Transaction // All accounts with a pooled authorization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason for tracking the tx object itself, rather than the tx hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could definitely track the tx hash instead. Is there a preference? I originally went with the point because it is how txs are generally tracked in the pool, but later noticed that we never actually need to use any of values of the tx and it's really just a tracker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tx is larger than txhash
but later noticed that we never actually need to use any of values of the tx and it's really just a tracker.
Exactly, I would prefer some memory-efficient approach, while the memory consumption is really negligible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it larger? We're just tracking the pointer to the transaction so we shouldn't be duplicating the transactions. The transactions themselves need to be stored in memory at least once anyway, right?
Maybe this will force us to keep the transactions longer than necessary in memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, It's a pointer. Though I feel it's a bit weird to track the entire transaction and the transaction fields are never used.
We have to add the SetCodeType support in the txpool, https://github.com/ethereum/go-ethereum/blob/master/core/txpool/legacypool/legacypool.go#L280 |
Hey @lightclient happy to see you working on this. While writing end-2-end tests for |
9e78130
to
84593a1
Compare
Co-authored-by: lightclient <[email protected]>
I’m fine with adding the restriction to the txpool, ensuring that delegation and normal transactions cannot coexist simultaneously. However, I’m curious about the typical transaction flow in real-world usage and unsure if it’s valid to first have a delegation and then follow it up with a normal transaction. In the legacyPool, we maintain a pendingNoncer to ensure the multiple inflight transactions from the same sender could be accepted and bundled in a single block. We can extend this noncer a bit by considering the auths as well in the future, to have more flexible/loose rules on txpool |
@@ -1711,6 +1735,7 @@ func (t *lookup) Remove(hash common.Hash) { | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether the error two lines up ever happens, because in that case we would retain a weird authlist and a memleak
// Verify no authorizations will invalidate existing transactions known to | ||
// the pool. | ||
if opts.KnownConflicts != nil { | ||
if conflicts := opts.KnownConflicts(from, tx.Authorities()); len(conflicts) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC we only check authority conflicts between "normal" transactions and ignore blob transactions. I think thats fine, I did a benchmark back then which showed that our blobpool can handle removals and reorgs quite well. I will try to find that benchmark again to add it
The new SetCode transaction type introduces some additional complexity when handling the transaction pool.
This complexity stems from two new account behaviors:
The first issue has already been considered extensively during the design of ERC-4337, and we're relatively confident in the solution of simply limiting the number of in-flight pending transactions an account can have to one. This puts a reasonable bound on transaction cancellation. Normally to cancel, you would need to spend 21,000 gas. Now it's possible to cancel for around the cost of warming the account and sending value (
2,600+9,000=11,600
). So 50% cheaper.The second issue is more novel and needs further consideration.
There are a few issues we must carefully design around:
transactions with conflicting authorizations. Otherwise, it might be possible to cherry-pick authorizations from txs and front run them with different txs at much lower fee amounts, effectively DoSing the authority. Fortunately, conflicting
authorizations do not affect the underlying validity of the transaction so we can just accept both.
A few situations to test:
the tx are removed.
5.1. This should also work when a self-sponsored setcode tx attempts
to replace itself.
the pool, otherwise someone might submit old validations to deny
service to unsuspecting users.
maybe try to write out each possible scenario for all types of txs entering the
pool +different 7702 situations and consider outcomes?