Skip to content

Commit

Permalink
rpcdaemon: fix retry logic in eth_estimateGas (#1991)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lupin012
lupin012 authored May 6, 2024
1 parent f32d4d2 commit d468386
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 19 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/rpc-integration-tests.yml
View file
Original file line number Diff line number Diff line change
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,eth_estimateGas/test_07.json --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 --transport_type http,websocket
# Capture test runner script exit status
test_exit_status=$?
Expand Down
1 change: 1 addition & 0 deletions silkworm/rpc/commands/eth_api.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -934,6 +934,7 @@ Task<void> EthereumRpcApi::handle_eth_estimate_gas(const nlohmann::json& request
co_return;
}
const auto latest_block = latest_block_with_hash->block;

StateReader state_reader(cached_database);
rpc::BlockHeaderProvider block_header_provider = [&chain_storage](BlockNum block_number) {
return chain_storage->read_canonical_header(block_number);
Expand Down
23 changes: 13 additions & 10 deletions silkworm/rpc/core/estimate_gas_oracle.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ Task<intx::uint256> EstimateGasOracle::estimate_gas(const Call& call, const silk
SILK_DEBUG << "Evaluate HI with gas in block " << header->gas_limit;
}

intx::uint256 gas_price = call.gas_price.value_or(0);
if (gas_price != 0) {
std::optional<intx::uint256> gas_price = call.gas_price;
if (gas_price && gas_price != 0) {
evmc::address from = call.from.value_or(evmc::address{0});

std::optional<silkworm::Account> account{co_await account_reader_(from, block_number + 1)};
Expand All @@ -59,16 +59,18 @@ Task<intx::uint256> EstimateGasOracle::estimate_gas(const Call& call, const silk
throw EstimateGasException{-32000, "insufficient funds for transfer"};
}
auto available = balance - call.value.value_or(0);
auto allowance = available / gas_price;
auto allowance = available / *gas_price;
SILK_DEBUG << "allowance: " << allowance << ", available: 0x" << intx::hex(available) << ", balance: 0x" << intx::hex(balance);
if (hi > allowance) {
SILK_WARN << "gas estimation capped by limited funds: original " << hi
<< ", balance 0x" << intx::hex(balance)
<< ", sent " << intx::hex(call.value.value_or(0))
<< ", gasprice " << intx::hex(gas_price)
<< ", gasprice " << intx::hex(*gas_price)
<< ", allowance " << allowance;
hi = uint64_t(allowance);
}
} else {
gas_price = block.header.base_fee_per_gas;
}

if (hi > kGasCap) {
Expand All @@ -84,7 +86,7 @@ Task<intx::uint256> EstimateGasOracle::estimate_gas(const Call& call, const silk
auto state = transaction_.create_state(this_executor, tx_database_, storage_, block_number);

ExecutionResult result{evmc_status_code::EVMC_SUCCESS};
silkworm::Transaction transaction{call.to_transaction()};
silkworm::Transaction transaction{call.to_transaction(gas_price)};
while (lo + 1 < hi) {
EVMExecutor executor{config_, workers_, state};
auto mid = (hi + lo) / 2;
Expand All @@ -93,10 +95,11 @@ Task<intx::uint256> EstimateGasOracle::estimate_gas(const Call& call, const silk
if (result.success()) {
hi = mid;
} else {
lo = mid;
if (result.pre_check_error_code && result.pre_check_error_code != PreCheckErrorCode::kIntrinsicGasTooLow) {
break;
result.error_code = evmc_status_code::EVMC_SUCCESS;
return result;
}
lo = mid;
}
}

Expand All @@ -116,7 +119,7 @@ Task<intx::uint256> EstimateGasOracle::estimate_gas(const Call& call, const silk
});

if (!exec_result.success()) {
throw_exception(exec_result, cap);
throw_exception(exec_result);
}
co_return hi;
}
Expand All @@ -125,10 +128,10 @@ ExecutionResult EstimateGasOracle::try_execution(EVMExecutor& executor, const si
return executor.call(block, transaction);
}

void EstimateGasOracle::throw_exception(ExecutionResult& result, uint64_t cap) {
void EstimateGasOracle::throw_exception(ExecutionResult& result) {
if (result.pre_check_error) {
SILK_DEBUG << "result error " << result.pre_check_error.value();
throw EstimateGasException{-1, "gas required exceeds allowance (" + std::to_string(cap) + ")"};
throw EstimateGasException{-32000, *result.pre_check_error};
} else {
auto error_message = result.error_message();
SILK_DEBUG << "result message: " << error_message << ", code " << *result.error_code;
Expand Down
4 changes: 2 additions & 2 deletions silkworm/rpc/core/estimate_gas_oracle.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
namespace silkworm::rpc {

const std::uint64_t kTxGas = 21'000;
const std::uint64_t kGasCap = 25'000'000;
const std::uint64_t kGasCap = 50'000'000;

using BlockHeaderProvider = std::function<Task<std::optional<silkworm::BlockHeader>>(uint64_t)>;
using AccountReader = std::function<Task<std::optional<silkworm::Account>>(const evmc::address&, uint64_t)>;
Expand Down Expand Up @@ -91,7 +91,7 @@ class EstimateGasOracle {
virtual ExecutionResult try_execution(EVMExecutor& executor, const silkworm::Block& _block, const silkworm::Transaction& transaction);

private:
void throw_exception(ExecutionResult& result, uint64_t cap);
void throw_exception(ExecutionResult& result);

const BlockHeaderProvider& block_header_provider_;
const AccountReader& account_reader_;
Expand Down
8 changes: 4 additions & 4 deletions silkworm/rpc/core/estimate_gas_oracle_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ TEST_CASE("estimate gas") {
SECTION("Call gas above allowance, always succeeds, gas capped") {
ExecutionResult expect_result_ok{.error_code = evmc_status_code::EVMC_SUCCESS};
call.gas = kGasCap * 2;
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _)).Times(24).WillRepeatedly(Return(expect_result_ok));
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _)).Times(25).WillRepeatedly(Return(expect_result_ok));
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();

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

try {
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(3)
.Times(2)
.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);
Expand All @@ -442,7 +442,7 @@ TEST_CASE("estimate gas") {

try {
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(3)
.Times(2)
.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);
Expand All @@ -466,7 +466,7 @@ TEST_CASE("estimate gas") {

try {
EXPECT_CALL(estimate_gas_oracle, try_execution(_, _, _))
.Times(3)
.Times(2)
.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);
Expand Down
8 changes: 6 additions & 2 deletions silkworm/rpc/types/call.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ struct Call {
access_list = new_access_list;
}

[[nodiscard]] silkworm::Transaction to_transaction() const {
[[nodiscard]] silkworm::Transaction to_transaction(const std::optional<intx::uint256>& override_gas_price = std::nullopt) const {
silkworm::Transaction txn{};
if (from) {
txn.set_sender(*from);
Expand All @@ -65,7 +65,11 @@ struct Call {
if (gas > kDefaultGasLimit) {
txn.gas_limit = kDefaultGasLimit;
}
if (gas_price) {

if (override_gas_price) {
txn.max_priority_fee_per_gas = override_gas_price.value();
txn.max_fee_per_gas = override_gas_price.value();
} else if (gas_price) {
txn.max_priority_fee_per_gas = gas_price.value();
txn.max_fee_per_gas = gas_price.value();
} else {
Expand Down
80 changes: 80 additions & 0 deletions silkworm/rpc/types/call_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,86 @@ TEST_CASE("create call with gas price", "[rpc][types][call]") {
CHECK(txn.nonce == 1);
}

TEST_CASE("create call without gas price and max_fee_per_gas & max_priority_fee_per_gas not zero", "[rpc][types][call]") {
Call call{
std::nullopt,
std::nullopt,
235, // gas
std::nullopt, // gas_price
18000, // max_priority_fee_per_gas
18000, // max_fee_per_gas
31337, // value
{}, // data
1, // nonce
{},
};
silkworm::Transaction txn = call.to_transaction();
CHECK(txn.gas_limit == 235);
CHECK(txn.max_fee_per_gas == 18000);
CHECK(txn.max_priority_fee_per_gas == 18000);
CHECK(txn.nonce == 1);
}

TEST_CASE("create call without gas price, max_fee_per_gas & max_priority_fee_per_gas", "[rpc][types][call]") {
Call call{
std::nullopt,
std::nullopt,
235, // gas
std::nullopt, // gas_price
std::nullopt, // max_priority_fee_per_gas
std::nullopt, // max_fee_per_gas
31337, // value
{}, // data
1, // nonce
{},
};
silkworm::Transaction txn = call.to_transaction();
CHECK(txn.gas_limit == 235);
CHECK(txn.max_fee_per_gas == 0);
CHECK(txn.max_priority_fee_per_gas == 0);
CHECK(txn.nonce == 1);
}

TEST_CASE("create call without gas price and use gas_overrides", "[rpc][types][call]") {
Call call{
std::nullopt,
std::nullopt,
235, // gas
std::nullopt, // gas_price
std::nullopt, // max_priority_fee_per_gas
std::nullopt, // max_fee_per_gas
31337, // value
{}, // data
1, // nonce
{},
};
silkworm::Transaction txn = call.to_transaction(22000);
CHECK(txn.gas_limit == 235);
CHECK(txn.max_fee_per_gas == 22000);
CHECK(txn.max_priority_fee_per_gas == 22000);
CHECK(txn.nonce == 1);
}

TEST_CASE("create call with gas price and use gas_overrides", "[rpc][types][call]") {
Call call{
std::nullopt,
std::nullopt,
235, // gas
21000, // gas_price
std::nullopt, // max_priority_fee_per_gas
std::nullopt, // max_fee_per_gas
31337, // value
{}, // data
1, // nonce
{},
};
silkworm::Transaction txn = call.to_transaction(22000);
CHECK(txn.gas_limit == 235);
CHECK(txn.max_fee_per_gas == 22000);
CHECK(txn.max_priority_fee_per_gas == 22000);
CHECK(txn.nonce == 1);
}

AccessList access_list{
{0xde0b295669a9fd93d5f28d9ec85e40f4cb697bae_address,
{
Expand Down

0 comments on commit d468386

Please sign in to comment.