Skip to content

Support various eth_call invocations post 1559 #3317

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.

Sign up for GitHub

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

Closed
MansurovB-source wants to merge 39 commits into from
Closed

Support various eth_call invocations post 1559 #3317

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
f5bed91
Added new ExecutionOptions for zero gas price eth_call
Aug 6, 2021
4dcc4f4
Changed Call And Restore to separate zero price transactions
Aug 6, 2021
9065595
Changes to "Execute" to identify transactions with commit and restore…
Aug 6, 2021
5574d5a
Changes to "Execute" to process transactions with commit and restore …
Aug 6, 2021
ab32915
Added a new condition for checking maxFee > maxPriorityFee
Aug 6, 2021
8883dd4
Fixed tests
Aug 6, 2021
6fff2c1
Added and fixed tests
Aug 6, 2021
7611928
Added description to the tests
Aug 6, 2021
0a412ff
Fixed condition for "insufficient max fee per gas for sender balance"
Aug 16, 2021
78a20f7
fixed the reason for the crash of tests
Aug 16, 2021
03e0b0e
Fixed eth_call tests
Aug 17, 2021
d2fc7ba
fixed eth_estimateGas tests
Aug 17, 2021
0385a18
fixed the default gas limit value for eth_call and eth_estimateGas
Aug 17, 2021
a4829c2
Added test for checking account balance after successfully executing …
Aug 18, 2021
9d8ac63
Added test
Aug 19, 2021
c7e8bb2
Removed unnecessary parts of QuickFail (deleting an account)
Aug 19, 2021
646834d
The inappropriate test was commented out
Aug 19, 2021
394c190
Refactoring
Aug 25, 2021
7f46a98
fixed special if-statement for CallAndRestore
Aug 25, 2021
78c8f41
fixed the reason for the test failure, refactoring
Aug 25, 2021
8652d2c
Merge remote-tracking branch 'origin/master' into changes_in_eth_call
kjazgar Aug 30, 2021
4c62c77
small fix
Sep 3, 2021
c91715e
fixup! small fix
Sep 3, 2021
7fff105
Behaviour when specifying the gas price and the gas price of type 155…
Sep 6, 2021
ac8f321
Added support for automatic recognition of the transaction type in et…
Sep 7, 2021
800b083
Get-style error output
Sep 13, 2021
9c19612
Removed support for automatic recognition of the transaction type
Sep 13, 2021
e1993b2
Fixed eth_call tests
Sep 13, 2021
11f7cf4
Fixed eth_estimateGas tests
Sep 13, 2021
b3b8739
fixed some tests, added new tests for eth_estimateGas and small refac…
Sep 14, 2021
1e323ed
Added JsonConverter for Nullable TxType
Sep 22, 2021
cd29e01
Added test case
Sep 22, 2021
f831fe1
Added tests for Nullable TxTypeConverter (JSON)
Sep 22, 2021
b257240
Nullable TxType
Sep 22, 2021
0eac807
fixed outdated tests
Sep 22, 2021
77bdd4d
Added support for automati recognition of the transaction type in eth…
Sep 22, 2021
5175f3c
Fixed tests and add new test cases
Sep 22, 2021
367ab1b
Merge branch 'master' into changes_in_eth_call
kjazgar Jan 18, 2022
5126e27
Merged master and changed tests.
kjazgar Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 50 additions & 13 deletions src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,17 @@ private enum ExecutionOptions
/// <summary>
/// Commit and later restore state, use for CallAndRestore
/// </summary>
CommitAndRestore = Commit | Restore
CommitAndRestore = Commit | Restore,

/// <summary>
/// Zero Gas price
/// </summary>
SkipGasPricingValidation = 4,

/// <summary>
/// Commit and restore with zero gas price
/// </summary>
CommitAndRestoreWithSkippingGasPricingValidation = CommitAndRestore | SkipGasPricingValidation
}

