Support various eth_call invocations post 1559 by MansurovB-source · Pull Request #3317 · NethermindEth/nethermind

MansurovB-source · 2021-08-16T10:39:20Z

Fixes #4948

Changes:

Support various eth_call invocations post-1559

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update
Code style update (formatting, renaming)
Refactoring (no functional changes, no api changes)
Build related changes
Other (please describe):

Testing

Requires testing

Yes
No

In case you checked yes, did you write tests??

Yes
No

#ethereum/go-ethereum#23027
#ethereum/execution-specs#35

… with zero gas price

…with zero gas price

LukaszRozmej · 2021-08-18T10:11:49Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

            //we commit only after all block is constructed 
+
+            // commitAndRestoreWithoutZeroGasPrice is CallAndRestore without a zero gas price
+            bool isCommitAndRestoreWithoutZeroGasPrice = commit && restore && ((executionOptions & ExecutionOptions.ZeroGasPrice) == ExecutionOptions.None);


Can we simplify it in any way?

It seems to me that it looks more understandable in this form.

…eth_call

MarekM25 · 2021-08-20T07:20:48Z

src/Nethermind/Nethermind.Evm.Test/TransactionProcessorTests.cs

-        [Test]
+        //Inappropriate test
+
+        /*[Test]


Why is it commented?

Did you give up the idea of verification of account cleanup, which we discussed?

MarekM25 · 2021-08-20T07:22:02Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

+            /// <summary>
+            /// Zero Gas price
+            /// </summary>
+            ZeroGasPrice = 4,


I would prefer flag SkipGasPricingValidation

or VerifyGasPricing

MarekM25 · 2021-08-20T07:26:30Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

        public void CallAndRestore(Transaction transaction, BlockHeader block, ITxTracer txTracer)
        {
-            Execute(transaction, block, txTracer, ExecutionOptions.CommitAndRestore);
+            IReleaseSpec spec = _specProvider.GetSpec(block.Number);


Take into account that you will change the behavior of three RPC calls here (eth_call, eth_estimateGas, and eth_createAccessList). Did you verify this behavior with other clients for each of the methods?

MarekM25 · 2021-08-20T07:27:36Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

            UInt256 senderReservedGasPayment = restore ? UInt256.Zero : (ulong) gasLimit * gasPrice;

+            if (isCommitAndRestoreWithoutZeroGasPrice)
+            {


You can merge it with the line 248

MarekM25 · 2021-08-20T07:30:55Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

            {
                UInt256 senderBalance = _stateProvider.GetBalance(caller);
-                if (!restore && ((ulong) intrinsicGas * gasPrice + value > senderBalance || senderReservedGasPayment + value > senderBalance))
+                if ((!restore || isCommitAndRestoreWithoutZeroGasPrice) && ((ulong) intrinsicGas * gasPrice + value > senderBalance || senderReservedGasPayment + value > senderBalance))


I am confused. Could we introduce variable like verifyGasPricing?

Instead of combination of restore and isCommitAndRestoreWithoutZeroGasPrice

MarekM25 · 2021-08-20T07:32:04Z

src/Nethermind/Nethermind.JsonRpc/IJsonRpcConfig.cs

        [ConfigItem(
            Description = "Gas limit for eth_call and eth_estimateGas",
-            DefaultValue = "100000000")]
+            DefaultValue = "25000000")]


Could you explain why? Is it from eth_call spec?

yes, it was in the specification
4.3.1.1 | eth_call MUST consider gas to equal 25 million if the gas parameter is equal to null[!]

Ok, but you need to change it in JsonRpcConfig.cs too

# Conflicts: # src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

LukaszRozmej · 2021-08-31T07:15:22Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

+            if (deleteCallerAccount)
+            {
+                _stateProvider.DeleteAccount(tx.SenderAddress ?? throw new InvalidOperationException());
+            }


Maybe this should be put in a finally clause?

private void QuickFail(Transaction tx, BlockHeader block, ITxTracer txTracer, bool eip658NotEnabled, string? reason, bool deleteCallerAccount = false) { try { block.GasUsed += tx.GasLimit; Address recipient = tx.To ?? ContractAddress.From( tx.SenderAddress ?? Address.Zero, _stateProvider.GetNonce(tx.SenderAddress ?? Address.Zero)); if (txTracer.IsTracingReceipt) { Keccak? stateRoot = null; if (eip658NotEnabled) { _stateProvider.RecalculateStateRoot(); stateRoot = _stateProvider.StateRoot; } txTracer.MarkAsFailed(recipient, tx.GasLimit, Array.Empty<byte>(), reason ?? "invalid", stateRoot); } } finally { if (deleteCallerAccount) { _stateProvider.DeleteAccount(tx.SenderAddress ?? throw new InvalidOperationException()); } } }

Do something like this?

…9 at the same time

…h_call and the_estimateGas and small refactoring

MarekM25 · 2021-09-08T09:52:44Z

src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs

+            string serialized = ctx._test.TestEthRpc("eth_call", ctx._test.JsonSerializer.Serialize(transaction));
+            Assert.AreEqual(2.Ether(), ctx._test.ReadOnlyState.GetBalance(new Address("0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24")));
+            Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"result\":\"0x\",\"id\":67}", serialized);
+


Cosmetic: remove whitespaces :)

MarekM25 · 2021-09-08T09:55:23Z

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

        public void CallAndRestore(Transaction transaction, BlockHeader block, ITxTracer txTracer)
        {
-            Execute(transaction, block, txTracer, ExecutionOptions.CommitAndRestore);
+            bool skipGasPricing = _specProvider.GetSpec(block.Number).IsEip1559Enabled


Do other clients skip validation when we have GasPrice = 0 or when it is set to null in TransactionForRpc?

MarekM25 · 2021-09-08T09:58:01Z

src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EstimateGas.cs

                ctx._test.TestEthRpc("eth_estimateGas", ctx._test.JsonSerializer.Serialize(transaction));
            Assert.AreEqual(
-                "{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32000,\"message\":\"insufficient funds for transfer: address 0x0001020304050607080910111213141516171819\"},\"id\":67}",
+                "{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"insufficient sender balance\"},\"id\":67}",


Could you explain why we need to change this error message?

MarekM25 · 2021-09-08T10:01:49Z

src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EstimateGas.cs

@@ -58,7 +58,7 @@ public async Task Eth_estimateGas_web3_sample_not_enough_gas_system_account()
                "{\"gasPrice\":\"0x100000\", \"data\": \"0x70a082310000000000000000000000006c1f09f6271fbe133db38db9c9280307f5d22160\", \"to\": \"0x0d8775f648430679a709e98d2b0cb6250d2887ef\"}");


It will be nice to add the same tests like it was before but with skipped gas pricing.

One more question: did geth change behavior only for blocks after 1559 or for all blocks?

MarekM25 · 2021-09-08T10:08:17Z

src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs

+        /// 5. After 1559: In geth if you do specify a gas price, that is interpreted as both the max and priority fee
+        /// and your account balance is checked against them + the current base fee.
+        /// </summary>
+        /// TODO maybe needs to be changed to ``geth``


I think we should resolve this ToDo

MarekM25 · 2021-09-08T10:10:02Z

src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs

+            TransactionForRpc transaction = ctx._test.JsonSerializer.Deserialize<TransactionForRpc>(
+                "{\"from\": \"0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24\", \"to\": \"0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24\", \"maxFeePerGas\": \"0x2DA2830C\", \"value\": \"0x2DA2830C\"}");
+            string serialized = ctx._test.TestEthRpc("eth_call", ctx._test.JsonSerializer.Serialize(transaction));
+            Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32015,\"message\":\"VM execution error.\",\"data\":\"insufficient sender balance\"},\"id\":67}", serialized);


Could we return errors like in geth?

max fee per gas less than block base fee

max priority fee per gas higher than max fee per gas

MarekM25 · 2021-09-08T10:11:43Z

src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthModule.TransactionExecutor.cs

+                if ((transactionCall.MaxFeePerGas != null || transactionCall.MaxPriorityFeePerGas != null) &&
+                    transactionCall.GasPrice != null)
+                {
+                    return ResultWrapper<TResult>.Fail("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified", ErrorCodes.InvalidInput);


It could be done in the next PRs, but please remember about other RPC methods like eth_sendTransaction (to verify in geth)

…toring

…_call and the_estimateGas and small refactoring

# Conflicts: # src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs # src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs # src/Nethermind/Nethermind.JsonRpc/JsonRpcConfig.cs

Behruz Mansurov added 10 commits August 6, 2021 18:00

Added new ExecutionOptions for zero gas price eth_call

f5bed91

Changed Call And Restore to separate zero price transactions

4dcc4f4

Changes to "Execute" to identify transactions with commit and restore…

9065595

… with zero gas price

Changes to "Execute" to process transactions with commit and restore …

5574d5a

…with zero gas price

Added a new condition for checking maxFee > maxPriorityFee

ab32915

Fixed tests

8883dd4

Added and fixed tests

6fff2c1

Added description to the tests

7611928

Fixed condition for "insufficient max fee per gas for sender balance"

0a412ff

fixed the reason for the crash of tests

78a20f7

MansurovB-source requested a review from MarekM25 August 16, 2021 15:00

Behruz Mansurov added 3 commits August 17, 2021 10:25

Fixed eth_call tests

03e0b0e

fixed eth_estimateGas tests

d2fc7ba

fixed the default gas limit value for eth_call and eth_estimateGas

0385a18

LukaszRozmej reviewed Aug 18, 2021

View reviewed changes

Behruz Mansurov added 4 commits August 18, 2021 15:07

Added test for checking account balance after successfully executing …

a4829c2

…eth_call

Added test

9d8ac63

Removed unnecessary parts of QuickFail (deleting an account)

c7e8bb2

The inappropriate test was commented out

646834d

MansurovB-source requested a review from LukaszRozmej August 19, 2021 16:25

MarekM25 requested changes Aug 20, 2021

View reviewed changes

Behruz Mansurov added 3 commits August 25, 2021 08:34

Refactoring

394c190

fixed special if-statement for CallAndRestore

7f46a98

fixed the reason for the test failure, refactoring

78c8f41

MansurovB-source requested a review from MarekM25 August 26, 2021 06:58

Merge remote-tracking branch 'origin/master' into changes_in_eth_call

8652d2c

# Conflicts: # src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

LukaszRozmej reviewed Aug 31, 2021

View reviewed changes

Behruz Mansurov added 3 commits September 3, 2021 16:36

small fix

4c62c77

fixup! small fix

c91715e

Behaviour when specifying the gas price and the gas price of type 155…

7fff105

…9 at the same time

Added support for automatic recognition of the transaction type in et…

ac8f321

…h_call and the_estimateGas and small refactoring

MarekM25 reviewed Sep 8, 2021

View reviewed changes

Behruz Mansurov added 5 commits September 13, 2021 16:33

Get-style error output

800b083

Removed support for automatic recognition of the transaction type

9c19612

Fixed eth_call tests

e1993b2

Fixed eth_estimateGas tests

11f7cf4

fixed some tests, added new tests for eth_estimateGas and small refac…

b3b8739

…toring

MansurovB-source requested review from LukaszRozmej and MarekM25 September 14, 2021 10:53

Behruz Mansurov and others added 9 commits September 22, 2021 17:58

Added JsonConverter for Nullable TxType

1e323ed

Added test case

cd29e01

Added tests for Nullable TxTypeConverter (JSON)

f831fe1

Nullable TxType

b257240

fixed outdated tests

0eac807

Added support for automati recognition of the transaction type in eth…

77bdd4d

…_call and the_estimateGas and small refactoring

Fixed tests and add new test cases

5175f3c

Merge branch 'master' into changes_in_eth_call

367ab1b

# Conflicts: # src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs # src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs # src/Nethermind/Nethermind.JsonRpc/JsonRpcConfig.cs

Merged master and changed tests.

5126e27

kjazgar self-assigned this Jan 23, 2022

dceleda mentioned this pull request Dec 1, 2022

Automatic transaction type detection #4949

Open

LukaszRozmej closed this Sep 9, 2024

benaadams deleted the changes_in_eth_call branch March 29, 2025 19:34

		@@ -58,7 +58,7 @@ public async Task Eth_estimateGas_web3_sample_not_enough_gas_system_account()
		"{\"gasPrice\":\"0x100000\", \"data\": \"0x70a082310000000000000000000000006c1f09f6271fbe133db38db9c9280307f5d22160\", \"to\": \"0x0d8775f648430679a709e98d2b0cb6250d2887ef\"}");

Conversation

MansurovB-source commented Aug 16, 2021 • edited by dceleda Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Types of changes

Testing

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MansurovB-source commented Aug 16, 2021 •

edited by dceleda

Loading