Added a new RPC endpoint (bor_sendRawTransactionConditional) to support EIP-4337 Bundled Transactions by pratikspatil024 · Pull Request #8229 · erigontech/erigon

pratikspatil024 · 2023-09-18T03:20:55Z

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.

AskAlexSharov · 2023-09-18T05:07:05Z

+		case v.IsStorage():
+			for slot, value := range v.Storage {
+				slot := slot
+				tempByte, _ := sdb.stateReader.ReadAccountStorage(k, tempAccount.Incarnation, &slot)


don't loose error plz

Updated here. Thanks!

AskAlexSharov · 2023-09-18T05:10:14Z

+		return common.Hash{}, err
+	}
+
+	txTemp, err := api.db.BeginRo(ctx)


in erigon we using naming:
txn - ethereum transaction
tx - databases transaction

Thanks for the information. Updated here.

AskAlexSharov · 2023-09-18T05:10:33Z

+	}
+	defer txTemp.Rollback()
+
+	currentHeader := rawdb.ReadCurrentHeader(txTemp)


check nillness plz

Done here, thanks.

AskAlexSharov · 2023-09-18T05:13:23Z

+
+	// 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)


you already have temptx above

api.db it's chaindata 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"

Oh, my bad. Thank you, updated here.
Yes, I need the chaindata db.

AskAlexSharov · 2023-09-19T09:04:14Z

+		return common.Hash{}, errors.New("only replay-protected (EIP-155) transactions allowed over RPC")
+	}
+
+	defer tx.Rollback()


Duplicate line

My bad, fixed here.
Thanks.

battlmonstr

This needs to be rebased onto devel with builtin erigon-lib

battlmonstr · 2023-09-22T08:53:32Z

 }
+
+// ValidateKnownAccounts validates the knownAccounts passed in the options parameter in the conditional transaction (EIP-4337)
+func (sdb *IntraBlockState) ValidateKnownAccounts(knownAccounts types.KnownAccounts) error {


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?

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.

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).

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?

battlmonstr · 2023-09-22T09:17:08Z

+}
+
+func SingleFromHex(hex string) *Value {
+	return &Value{Single: libcommon.HexToRefHash(hex)}


Suggested change

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

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)

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

battlmonstr

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.

battlmonstr · 2023-09-22T11:02:46Z

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

pratikspatil024 · 2023-10-03T03:24:37Z

Hi @battlmonstr - thanks for the thorough review, I will be addressing them.

I have just rebased my branch (pratikspatil024:aa-4337) with the latest devel changes, could you also do the same with the base branch of this PR (ledgerwatch:bor_account_abstraction)? Thanks.

…nownAccountStorageCondition

…geConditions

pratikspatil024 · 2023-10-05T03:40:08Z

Hi @battlmonstr

as a client who sends this transaction

The bundlers are the ones sending the transaction (this transaction is known as a conditional transaction.)

how do I know it was rejected? How is it done with eth_sendRawTransaction?

Yes, this transaction is treated exactly the same as the conditional transaction (only with some additional checks which validate the extra data (TransactionConditions) which is sent with the transaction). We have 2 more error codes which will be sent if the validation fails.

This is explained in detail in PIP-15 and the spec by the EF researchers.

battlmonstr · 2023-10-12T16:14:01Z

