Skip to content
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

rpcdaemon: fix eth_estimateGas error handling #1969

Merged
merged 14 commits into from
Apr 23, 2024
Merged

rpcdaemon: fix eth_estimateGas error handling #1969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
406f9c9
fix error handling
lupin012 Apr 15, 2024
2dcc851
fix error handling
lupin012 Apr 15, 2024
3c92afd
fix test
lupin012 Apr 15, 2024
f2ccdf1
use new rpc-version
lupin012 Apr 16, 2024
de28f8e
fix comment review
lupin012 Apr 16, 2024
7d98310
temporary CI to verify single test
lupin012 Apr 16, 2024
957e5a2
save temporary file with cout
lupin012 Apr 16, 2024
87b8c19
save with LOG info to debug CI
lupin012 Apr 16, 2024
50f5b4a
Update evm_executor.cpp
canepat Apr 17, 2024
97c6e97
Update evm_executor.cpp
canepat Apr 17, 2024
84b561c
Update evm_executor.cpp
canepat Apr 17, 2024
b07f05d
restore final impl
lupin012 Apr 17, 2024
1e642fb
exclude temporary estimateGas/test_07.json because with new block ret…
lupin012 Apr 17, 2024
23819f6
modify tests and add new ones
lupin012 Apr 21, 2024
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
4 changes: 2 additions & 2 deletions .github/workflows/rpc-integration-tests.yml
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading