Validate transactions by consensus rules by m-Peter · Pull Request #323 · onflow/flow-evm-gateway

m-Peter · 2024-06-27T13:44:45Z

Closes: #117

Description

Performs the same validation rules as the Geth TxPool

For contributor use:

Targeted PR against master branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the standards mentioned here.
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

Summary by CodeRabbit

New Features
- Enhanced transaction validation logic with additional parameters and consensus rule checks.
Tests
- Added new test functions for validating consensus rules and transaction scenarios.
- Updated integration tests to use the latest network ID.
Chores
- Introduced constants for transaction size constraints to improve consistency and maintainability.

devbugging · 2024-06-28T17:02:12Z

+	head := &types.Header{
+		Number:   big.NewInt(20_182_324),
+		Time:     uint64(time.Now().Unix()),
+		GasLimit: 30_000_000,
+	}
+	chainConfig := emulator.DefaultChainConfig
+	chainConfig.ChainID = e.config.EVMNetworkID
+	signer := types.MakeSigner(
+		chainConfig,
+		head.Number,
+		head.Time,
+	)
+	opts := &txpool.ValidationOptions{
+		Config: chainConfig,
+		Accept: 0 |
+			1<<types.LegacyTxType |
+			1<<types.AccessListTxType |
+			1<<types.DynamicFeeTxType |
+			1<<types.BlobTxType,
+		MaxSize: models.TxMaxSize,
+		MinTip:  new(big.Int),
+	}


can we move this inside the ValidateConsensusRules

Ideally, this is something that we should have cached somewhere, maybe in our implementation of TxPool.
So that we don't create all these objects for every request to eth_sendRawTransaction.

Let's just have these defined as variables in the models where validate transaction is, not sure why would we need to have them defined in a function.

I have updated this in 4ad00ea .
We can't define them as variable in the top-level scope, as we need access to the Gateway config, for fetching the current EVM Networkd ID (chain ID).

devbugging · 2024-06-28T17:02:24Z

+		MinTip:  new(big.Int),
+	}
+
+	if err := models.ValidateConsensusRules(tx, head, signer, opts); err != nil {


can we call this inside the ValidateTransaction

We could add this logic to the existing ValidateTransaction method. I just introduced a different method, to be able to test it in isolation.

Updated in 4ad00ea

devbugging · 2024-06-28T17:04:14Z

 	return nil
 }
+
+func ValidateConsensusRules(


why is defining this here needed? can't this just be called in the existing ValidateTransaction

I can merge the two functions. I just wanted to keep them apart, so that they can be composed in other parts of the code-base. But maybe that was a premature thinking 👍

Function removed in cb82dc6

devbugging · 2024-06-28T17:04:34Z


 }
+
+func TestValidateConsensusRules(t *testing.T) {


this feels to me like it's testing txpool.ValidateTransaction which is test in go-ethereum so I don't feel like we should add tests here

Unfortunately the test cases on go-ethereum were very little. And because that method is a rather black-box for us, I wanted to make sure that we validate transaction properly, without false positives / false negatives.

Sorry, I don't exactly understand, aren't all this test existing in go-ethereum already? is there any bellow you added yourself? If not, I don't understand why would we want to have them here.

I feel the same, I think this test for Ethereum, we don't need to test this logic.

@sideninja @ramtinms These tests do not exist in the go-ethereum repository.
The rationale behind them is not to test the logic of txpool.ValidateTransaction(tx, head, signer, opts) .
I have added these tests mainly as a safeguard, against future upgrades of the go-ethereum package.
And the reason is that this method accepts several parameters.
Suppose that the method is updated and makes use of extra fields from the head or opts arguments.
Without the tests, any such changes will go unnoticed, and might result in crashes on production.

head := &types.Header{ Number: big.NewInt(20_182_324), Time: uint64(time.Now().Unix()), GasLimit: 30_000_000, }

For example, right now we only populate the above fields for the head argument.

ramtinms · 2024-07-01T19:20:51Z

+	chainConfig := emulator.DefaultChainConfig
+	chainConfig.ChainID = e.config.EVMNetworkID


maybe just rewrite this to this way
emulator.NewConfig(WithChainID(chainID))

Good idea 👌
Updated in 73261f7

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 592a7d9 and 73261f7.

Files selected for processing (1)

services/requester/requester.go (2 hunks)

Files skipped from review as they are similar to previous changes (1)

services/requester/requester.go

coderabbitai

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 73261f7 and 4ad00ea.

Files selected for processing (3)

models/transaction.go (3 hunks)
models/transaction_test.go (3 hunks)
services/requester/requester.go (4 hunks)

Files not reviewed due to errors (1)

models/transaction_test.go (no review received)

Files skipped from review as they are similar to previous changes (1)

models/transaction.go

Additional comments not posted (1)

services/requester/requester.go (1)

194-194: Ensure error handling for validation.

The validation logic is a good addition. Ensure that the error messages provide sufficient context for debugging.

coderabbitai · 2024-07-02T08:08:48Z

+	head := &types.Header{
+		Number:   big.NewInt(20_182_324),
+		Time:     uint64(time.Now().Unix()),
+		GasLimit: 30_000_000,
+	}
+	emulatorConfig := emulator.NewConfig(
+		emulator.WithChainID(config.EVMNetworkID),
+		emulator.WithBlockNumber(head.Number),
+		emulator.WithBlockTime(head.Time),
+	)
+	evmSigner := emulator.GetSigner(emulatorConfig)
+	validationOptions := &txpool.ValidationOptions{
+		Config: emulatorConfig.ChainConfig,
+		Accept: 0 |
+			1<<types.LegacyTxType |
+			1<<types.AccessListTxType |
+			1<<types.DynamicFeeTxType |
+			1<<types.BlobTxType,
+		MaxSize: models.TxMaxSize,
+		MinTip:  new(big.Int),
+	}


Ensure head values are configurable.

The head values such as Number, Time, and GasLimit are hard-coded. Consider making these configurable to enhance flexibility and testing.

head := &types.Header{ Number: big.NewInt(config.BlockNumber), Time: uint64(time.Now().Unix()), GasLimit: config.GasLimit, }

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4ad00ea and cb82dc6.

Files selected for processing (2)

models/transaction.go (3 hunks)
models/transaction_test.go (3 hunks)

Files skipped from review as they are similar to previous changes (2)

models/transaction.go
models/transaction_test.go

devbugging · 2024-07-02T16:28:21Z

+	head := &types.Header{
+		Number:   big.NewInt(20_182_324),
+		Time:     uint64(time.Now().Unix()),
+		GasLimit: 30_000_000,
+	}
+	emulatorConfig := emulator.NewConfig(
+		emulator.WithChainID(config.EVMNetworkID),
+		emulator.WithBlockNumber(head.Number),
+		emulator.WithBlockTime(head.Time),
+	)
+	evmSigner := emulator.GetSigner(emulatorConfig)
+	validationOptions := &txpool.ValidationOptions{
+		Config: emulatorConfig.ChainConfig,
+		Accept: 0 |
+			1<<types.LegacyTxType |
+			1<<types.AccessListTxType |
+			1<<types.DynamicFeeTxType |
+			1<<types.BlobTxType,
+		MaxSize: models.TxMaxSize,
+		MinTip:  new(big.Int),
+	}


why do we want to have these config here in the requester? I think it would be better if we just place it in the models/transaction with the ValidateTransaction function on the top as a variable

Because we need access to config.EVMNetworkID. This means that even if we pass the config as a parameter, we will have to construct all these objects every time we call the ValidateTransaction function.

devbugging · 2024-07-02T16:42:53Z

+	)
+	evmSigner := emulator.GetSigner(emulatorConfig)
+	validationOptions := &txpool.ValidationOptions{
+		Config: emulatorConfig.ChainConfig,


Why not use https://github.com/onflow/flow-go/blob/fc7c03c2c524fa8881bc9e3b4cf53b6b4f471dc3/fvm/evm/emulator/config.go#L51

It was suggested by Ramtin (#323 (comment)), to construct a new config, with the desired options applied. This also facilitates fetching the signer, based on the created config. Note that we do not need the default config for all networks, as the default config is hardcoded to PreviewNet.

…ansaction method

…saction function

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d5906af and cf8bc7d.

Files selected for processing (2)

models/transaction.go (3 hunks)
models/transaction_test.go (3 hunks)

Files skipped from review as they are similar to previous changes (2)

models/transaction.go
models/transaction_test.go

m-Peter self-assigned this Jun 27, 2024

This comment was marked as outdated.

Sign in to view

m-Peter force-pushed the consensus-rules-tx-validation branch 2 times, most recently from e4d094d to ffc4131 Compare June 27, 2024 15:34

m-Peter changed the title ~~WIP~~ Validate transactions by consensus rules Jun 27, 2024

m-Peter added Improvement EVM labels Jun 27, 2024

m-Peter added this to the Flow-EVM-M2 milestone Jun 27, 2024

m-Peter force-pushed the consensus-rules-tx-validation branch from ffc4131 to 0e712e1 Compare June 28, 2024 11:13

m-Peter marked this pull request as ready for review June 28, 2024 11:17

m-Peter requested review from devbugging and peterargue as code owners June 28, 2024 11:17

m-Peter commented Jun 28, 2024

View reviewed changes

Comment thread services/requester/requester.go Outdated

m-Peter commented Jun 28, 2024

View reviewed changes

Comment thread tests/integration_test.go

m-Peter commented Jun 28, 2024

View reviewed changes

Comment thread tests/integration_test.go

m-Peter requested a review from ramtinms June 28, 2024 11:24

This comment was marked as outdated.

Sign in to view

devbugging reviewed Jun 28, 2024

View reviewed changes

m-Peter force-pushed the consensus-rules-tx-validation branch from 0e712e1 to afa0980 Compare July 1, 2024 12:31

This comment was marked as outdated.

Sign in to view

ramtinms reviewed Jul 1, 2024

View reviewed changes

m-Peter force-pushed the consensus-rules-tx-validation branch from afa0980 to 592a7d9 Compare July 2, 2024 06:18

This comment was marked as outdated.

Sign in to view

coderabbitai Bot reviewed Jul 2, 2024

View reviewed changes

m-Peter requested a review from devbugging July 2, 2024 08:46

m-Peter requested a review from ramtinms July 2, 2024 08:46

devbugging reviewed Jul 2, 2024

View reviewed changes

m-Peter added 5 commits July 2, 2024 19:56

Add the ValidateConsensusRules method and related unit tests

e79622c

Add a basic wiring of ValidateConsensusRules in requester's SendRawTr…

ffea4d4

…ansaction method

Improve EVM Chain config and signer initialization

823ae09

Call txpool.ValidateTransaction from the existing models.ValidateTran…

7d0dbcc

…saction function

Remove ValidateConsensusRules wrapper function

d5906af

m-Peter force-pushed the consensus-rules-tx-validation branch from cb82dc6 to d5906af Compare July 2, 2024 16:59

This comment was marked as outdated.

Sign in to view

devbugging approved these changes Jul 3, 2024

View reviewed changes

Merge branch 'main' into consensus-rules-tx-validation

cf8bc7d

coderabbitai Bot reviewed Jul 3, 2024

View reviewed changes

m-Peter merged commit 5127995 into main Jul 3, 2024

m-Peter deleted the consensus-rules-tx-validation branch July 3, 2024 15:24

coderabbitai Bot mentioned this pull request Sep 27, 2024

Fix Genesis block hash calculation for Testnet network #566

Merged

6 tasks

coderabbitai Bot mentioned this pull request Dec 18, 2024

Local state transaction re-execution #712

Merged

6 tasks

coderabbitai Bot mentioned this pull request Jan 28, 2025

Increase computation limit for Cadence tx #736

Merged

6 tasks

coderabbitai Bot mentioned this pull request Feb 17, 2025

Consolidate the validations for max TX gas limit #759

Merged

6 tasks

coderabbitai Bot mentioned this pull request May 8, 2025

Create ValidationOptions for each tx submission #818

Merged

6 tasks

coderabbitai Bot mentioned this pull request Aug 28, 2025

Track & apply surge-factor pricing changes #867

Open

6 tasks

		chainConfig := emulator.DefaultChainConfig
		chainConfig.ChainID = e.config.EVMNetworkID

Conversation

m-Peter commented Jun 27, 2024 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary by CodeRabbit

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devbugging Jul 1, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

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!

This comment was marked as outdated.

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

m-Peter commented Jun 27, 2024 •

edited by coderabbitai Bot

Loading

devbugging Jul 1, 2024 •

edited

Loading