Skip to content

Commit

Permalink
rpcdaemon: fix eth_estimateGas error handling (#1969)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lupin012
lupin012 authored Apr 23, 2024
1 parent 4ac82e9 commit 1f7b484
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 35 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/rpc-integration-tests.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ jobs:
- name: Checkout RPC Tests Repository & Install Requirements
run: |
rm -rf ${{runner.workspace}}/rpc-tests
git -c advice.detachedHead=false clone --depth 1 --branch v0.11.0 https://github.com/erigontech/rpc-tests ${{runner.workspace}}/rpc-tests
git -c advice.detachedHead=false clone --depth 1 --branch v0.12.0 https://github.com/erigontech/rpc-tests ${{runner.workspace}}/rpc-tests
cd ${{runner.workspace}}/rpc-tests
pip3 install -r requirements.txt
Expand Down Expand Up @@ -68,7 +68,7 @@ jobs:
rm -rf ./mainnet/results/
# Run RPC integration test runner via http
python3 ./run_tests.py --continue --blockchain mainnet --jwt ${{runner.workspace}}/silkworm/build/cmd/jwt.hex --display-only-fail --port 8545 -x admin_,eth_mining,eth_getWork,eth_coinbase,eth_createAccessList/test_16.json,engine_,net_,web3_,txpool_,eth_submitWork,eth_submitHashrate,eth_protocolVersion,erigon_nodeInfo --transport_type http,websocket
python3 ./run_tests.py --continue --blockchain mainnet --jwt ${{runner.workspace}}/silkworm/build/cmd/jwt.hex --display-only-fail --port 8545 -x admin_,eth_mining,eth_getWork,eth_coinbase,eth_createAccessList/test_16.json,engine_,net_,web3_,txpool_,eth_submitWork,eth_submitHashrate,eth_protocolVersion,erigon_nodeInfo,eth_estimateGas/test_07.json --transport_type http,websocket
# Capture test runner script exit status
test_exit_status=$?
Expand Down
7 changes: 3 additions & 4 deletions silkworm/rpc/core/estimate_gas_oracle.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ Task<intx::uint256> EstimateGasOracle::estimate_gas(const Call& call, const silk
SILK_DEBUG << "balance for address " << from << ": 0x" << intx::hex(balance);
if (call.value.value_or(0) > balance) {
// TODO(sixtysixter) what is the right error code?
throw EstimateGasException{-1, "insufficient funds for transfer"};
throw EstimateGasException{-32000, "insufficient funds for transfer"};
}
auto available = balance - call.value.value_or(0);
auto allowance = available / gas_price;
Expand Down Expand Up @@ -89,13 +89,12 @@ Task<intx::uint256> EstimateGasOracle::estimate_gas(const Call& call, const silk
EVMExecutor executor{config_, workers_, state};
auto mid = (hi + lo) / 2;
transaction.gas_limit = mid;

result = try_execution(executor, block, transaction);
if (result.success()) {
hi = mid;
} else {
lo = mid;
if (result.pre_check_error == std::nullopt) {
if (result.pre_check_error_code && result.pre_check_error_code != PreCheckErrorCode::kIntrinsicGasTooLow) {
break;
}
}
Expand All @@ -106,7 +105,7 @@ Task<intx::uint256> EstimateGasOracle::estimate_gas(const Call& call, const silk
transaction.gas_limit = hi;
result = try_execution(executor, block, transaction);
SILK_DEBUG << "HI == cap tested again with " << (result.error_code == evmc_status_code::EVMC_SUCCESS ? "succeed" : "failed");
} else if (result.error_code == std::nullopt) {
} else if (!result.pre_check_error_code || result.pre_check_error_code == PreCheckErrorCode::kIntrinsicGasTooLow) {
result.pre_check_error = std::nullopt;
result.error_code = evmc_status_code::EVMC_SUCCESS;
}
Expand Down
84 changes: 67 additions & 17 deletions silkworm/rpc/core/estimate_gas_oracle_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ TEST_CASE("estimate gas") {

SECTION("Call empty, always fails but success in last step") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(16)
.WillOnce(Return(expect_result_fail))
Expand Down Expand Up @@ -138,7 +138,7 @@ TEST_CASE("estimate gas") {

SECTION("Call empty, alternatively fails and succeeds") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(14)
.WillOnce(Return(expect_result_fail))
Expand All @@ -163,7 +163,7 @@ TEST_CASE("estimate gas") {

SECTION("Call empty, alternatively succeeds and fails") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(14)
.WillOnce(Return(expect_result_ok))
Expand All @@ -186,10 +186,36 @@ TEST_CASE("estimate gas") {
CHECK(estimate_gas == 0x6d5e);
}

SECTION("Call empty, alternatively succeeds and fails with intrinsic") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail_pre_check{.pre_check_error = "intrinsic ", .pre_check_error_code = kIntrinsicGasTooLow};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(14)
.WillOnce(Return(expect_result_ok))
.WillOnce(Return(expect_result_fail_pre_check))
.WillOnce(Return(expect_result_ok))
.WillOnce(Return(expect_result_fail_pre_check))
.WillOnce(Return(expect_result_ok))
.WillOnce(Return(expect_result_fail_pre_check))
.WillOnce(Return(expect_result_ok))
.WillOnce(Return(expect_result_fail_pre_check))
.WillOnce(Return(expect_result_ok))
.WillOnce(Return(expect_result_fail_pre_check))
.WillOnce(Return(expect_result_ok))
.WillOnce(Return(expect_result_fail_pre_check))
.WillOnce(Return(expect_result_ok))
.WillRepeatedly(Return(expect_result_fail));
auto result = boost::asio::co_spawn(pool, estimate_gas_oracle.estimate_gas(call, block), boost::asio::use_future);
const intx::uint256& estimate_gas = result.get();

CHECK(estimate_gas == 0x6d5e);
}

SECTION("Call with gas, always fails but succes last step") {
call.gas = kTxGas * 4;
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(17)
.WillOnce(Return(expect_result_fail))
Expand Down Expand Up @@ -229,7 +255,7 @@ TEST_CASE("estimate gas") {

SECTION("Call with gas_price, gas not capped") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
call.gas = kTxGas * 2;
call.gas_price = intx::uint256{10'000};

Expand Down Expand Up @@ -259,7 +285,7 @@ TEST_CASE("estimate gas") {

SECTION("Call with gas_price, gas capped") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
call.gas = kTxGas * 2;
call.gas_price = intx::uint256{40'000};

Expand All @@ -286,7 +312,7 @@ TEST_CASE("estimate gas") {

SECTION("Call with gas_price and value, gas not capped") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
call.gas = kTxGas * 2;
call.gas_price = intx::uint256{10'000};
call.value = intx::uint256{500'000'000};
Expand Down Expand Up @@ -317,7 +343,7 @@ TEST_CASE("estimate gas") {

SECTION("Call with gas_price and value, gas capped") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
call.gas = kTxGas * 2;
call.gas_price = intx::uint256{20'000};
call.value = intx::uint256{500'000'000};
Expand Down Expand Up @@ -365,7 +391,7 @@ TEST_CASE("estimate gas") {
}

SECTION("Call with too high value, exception") {
ExecutionResult expect_result_fail{.pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
call.value = intx::uint256{2'000'000'000};

try {
Expand All @@ -383,17 +409,17 @@ TEST_CASE("estimate gas") {
}

SECTION("Call fail, try exception") {
ExecutionResult expect_result_fail_pre_check{.error_code = 4, .pre_check_error = "intrinsic gas"};
ExecutionResult expect_result_fail{.error_code = 4};
ExecutionResult expect_result_fail_pre_check{.pre_check_error = "insufficient funds", .pre_check_error_code = kInsufficientFunds};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS};
call.gas = kTxGas * 2;
call.gas_price = intx::uint256{20'000};
call.value = intx::uint256{500'000'000};

try {
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(3)
.WillOnce(Return(expect_result_fail_pre_check))
.WillRepeatedly(Return(expect_result_fail));
.WillOnce(Return(expect_result_fail))
.WillRepeatedly(Return(expect_result_fail_pre_check));
auto result = boost::asio::co_spawn(pool, estimate_gas_oracle.estimate_gas(call, block), boost::asio::use_future);
result.get();
CHECK(false);
Expand All @@ -407,18 +433,42 @@ TEST_CASE("estimate gas") {
}

SECTION("Call fail, try exception with data") {
ExecutionResult expect_result_fail_pre_check{.error_code = 4, .pre_check_error = "intrinsic gas"};
auto data = *silkworm::from_hex("2ac3c1d3e24b45c6c310534bc2dd84b5ed576335");
ExecutionResult expect_result_fail{.error_code = 4, .data = data};
ExecutionResult expect_result_fail_pre_check{.pre_check_error = "insufficient funds", .pre_check_error_code = kInsufficientFunds};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_OUT_OF_GAS, .data = data};
call.gas = kTxGas * 2;
call.gas_price = intx::uint256{20'000};
call.value = intx::uint256{500'000'000};

try {
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(3)
.WillOnce(Return(expect_result_fail))
.WillRepeatedly(Return(expect_result_fail_pre_check));
auto result = boost::asio::co_spawn(pool, estimate_gas_oracle.estimate_gas(call, block), boost::asio::use_future);
result.get();
CHECK(false);
} catch (const silkworm::rpc::EstimateGasException&) {
CHECK(true);
} catch (const std::exception&) {
CHECK(false);
} catch (...) {
CHECK(false);
}
}

SECTION("Call fail-EVMC_INVALID_INSTRUCTION, try exception") {
ExecutionResult expect_result_fail_pre_check{.pre_check_error = "insufficient funds", .pre_check_error_code = kInsufficientFunds};
ExecutionResult expect_result_fail{.error_code = evmc_status_code::EVMC_INVALID_INSTRUCTION};
call.gas = kTxGas * 2;
call.gas_price = intx::uint256{20'000};
call.value = intx::uint256{500'000'000};

try {
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(3)
.WillOnce(Return(expect_result_fail_pre_check))
.WillRepeatedly(Return(expect_result_fail));
.WillOnce(Return(expect_result_fail))
.WillRepeatedly(Return(expect_result_fail_pre_check));
auto result = boost::asio::co_spawn(pool, estimate_gas_oracle.estimate_gas(call, block), boost::asio::use_future);
result.get();
CHECK(false);
Expand Down
20 changes: 11 additions & 9 deletions silkworm/rpc/core/evm_executor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,29 +182,31 @@ void EVMExecutor::reset() {
ibs_state_.clear_journal_and_substate();
}

std::optional<std::string> EVMExecutor::pre_check(const EVM& evm, const silkworm::Transaction& txn, const intx::uint256& base_fee_per_gas, const intx::uint128& g0) {
std::optional<EVMExecutor::PreCheckResult> EVMExecutor::pre_check(const EVM& evm, const silkworm::Transaction& txn,
const intx::uint256& base_fee_per_gas, const intx::uint128& g0) {
const evmc_revision rev{evm.revision()};

if (rev >= EVMC_LONDON) {
if (txn.max_fee_per_gas > 0 || txn.max_priority_fee_per_gas > 0) {
if (txn.max_fee_per_gas < base_fee_per_gas) {
const std::string from = address_to_hex(*txn.sender());
return "fee cap less than block base fee: address " + from + ", gasFeeCap: " +
intx::to_string(txn.max_fee_per_gas) + " baseFee: " + intx::to_string(base_fee_per_gas);
std::string error = "fee cap less than block base fee: address " + from + ", gasFeeCap: " +
intx::to_string(txn.max_fee_per_gas) + " baseFee: " + intx::to_string(base_fee_per_gas);
return PreCheckResult{error, PreCheckErrorCode::kFeeCapLessThanBlockFeePerGas};
}

if (txn.max_fee_per_gas < txn.max_priority_fee_per_gas) {
std::string from = address_to_hex(*txn.sender());
std::string error = "tip higher than fee cap: address " + from + ", tip: " + intx::to_string(txn.max_priority_fee_per_gas) + " gasFeeCap: " +
intx::to_string(txn.max_fee_per_gas);
return error;
return PreCheckResult{error, PreCheckErrorCode::kTipHigherThanFeeCap};
}
}
}
if (txn.gas_limit < g0) {
std::string from = address_to_hex(*txn.sender());
std::string error = "intrinsic gas too low: address " + from + ", have " + std::to_string(txn.gas_limit) + ", want " + intx::to_string(g0);
return error;
return PreCheckResult{error, PreCheckErrorCode::kIntrinsicGasTooLow};
}
return std::nullopt;
}
Expand Down Expand Up @@ -238,10 +240,10 @@ ExecutionResult EVMExecutor::call(
const intx::uint128 g0{protocol::intrinsic_gas(txn, rev)};
SILKWORM_ASSERT(g0 <= UINT64_MAX); // true due to the precondition (transaction must be valid)

const auto error = pre_check(evm, txn, base_fee_per_gas, g0);
if (error) {
const auto pre_check_result = pre_check(evm, txn, base_fee_per_gas, g0);
if (pre_check_result) {
Bytes data{};
return {std::nullopt, txn.gas_limit, data, error};
return {std::nullopt, txn.gas_limit, data, pre_check_result->pre_check_error, pre_check_result->pre_check_error_code};
}

intx::uint256 want;
Expand All @@ -258,7 +260,7 @@ ExecutionResult EVMExecutor::call(
Bytes data{};
std::string from = address_to_hex(*txn.sender());
std::string msg = "insufficient funds for gas * price + value: address " + from + " have " + intx::to_string(have) + " want " + intx::to_string(want + txn.value);
return {std::nullopt, txn.gas_limit, data, msg};
return {std::nullopt, txn.gas_limit, data, msg, PreCheckErrorCode::kInsufficientFunds};
}
} else {
ibs_state_.subtract_from_balance(*txn.sender(), want);
Expand Down
18 changes: 15 additions & 3 deletions silkworm/rpc/core/evm_executor.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,19 @@

namespace silkworm::rpc {

enum PreCheckErrorCode {
kIntrinsicGasTooLow,
kInsufficientFunds,
kFeeCapLessThanBlockFeePerGas,
kTipHigherThanFeeCap
};

struct ExecutionResult {
std::optional<int64_t> error_code;
uint64_t gas_left{0};
Bytes data;
std::optional<std::string> pre_check_error;
std::optional<std::string> pre_check_error{std::nullopt};
std::optional<PreCheckErrorCode> pre_check_error_code{std::nullopt};

bool success() const {
return ((error_code == std::nullopt || *error_code == evmc_status_code::EVMC_SUCCESS) && pre_check_error == std::nullopt);
Expand Down Expand Up @@ -119,8 +127,12 @@ class EVMExecutor {
const IntraBlockState& get_ibs_state() { return ibs_state_; }

private:
static std::optional<std::string> pre_check(const EVM& evm, const silkworm::Transaction& txn,
const intx::uint256& base_fee_per_gas, const intx::uint128& g0);
struct PreCheckResult {
std::string pre_check_error;
PreCheckErrorCode pre_check_error_code;
};
static std::optional<PreCheckResult> pre_check(const EVM& evm, const silkworm::Transaction& txn,
const intx::uint256& base_fee_per_gas, const intx::uint128& g0);
uint64_t refund_gas(const EVM& evm, const silkworm::Transaction& txn, uint64_t gas_left, uint64_t gas_refund);

const silkworm::ChainConfig& config_;
Expand Down

0 comments on commit 1f7b484

Please sign in to comment.