+		// check if the value is hex string or an object
+		switch {


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 { ...

battlmonstr · 2023-10-12T16:16:38Z

+func (tx *AccessListTx) PutOptions(options *types2.TransactionConditions) {
+	tx.TransactionConditions = options
+}
+
+func (tx *AccessListTx) GetOptions() *types2.TransactionConditions {
+	return tx.TransactionConditions
+}


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()

battlmonstr · 2023-10-12T16:22:20Z

+func (txw BlobTxWrapper) PutOptions(options *types2.TransactionConditions) {
+	txw.Tx.TransactionConditions = options
+}
+
+func (txw BlobTxWrapper) GetOptions() *types2.TransactionConditions {
+	return txw.Tx.TransactionConditions
+}


see my comment about AccessListTx.PutOptions

battlmonstr · 2023-10-12T16:22:31Z

+func (tx *DynamicFeeTransaction) PutOptions(options *types2.TransactionConditions) {
+	tx.TransactionConditions = options
+}
+
+func (tx *DynamicFeeTransaction) GetOptions() *types2.TransactionConditions {
+	return tx.TransactionConditions
+}


see my comment about AccessListTx.PutOptions

battlmonstr · 2023-10-12T16:22:37Z

+func (tx *LegacyTx) PutOptions(options *types2.TransactionConditions) {
+	tx.TransactionConditions = options
+}
+
+func (tx *LegacyTx) GetOptions() *types2.TransactionConditions {
+	return tx.TransactionConditions
+}


see my comment about AccessListTx.PutOptions

battlmonstr · 2023-10-12T16:25:47Z

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.

battlmonstr

@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?

battlmonstr · 2023-10-12T20:11:19Z

+	// put options data in Tx, to use it later while block building
+	txn.PutOptions(&options)


how do options get from here to the mining stage? isn't the txn object discarded after this call?

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?

@battlmonstr you described well. Is this metadata encoded into txn rlp? Or it’s some side-data?

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.

@AskAlexSharov Thanks for the suggestions. This metadata comes as a separate parameter of this new JSON RPC call, so not inside a txn encoding.

Hey @battlmonstr - thanks for pointing this out. I'll update this so that the options are available to the miner.

…ontinue later

pratikspatil024 · 2023-10-19T06:00:32Z

@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.

pratikspatil024 · 2023-11-06T06:34:00Z

Hey folks, any updates on this?

battlmonstr

LGTM as a first step (see also new comments/suggestions).

battlmonstr · 2023-12-13T14:49:54Z

 }
+
+// ValidateKnownAccounts validates the knownAccounts passed in the options parameter in the conditional transaction (EIP-4337)
+func (sdb *IntraBlockState) ValidateKnownAccounts(knownAccounts types2.KnownAccountStorageConditions) error {


This should be decoupled from IntraBlockState, see this thread:
#8229 (comment)

battlmonstr · 2023-12-13T14:51:15Z

 // 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 {


Rename to match the method above:

Suggested change

func HexToRefHash(s string) *Hash {

func HexToHashRef(s string) *Hash {

battlmonstr · 2023-12-13T14:52:24Z

move this to the root polygon folder

AskAlexSharov reviewed Sep 18, 2023

View reviewed changes

pratikspatil024 mentioned this pull request Sep 18, 2023

Added a new RPC endpoint (bor_sendRawTransactionConditional) to support EIP-4337 Bundled Transactions ledgerwatch/erigon-lib#1129

Draft

AskAlexSharov reviewed Sep 19, 2023

View reviewed changes

yperbasis added the polygon label Sep 19, 2023

battlmonstr suggested changes Sep 22, 2023

View reviewed changes

battlmonstr reviewed Sep 22, 2023

View reviewed changes

mh0lt marked this pull request as ready for review September 28, 2023 11:05

pratikspatil024 added 3 commits October 3, 2023 08:51

initial commit to implement bor_sendRawTransactionConditional

883ed31

addressed comments

b6b5a38

removed duplicate line

74e53a4

pratikspatil024 force-pushed the aa-4337 branch from 4d05650 to 74e53a4 Compare October 3, 2023 03:22

pratikspatil024 added 12 commits October 3, 2023 08:59

fix go.mod & go.sum

d257b26

added func HexToRefHash() in erigon-lib

6c446e8

small fix

0a45d29

made optionsAA4337 public

7d77722

added check to check nil account data

8a91bea

renamed OptionsAA4337 to TransactionConditions

cc8ddac

renamed KnownAccounts to KnownAccountStorageConditions and Value to K…

afbfb8b

…nownAccountStorageCondition

renaming of Single/Storage and relevent functions

0326018

inlined 2 constants

f67a504

moved ErrKnownAccounts to func UnmarshalJSON

75f7622

separate length calculation and validation logic of KnownAccountStora…

f6d97dd

…geConditions

moved errors to a better place

220acfd

pratikspatil024 force-pushed the aa-4337 branch from f7b5195 to 220acfd Compare October 3, 2023 09:14

pratikspatil024 added 5 commits October 3, 2023 16:48

moved transaction_conditional to erigon/types

3d28edd

generalised validate block range and timestamp range functions

cc63de0

refactored the 4 validation functions into 1

254e288

made InsertKnownAccounts private for test

c095f93

meaningful variable names

5ae1dd4

pratikspatil024 marked this pull request as draft October 5, 2023 02:54

battlmonstr reviewed Oct 12, 2023

View reviewed changes

battlmonstr suggested changes Oct 12, 2023

View reviewed changes

battlmonstr reviewed Oct 12, 2023

View reviewed changes

pratikspatil024 added 2 commits October 19, 2023 09:41

moved tempAccount check outside switch

a6a66ad

only checking options on the RPC level, removed unwanted code, will c…

3c5c466

…ontinue later

pratikspatil024 marked this pull request as ready for review October 19, 2023 06:00

mh0lt approved these changes Dec 12, 2023

View reviewed changes

battlmonstr approved these changes Dec 13, 2023

View reviewed changes

mh0lt merged commit 1abdcf9 into erigontech:bor_account_abstraction Dec 14, 2023

	return &Value{Single: libcommon.HexToRefHash(hex)}
	return &Value{Single: &libcommon.HexToHash(hex)}

		// put options data in Tx, to use it later while block building
		txn.PutOptions(&options)

	func HexToRefHash(s string) *Hash {
	func HexToHashRef(s string) *Hash {

Conversation

pratikspatil024 commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

battlmonstr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

battlmonstr Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

battlmonstr left a comment

Choose a reason for hiding this comment

Uh oh!

battlmonstr commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pratikspatil024 commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pratikspatil024 commented Oct 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

battlmonstr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pratikspatil024 commented Sep 18, 2023 •

edited

Loading

battlmonstr Oct 12, 2023 •

edited

Loading

battlmonstr commented Sep 22, 2023 •

edited

Loading

pratikspatil024 commented Oct 3, 2023 •

edited

Loading

battlmonstr left a comment •

edited

Loading

battlmonstr Oct 12, 2023 •

edited

Loading

battlmonstr Oct 13, 2023 •

edited

Loading