public TransactionProcessor(
Expand All @@ -80,7 +90,17 @@ public TransactionProcessor(

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

@MarekM25 MarekM25 Aug 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

spec = new SystemTransactionReleaseSpec(spec);

bool skipGasPricing = spec.IsEip1559Enabled
? (transaction.IsEip1559
? (transaction.MaxFeePerGas.IsZero && transaction.MaxPriorityFeePerGas.IsZero)
: transaction.GasPrice.IsZero)
: transaction.GasPrice.IsZero;

Execute(transaction, block, txTracer,
skipGasPricing ? ExecutionOptions.CommitAndRestoreWithSkippingGasPricingValidation : ExecutionOptions.CommitAndRestore);
}

public void BuildUp(Transaction transaction, BlockHeader block, ITxTracer txTracer)
Expand All @@ -96,7 +116,7 @@ public void Execute(Transaction transaction, BlockHeader block, ITxTracer txTrac
Execute(transaction, block, txTracer, ExecutionOptions.Commit);
}

private void QuickFail(Transaction tx, BlockHeader block, ITxTracer txTracer, bool eip658NotEnabled, string? reason)
private void QuickFail(Transaction tx, BlockHeader block, ITxTracer txTracer, bool eip658NotEnabled, string? reason, bool deleteCallerAccount = false)
{
block.GasUsed += tx.GasLimit;

Expand All @@ -115,6 +135,11 @@ private void QuickFail(Transaction tx, BlockHeader block, ITxTracer txTracer, bo

txTracer.MarkAsFailed(recipient, tx.GasLimit, Array.Empty<byte>(), reason ?? "invalid", stateRoot);
}

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

@LukaszRozmej LukaszRozmej Aug 31, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be put in a finally clause?

@MansurovB-source MansurovB-source Sep 2, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

        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?

@LukaszRozmej LukaszRozmej Sep 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@MansurovB-source MansurovB-source Sep 17, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

}

private void Execute(Transaction transaction, BlockHeader block, ITxTracer txTracer, ExecutionOptions executionOptions)
Expand All @@ -123,10 +148,14 @@ private void Execute(Transaction transaction, BlockHeader block, ITxTracer txTra

// restore is CallAndRestore - previous call, we will restore state after the execution
bool restore = (executionOptions & ExecutionOptions.Restore) != ExecutionOptions.None;

// commit - is for standard execute, we will commit thee state after execution
bool commit = (executionOptions & ExecutionOptions.Commit) != ExecutionOptions.None || eip658NotEnabled;
//!commit - is for build up during block production, we won't commit state after each transaction to support rollbacks
//we commit only after all block is constructed
//we commit only after all block is constructed

bool skipGasPricing = (executionOptions & ExecutionOptions.SkipGasPricingValidation) != ExecutionOptions.None;

bool notSystemTransaction = !transaction.IsSystem();
bool deleteCallerAccount = false;

Expand All @@ -137,9 +166,17 @@ private void Execute(Transaction transaction, BlockHeader block, ITxTracer txTra
}

UInt256 value = transaction.Value;

if (!skipGasPricing && restore && transaction.MaxPriorityFeePerGas > transaction.MaxFeePerGas)
{
TraceLogInvalidTx(transaction, "MAX PRIORITY FEE PER GAS HIGHER THAN MAX FEE PER GAS");
QuickFail(transaction, block, txTracer, eip658NotEnabled, "max priority fee per gas higher than max fee per gas");
return;
}

if (!transaction.TryCalculatePremiumPerGas(block.BaseFeePerGas, out UInt256 premiumPerGas) && !restore)
if (!transaction.TryCalculatePremiumPerGas(block.BaseFeePerGas, out UInt256 premiumPerGas) && !skipGasPricing)
{
// max fee per gas (feeCap) less than block base fee
TraceLogInvalidTx(transaction, "MINER_PREMIUM_IS_NEGATIVE");
QuickFail(transaction, block, txTracer, eip658NotEnabled, "miner premium is negative");
return;
Expand Down Expand Up @@ -212,29 +249,29 @@ private void Execute(Transaction transaction, BlockHeader block, ITxTracer txTra
}
}

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

if (notSystemTransaction)
{
UInt256 senderBalance = _stateProvider.GetBalance(caller);
if (!restore && ((ulong) intrinsicGas * effectiveGasPrice + value > senderBalance || senderReservedGasPayment + value > senderBalance))
if (!skipGasPricing && ((ulong) intrinsicGas * effectiveGasPrice + value > senderBalance || senderReservedGasPayment + value > senderBalance))
{
TraceLogInvalidTx(transaction, $"INSUFFICIENT_SENDER_BALANCE: ({caller})_BALANCE = {senderBalance}");
QuickFail(transaction, block, txTracer, eip658NotEnabled, "insufficient sender balance");
QuickFail(transaction, block, txTracer, eip658NotEnabled, "insufficient sender balance", deleteCallerAccount && restore);
return;
}

if (!restore && spec.IsEip1559Enabled && !transaction.IsServiceTransaction && senderBalance < (UInt256)transaction.GasLimit * transaction.MaxFeePerGas + value)
if (!skipGasPricing && spec.IsEip1559Enabled && !transaction.IsServiceTransaction && senderBalance < (UInt256)transaction.GasLimit * transaction.MaxFeePerGas + value)
{
TraceLogInvalidTx(transaction, $"INSUFFICIENT_MAX_FEE_PER_GAS_FOR_SENDER_BALANCE: ({caller})_BALANCE = {senderBalance}, MAX_FEE_PER_GAS: {transaction.MaxFeePerGas}");
QuickFail(transaction, block, txTracer, eip658NotEnabled, "insufficient MaxFeePerGas for sender balance");
QuickFail(transaction, block, txTracer, eip658NotEnabled, "insufficient MaxFeePerGas for sender balance", deleteCallerAccount && restore);
return;
}

if (transaction.Nonce != _stateProvider.GetNonce(caller))
{
TraceLogInvalidTx(transaction, $"WRONG_TRANSACTION_NONCE: {transaction.Nonce} (expected {_stateProvider.GetNonce(caller)})");
QuickFail(transaction, block, txTracer, eip658NotEnabled, "wrong transaction nonce");
QuickFail(transaction, block, txTracer, eip658NotEnabled, "wrong transaction nonce", deleteCallerAccount && restore);
return;
}

Expand Down Expand Up @@ -327,7 +364,7 @@ private void Execute(Transaction transaction, BlockHeader block, ITxTracer txTra
}
else
{
// tks: there is similar code fo contract creation from init and from CREATE
// tks: there is similar code for contract creation from init and from CREATE
// this may lead to inconsistencies (however it is tested extensively in blockchain tests)
if (transaction.IsContractCreation)
{
Expand Down
12 changes: 6 additions & 6 deletions src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EstimateGas.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public async Task Eth_estimateGas_web3_should_return_insufficient_balance_error(
string serialized =
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}",

@MarekM25 MarekM25 Sep 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

serialized);
ctx._test.ReadOnlyState.AccountExists(someAccount).Should().BeFalse();
}
Expand All @@ -58,7 +58,7 @@ public async Task Eth_estimateGas_web3_sample_not_enough_gas_system_account()
"{\"gasPrice\":\"0x100000\", \"data\": \"0x70a082310000000000000000000000006c1f09f6271fbe133db38db9c9280307f5d22160\", \"to\": \"0x0d8775f648430679a709e98d2b0cb6250d2887ef\"}");

@MarekM25 MarekM25 Sep 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

string serialized =
ctx._test.TestEthRpc("eth_estimateGas", ctx._test.JsonSerializer.Serialize(transaction));
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"result\":\"0x53b8\",\"id\":67}", serialized);
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32000,\"message\":\"insufficient funds for transfer: address 0xfffffffffffffffffffffffffffffffffffffffe\"},\"id\":67}", serialized);
ctx._test.ReadOnlyState.AccountExists(Address.SystemUser).Should().BeFalse();
}

Expand All @@ -72,7 +72,7 @@ public async Task Eth_estimateGas_web3_sample_not_enough_gas_other_account()
"{\"from\":\"0x0001020304050607080910111213141516171819\",\"gasPrice\":\"0x100000\", \"data\": \"0x70a082310000000000000000000000006c1f09f6271fbe133db38db9c9280307f5d22160\", \"to\": \"0x0d8775f648430679a709e98d2b0cb6250d2887ef\"}");
string serialized =
ctx._test.TestEthRpc("eth_estimateGas", ctx._test.JsonSerializer.Serialize(transaction));
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"result\":\"0x53b8\",\"id\":67}", serialized);
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"insufficient sender balance\"},\"id\":67}", serialized);
ctx._test.ReadOnlyState.AccountExists(someAccount).Should().BeFalse();
}

Expand All @@ -86,7 +86,7 @@ public async Task Eth_estimateGas_web3_above_block_gas_limit()
"{\"from\":\"0x0001020304050607080910111213141516171819\",\"gas\":\"0x100000000\",\"gasPrice\":\"0x100000\", \"data\": \"0x70a082310000000000000000000000006c1f09f6271fbe133db38db9c9280307f5d22160\", \"to\": \"0x0d8775f648430679a709e98d2b0cb6250d2887ef\"}");
string serialized =
ctx._test.TestEthRpc("eth_estimateGas", ctx._test.JsonSerializer.Serialize(transaction));
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"result\":\"0x53b8\",\"id\":67}", serialized);
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"insufficient sender balance\"},\"id\":67}", serialized);
ctx._test.ReadOnlyState.AccountExists(someAccount).Should().BeFalse();
}

Expand Down Expand Up @@ -186,15 +186,15 @@ public async Task Estimate_gas_with_gas_pricing()
TransactionForRpc transaction = ctx._test.JsonSerializer.Deserialize<TransactionForRpc>(
"{\"from\": \"0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24\", \"to\": \"0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24\", \"gasPrice\": \"0x10\"}");
string serialized = ctx._test.TestEthRpc("eth_estimateGas", ctx._test.JsonSerializer.Serialize(transaction));
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"result\":\"0x5208\",\"id\":67}", serialized);
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"insufficient sender balance\"},\"id\":67}", serialized);
}

[Test]
public async Task Estimate_gas_without_gas_pricing_after_1559_legacy()
{
using Context ctx = await Context.CreateWithLondonEnabled();
TransactionForRpc transaction = ctx._test.JsonSerializer.Deserialize<TransactionForRpc>(
"{\"from\": \"0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24\", \"to\": \"0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24\", \"gasPrice\": \"0x10\"}");
"{\"from\": \"0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24\", \"to\": \"0x32e4e4c7c5d1cea5db5f9202a9e4d99e56c91a24\"}");
string serialized = ctx._test.TestEthRpc("eth_estimateGas", ctx._test.JsonSerializer.Serialize(transaction));
Assert.AreEqual("{\"jsonrpc\":\"2.0\",\"result\":\"0x5208\",\"id\":67}", serialized);
}
Expand Down
Loading