-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Added a new RPC endpoint (bor_sendRawTransactionConditional) to support EIP-4337 Bundled Transactions #8229
Added a new RPC endpoint (bor_sendRawTransactionConditional) to support EIP-4337 Bundled Transactions #8229
Conversation
core/state/intra_block_state.go
Outdated
case v.IsStorage(): | ||
for slot, value := range v.Storage { | ||
slot := slot | ||
tempByte, _ := sdb.stateReader.ReadAccountStorage(k, tempAccount.Incarnation, &slot) |
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.
don't loose error plz
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.
Updated here. Thanks!
turbo/jsonrpc/bor_snapshot.go
Outdated
return common.Hash{}, err | ||
} | ||
|
||
txTemp, err := api.db.BeginRo(ctx) |
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.
in erigon we using naming:
txn
- ethereum transaction
tx
- databases transaction
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.
Thanks for the information. Updated here.
turbo/jsonrpc/bor_snapshot.go
Outdated
} | ||
defer txTemp.Rollback() | ||
|
||
currentHeader := rawdb.ReadCurrentHeader(txTemp) |
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.
check nillness plz
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.
Done here, thanks.
|
||
// this has been moved to prior to adding of transactions to capture the | ||
// pre state of the db - which is used for logging in the messages below | ||
tx, err := api.db.BeginRo(ctx) |
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.
- you already have
temptx
above api.db
it'schaindata
db, it's not related to txpool's db. So, "pre-state" of which db do you need? I don't see any "logging below"
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.
Oh, my bad. Thank you, updated here.
Yes, I need the chaindata
db.
turbo/jsonrpc/bor_snapshot.go
Outdated
return common.Hash{}, errors.New("only replay-protected (EIP-155) transactions allowed over RPC") | ||
} | ||
|
||
defer tx.Rollback() |
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.
Duplicate line
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.
My bad, fixed here.
Thanks.
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.
This needs to be rebased onto devel
with builtin erigon-lib
core/state/intra_block_state.go
Outdated
@@ -825,3 +826,46 @@ func (sdb *IntraBlockState) AddressInAccessList(addr libcommon.Address) bool { | |||
func (sdb *IntraBlockState) SlotInAccessList(addr libcommon.Address, slot libcommon.Hash) (addressPresent bool, slotPresent bool) { | |||
return sdb.accessList.Contains(addr, slot) | |||
} | |||
|
|||
// ValidateKnownAccounts validates the knownAccounts passed in the options parameter in the conditional transaction (EIP-4337) | |||
func (sdb *IntraBlockState) ValidateKnownAccounts(knownAccounts types.KnownAccounts) error { |
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.
Let's try to find a better place for this method, because I feel that it doesn't belong to the IntraBlockState.
I'd also love to make it more easy to unit test by accepting StateReader interface as a parameter.
In the comment above you refer to the full EIP-4337. Could you please add a link to a subchapter of EIP-4337 where the relevant information is described?
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.
Could you please recommend any other place for this? The reason I added it here was because the storage slots were available at this location.
About the comment, it is not in the EIP, but in the spec released by the EF researchers here - https://notes.ethereum.org/@yoav/SkaX2lS9j.
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.
Let me try to find and suggest a better place for this, and get back to you. I think it should be in the same file as ValidateTransactionConditions once we figure it out.
My current suggestion is to refactor this method to a free function by adding a parameter of type state.StateReader
- you can use it to get the storage slots. The stateReader instance is accessible in SpawnMiningExecStage func, and can be passed to addTransactionsToMiningBlock as a new parameter and forwarded to ValidateTransactionConditions. It is also accessible as readerTemp
in SendRawTransactionConditional func (currentState could be removed there).
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.
We'd like to have a new top-level polygon
folder/package for this with intention to centralize the other polygon-specific code into it in the future.
If you make a new file like polygon/transaction_conditional.go
and move
ValidateKnownAccounts and ValidateTransactionConditions functions there, would it make sense to you?
} | ||
|
||
func SingleFromHex(hex string) *Value { | ||
return &Value{Single: libcommon.HexToRefHash(hex)} |
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.
return &Value{Single: libcommon.HexToRefHash(hex)} | |
return &Value{Single: &libcommon.HexToHash(hex)} |
with this there's no need for a new HexToRefHash function in erigon-lib
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.
Sorry, this will not work. It gives the following error. - cannot take address of libcommon.HexToHash(hex) (value of type "github.com/ledgerwatch/erigon-lib/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.
Oh, I didn't expect this. Maybe this is possible?
func SingleFromHex(hex string) *Value {
hash := libcommon.HexToHash(hex)
return &KnownAccountStorageCondition{StorageRootHash: &hash}
See also: https://stackoverflow.com/questions/10535743/address-of-a-temporary-in-go
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.
A more general question: as a client who sends this transaction, how do I know it was rejected? How is it done with eth_sendRawTransaction? I feel in this case it is more necessary, because of unpredictable state changes. As as user I wouldn't want to sit and wait, and also not spam with more sendRawTransactionConditional to increase the chance of success.
It is currently done using transaction replay in otterscan: https://github.com/ledgerwatch/erigon/blob/devel/turbo/jsonrpc/otterscan_transaction_error.go We need to make sure that the new transaction validation errors are recoverable via the otterscan runTracer API call, see here: https://github.com/ledgerwatch/erigon/blob/devel/turbo/jsonrpc/otterscan_api.go#L117 |
4d05650
to
74e53a4
Compare
Hi @battlmonstr - thanks for the thorough review, I will be addressing them. I have just rebased my branch ( |
…nownAccountStorageCondition
f7b5195
to
220acfd
Compare
Hi @battlmonstr
The bundlers are the ones sending the transaction (this transaction is known as a conditional transaction.)
Yes, this transaction is treated exactly the same as the conditional transaction (only with some additional checks which validate the extra data ( This is explained in detail in PIP-15 and the spec by the EF researchers. |
// check if the value is hex string or an object | ||
switch { |
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.
looks like now we could move the tempAccount
check and else
logic outside switch before it, is it ok for you?
if tempAccount == nil {
return fmt.Errorf("Storage Trie is nil for: %v", address)
}
// check if the value is hex string or an object
switch {
...
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.
Done.
core/types/access_list_tx.go
Outdated
func (tx *AccessListTx) PutOptions(options *types2.TransactionConditions) { | ||
tx.TransactionConditions = options | ||
} | ||
|
||
func (tx *AccessListTx) GetOptions() *types2.TransactionConditions { | ||
return tx.TransactionConditions | ||
} |
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.
as the logic is trivial now, do you still need these methods, or is it ok to access the field directly (and remove the methods)?
if you prefer to keep it, it should be renamed to Get/PutTransactionConditions()
core/types/blob_tx_wrapper.go
Outdated
func (txw BlobTxWrapper) PutOptions(options *types2.TransactionConditions) { | ||
txw.Tx.TransactionConditions = options | ||
} | ||
|
||
func (txw BlobTxWrapper) GetOptions() *types2.TransactionConditions { | ||
return txw.Tx.TransactionConditions | ||
} |
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.
see my comment about AccessListTx.PutOptions
core/types/dynamic_fee_tx.go
Outdated
func (tx *DynamicFeeTransaction) PutOptions(options *types2.TransactionConditions) { | ||
tx.TransactionConditions = options | ||
} | ||
|
||
func (tx *DynamicFeeTransaction) GetOptions() *types2.TransactionConditions { | ||
return tx.TransactionConditions | ||
} |
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.
see my comment about AccessListTx.PutOptions
core/types/legacy_tx.go
Outdated
func (tx *LegacyTx) PutOptions(options *types2.TransactionConditions) { | ||
tx.TransactionConditions = options | ||
} | ||
|
||
func (tx *LegacyTx) GetOptions() *types2.TransactionConditions { | ||
return tx.TransactionConditions | ||
} |
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.
see my comment about AccessListTx.PutOptions
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.
Thanks. It is good for now, I will try to suggest an alternative approach, and we'll see if we want to do it, or keep it as it is. It is on my TODO list.
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.
@pratikspatil024 Thanks for the updates. This looks good to me apart from 2 questions that should be confirmed:
- Clarify how conditions propagate from the API call to the mining stage.
- Are those TransactionConditions in-memory only, or are they persisted in any way? If erigon process restarts, do we start from an empty txn pool or do we read it from the database? In this case do we want to recover conditions as well?
- Do we want the new validation errors be visible in block explorers? If yes, could we confirm that Otterscan runTracer API call can recover it? If not, what are the obstacles?
turbo/jsonrpc/bor_snapshot.go
Outdated
// put options data in Tx, to use it later while block building | ||
txn.PutOptions(&options) |
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.
how do options get from here to the mining stage? isn't the txn
object discarded after this call?
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 seems to us that this property will not propagate to the mining stage
the new API call adds a transaction to the pool using the txpool GRPC Add()
API:
https://github.com/ledgerwatch/interfaces/blob/master/txpool/txpool.proto#L90
it is then added to the pool in the mining process with txPool.AddLocalTxs()
:
https://github.com/ledgerwatch/erigon/blob/devel/erigon-lib/txpool/txpool_grpc_server.go#L216
the mining process could be a separate process from the JSON RPC API server process
later on the mining stage grabs transactions from the pool using YieldBest()
here:
https://github.com/ledgerwatch/erigon/blob/devel/eth/stagedsync/stage_mining_exec.go#L202
if we want to pass TransactionConditions from the API following the same route, we'll need to extend all 3 call points (GRPC Add, AddLocalTxs, YieldBest) with extra parameters, and add a new property to TxSlot
or metaTx
to hold it.
@AskAlexSharov , @mh0lt , is this analysis correct? is it a reasonable thing to do, or do we want a different approach for bor-specific transaction metadata?
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.
@battlmonstr you described well. Is this metadata encoded into txn rlp? Or it’s some side-data?
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.
If external:
- txpool already has MetaTx wrapper for external data (or aggregates).
- Grpc methods: need extend.
But: txpool itself does sorting txs from best to worst - by method Less. Maybe method Less must his new fields? (It knows current block number). Then miner will have less logic.
Anyway it feels useful feature that: miner can YeldBest with conditions - it may allow build more flexible miners.
FYI: in our architecture view - miner was inside txpool process (always) - this is reason why there is no grpc between txpool and miner.
About conditions type: plz check if possible use uint256 instead of big.Int.
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.
@AskAlexSharov Thanks for the suggestions. This metadata comes as a separate parameter of this new JSON RPC call, so not inside a txn encoding.
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.
Hey @battlmonstr - thanks for pointing this out. I'll update this so that the options
are available to the miner.
@battlmonstr, @AskAlexSharov -we have decided to only perform the checks on the API/RPC level for now, so cleaned up the code a bit. Will add checks and continue on top of these changes later. Can you please review this PR again? Thanks. |
Hey folks, any updates on this? |
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.
LGTM as a first step (see also new comments/suggestions).
@@ -827,3 +828,46 @@ func (sdb *IntraBlockState) AddressInAccessList(addr libcommon.Address) bool { | |||
func (sdb *IntraBlockState) SlotInAccessList(addr libcommon.Address, slot libcommon.Hash) (addressPresent bool, slotPresent bool) { | |||
return sdb.accessList.Contains(addr, slot) | |||
} | |||
|
|||
// ValidateKnownAccounts validates the knownAccounts passed in the options parameter in the conditional transaction (EIP-4337) | |||
func (sdb *IntraBlockState) ValidateKnownAccounts(knownAccounts types2.KnownAccountStorageConditions) error { |
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.
This should be decoupled from IntraBlockState, see this thread:
#8229 (comment)
@@ -61,6 +61,12 @@ func BigToHash(b *big.Int) Hash { return BytesToHash(b.Bytes()) } | |||
// If b is larger than len(h), b will be cropped from the left. | |||
func HexToHash(s string) Hash { return BytesToHash(hexutility.FromHex(s)) } | |||
|
|||
func HexToRefHash(s string) *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.
Rename to match the method above:
func HexToRefHash(s string) *Hash { | |
func HexToHashRef(s string) *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.
move this to the root polygon
folder
Added a new RPC endpoint (bor_sendRawTransactionConditional ) to support EIP-4337 Bundled Transactions.
Implements PIP-15.
This is still a WIP as it will require some changes in the txpool which will be made in PR#1229 in erigon-lib.
Here is the corresponding PR in bor.