-
Notifications
You must be signed in to change notification settings - Fork 432
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
Support various eth_call invocations post 1559 #3317
Conversation
… with zero gas price
…with zero gas price
// 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 | ||
|
||
// commitAndRestoreWithoutZeroGasPrice is CallAndRestore without a zero gas price | ||
bool isCommitAndRestoreWithoutZeroGasPrice = commit && restore && ((executionOptions & ExecutionOptions.ZeroGasPrice) == ExecutionOptions.None); |
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.
Can we simplify it in any way?
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 me that it looks more understandable in this form.
[Test] | ||
//Inappropriate test | ||
|
||
/*[Test] |
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.
Why is it commented?
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.
Did you give up the idea of verification of account cleanup, which we discussed?
/// <summary> | ||
/// Zero Gas price | ||
/// </summary> | ||
ZeroGasPrice = 4, |
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.
I would prefer flag SkipGasPricingValidation
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.
or VerifyGasPricing
@@ -80,7 +90,17 @@ private enum ExecutionOptions | |||
|
|||
public void CallAndRestore(Transaction transaction, BlockHeader block, ITxTracer txTracer) | |||
{ | |||
Execute(transaction, block, txTracer, ExecutionOptions.CommitAndRestore); | |||
IReleaseSpec spec = _specProvider.GetSpec(block.Number); |
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.
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?
@@ -214,17 +247,22 @@ private void Execute(Transaction transaction, BlockHeader block, ITxTracer txTra | |||
|
|||
UInt256 senderReservedGasPayment = restore ? UInt256.Zero : (ulong) gasLimit * gasPrice; | |||
|
|||
if (isCommitAndRestoreWithoutZeroGasPrice) | |||
{ |
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 can merge it with the line 248
if (notSystemTransaction) | ||
{ | ||
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)) |
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.
I am confused. Could we introduce variable like verifyGasPricing?
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.
Instead of combination of restore and isCommitAndRestoreWithoutZeroGasPrice
@@ -72,7 +72,7 @@ public interface IJsonRpcConfig : IConfig | |||
|
|||
[ConfigItem( | |||
Description = "Gas limit for eth_call and eth_estimateGas", | |||
DefaultValue = "100000000")] | |||
DefaultValue = "25000000")] |
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 explain why? Is it from eth_call spec?
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.
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[!]
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.
Ok, but you need to change it in JsonRpcConfig.cs too
# Conflicts: # src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs
if (deleteCallerAccount) | ||
{ | ||
_stateProvider.DeleteAccount(tx.SenderAddress ?? throw new InvalidOperationException()); | ||
} |
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.
Maybe this should be put in a finally clause?
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.
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?
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.
yes
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.
ok
…h_call and the_estimateGas and small refactoring
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); | ||
|
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.
Cosmetic: remove whitespaces :)
@@ -80,7 +90,14 @@ private enum ExecutionOptions | |||
|
|||
public void CallAndRestore(Transaction transaction, BlockHeader block, ITxTracer txTracer) | |||
{ | |||
Execute(transaction, block, txTracer, ExecutionOptions.CommitAndRestore); | |||
bool skipGasPricing = _specProvider.GetSpec(block.Number).IsEip1559Enabled |
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.
Do other clients skip validation when we have GasPrice = 0 or when it is set to null in TransactionForRpc?
@@ -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}", |
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 explain why we need to change this error message?
@@ -58,7 +58,7 @@ public async Task Eth_estimateGas_web3_sample_not_enough_gas_system_account() | |||
"{\"gasPrice\":\"0x100000\", \"data\": \"0x70a082310000000000000000000000006c1f09f6271fbe133db38db9c9280307f5d22160\", \"to\": \"0x0d8775f648430679a709e98d2b0cb6250d2887ef\"}"); |
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 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?
/// 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`` |
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.
I think we should resolve this ToDo
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); |
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 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
if ((transactionCall.MaxFeePerGas != null || transactionCall.MaxPriorityFeePerGas != null) && | ||
transactionCall.GasPrice != null) | ||
{ | ||
return ResultWrapper<TResult>.Fail("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified", ErrorCodes.InvalidInput); |
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 could be done in the next PRs, but please remember about other RPC methods like eth_sendTransaction (to verify in geth)
…_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
Fixes #4948
Changes:
Support various eth_call invocations post-1559
Types of changes
Testing
Requires testing
In case you checked yes, did you write tests??
#ethereum/go-ethereum#23027
#ethereum/execution-specs#35