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

node: fix Execution stage unwind after invalid block #2360

Merged
merged 2 commits into from
Sep 18, 2024
Merged

node: fix Execution stage unwind after invalid block #2360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d112b97
node: fix Execution stage unwind after invalid block
canepat Sep 17, 2024
60ff579
node: fix invalid block lifetime in Execution stage
canepat Sep 17, 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
18 changes: 13 additions & 5 deletions silkworm/node/stagedsync/stages/stage_execution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,9 @@ Stage::Result Execution::forward(db::RWTxn& txn) {
const auto execution_result{execute_batch(txn, max_block_num, analysis_cache, state_pool,
prune_history, prune_receipts, prune_call_traces)};

// If we return with success we must persist data
if (execution_result != Stage::Result::kSuccess) {
// If we return with success we must persist data. Though counterintuitive, we must also persist on
// kInvalidBlock to save good progress done so far: the subsequent unwind will remove last invalid updates
if (execution_result != Stage::Result::kSuccess && execution_result != Stage::Result::kInvalidBlock) {
throw StageError(execution_result);
}

Expand All @@ -119,6 +120,11 @@ Stage::Result Execution::forward(db::RWTxn& txn) {
auto [_, duration]{commit_stopwatch.stop()};
log::Info(log_prefix_ + " commit", {"batch time", StopWatch::format(duration)});

// If we got an invalid block, now after persisting we can exit
if (execution_result == Stage::Result::kInvalidBlock) {
ret = execution_result;
break;
}
block_num_++;
}

Expand Down Expand Up @@ -235,12 +241,12 @@ Stage::Result Execution::execute_batch(db::RWTxn& txn, BlockNum max_block_num, A
execution::block::BlockExecutor block_executor{&chain_config_, write_receipts, write_traces, kWriteChangeSets};
try {
if (const ValidationResult res = block_executor.execute_single(block, buffer, analysis_cache, state_pool); res != ValidationResult::kOk) {
// Persist work done so far
// Flush work done so far not to lose progress up to the previous valid block and to correctly trigger unwind
// This requires to commit in Execution::forward also for kInvalidBlock: unwind will remove last invalid block updates
if (write_receipts) {
buffer.insert_receipts(block_num_, receipts);
}
buffer.write_to_db();
prefetched_blocks_.clear();

// Notify sync_loop we need to unwind
sync_context_->unwind_point.emplace(block_num_ - 1u);
Expand All @@ -251,6 +257,8 @@ Stage::Result Execution::execute_batch(db::RWTxn& txn, BlockNum max_block_num, A
{"block", std::to_string(block_num_),
"hash", to_hex(block.header.hash().bytes, true),
"error", std::string(magic_enum::enum_name<ValidationResult>(res))});

prefetched_blocks_.clear(); // Must stay here to keep `block` reference valid
return Result::kInvalidBlock;
}

Expand Down Expand Up @@ -560,7 +568,7 @@ void Execution::revert_state(ByteView key, ByteView value, db::RWCursorDupSort&
Bytes code_hash_key(kAddressLength + db::kIncarnationLength, '\0');
std::memcpy(&code_hash_key[0], &key[0], kAddressLength);
endian::store_big_u64(&code_hash_key[kAddressLength], account.incarnation);
auto new_code_hash{plain_code_table.find(db::to_slice(code_hash_key))};
auto new_code_hash = plain_code_table.find(db::to_slice(code_hash_key));
std::memcpy(&account.code_hash.bytes[0], new_code_hash.value.data(), kHashLength);
}
// cleaning up contract codes
Expand Down
85 changes: 79 additions & 6 deletions silkworm/node/stagedsync/stages_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,8 @@ TEST_CASE("Sync Stages") {
REQUIRE(written_senders.empty());
}

// TODO(canepat) refactor these tests to use Execution stage instead of mimicking it

SECTION("Execution and HashState") {
using namespace magic_enum;
using StageResult = stagedsync::Stage::Result;
Expand All @@ -313,7 +315,7 @@ TEST_CASE("Sync Stages") {
// ---------------------------------------

uint64_t block_number{1};
auto miner{0x5a0b54d5dc17e0aadc383d2db43b0a0d3e029c4c_address};
const auto miner{0x5a0b54d5dc17e0aadc383d2db43b0a0d3e029c4c_address};

Block block{};
block.header.number = block_number;
Expand All @@ -327,8 +329,7 @@ TEST_CASE("Sync Stages") {
};
block.header.receipts_root = trie::root_hash(receipts, kEncoder);

// This contract initially sets its 0th storage to 0x2a
// and its 1st storage to 0x01c9.
// This contract initially sets its 0th storage to 0x2a and its 1st storage to 0x01c9.
// When called, it updates its 0th storage to the input provided.
Bytes contract_code{*from_hex("600035600055")};
Bytes deployment_code{*from_hex("602a6000556101c960015560068060166000396000f3") + contract_code};
Expand All @@ -351,7 +352,10 @@ TEST_CASE("Sync Stages") {
// ---------------------------------------
// Execute first block
// ---------------------------------------
auto actual_validation_result = execute_block(block, buffer, node_settings.chain_config.value());
std::vector<Receipt> block_receipts;
auto actual_validation_result = execute_block(block, buffer, node_settings.chain_config.value(), block_receipts);
// We must insert receipts to mimic the behaviour of Execution stage
buffer.insert_receipts(block_number, block_receipts);
// We need double parentheses here: https://github.com/conan-io/conan-center-index/issues/13993
REQUIRE((enum_name(actual_validation_result) == enum_name(ValidationResult::kOk)));
auto contract_address{create_address(sender, /*nonce=*/0)};
Expand All @@ -372,7 +376,10 @@ TEST_CASE("Sync Stages") {
block.transactions[0].to = contract_address;
block.transactions[0].data = ByteView(new_val);

actual_validation_result = execute_block(block, buffer, node_settings.chain_config.value());
block_receipts.clear();
actual_validation_result = execute_block(block, buffer, node_settings.chain_config.value(), block_receipts);
// We must insert receipts to mimic the behaviour of Execution stage
buffer.insert_receipts(block_number, block_receipts);
// We need double parentheses here: https://github.com/conan-io/conan-center-index/issues/13993
REQUIRE((enum_name(actual_validation_result) == enum_name(ValidationResult::kOk)));

Expand All @@ -389,13 +396,28 @@ TEST_CASE("Sync Stages") {
block.transactions[0].to = contract_address;
block.transactions[0].data = ByteView{new_val};

actual_validation_result = execute_block(block, buffer, node_settings.chain_config.value());
block_receipts.clear();
actual_validation_result = execute_block(block, buffer, node_settings.chain_config.value(), block_receipts);
// We must insert receipts to mimic the behaviour of Execution stage
buffer.insert_receipts(block_number, block_receipts);
// We need double parentheses here: https://github.com/conan-io/conan-center-index/issues/13993
REQUIRE((enum_name(actual_validation_result) == enum_name(ValidationResult::kOk)));
REQUIRE_NOTHROW(buffer.write_to_db());
REQUIRE_NOTHROW(db::stages::write_stage_progress(txn, db::stages::kExecutionKey, 3));
REQUIRE_NOTHROW(txn.commit_and_renew());

// Check state after 3rd block in database
auto plain_state_cursor = txn.ro_cursor(db::table::kPlainState);
REQUIRE(plain_state_cursor->seek(db::to_slice(sender)));
auto current_record = plain_state_cursor->current(/*throw_notfound=*/false);
REQUIRE(current_record);
REQUIRE(db::from_slice(current_record.key) == ByteView{sender.bytes});
auto decoded_account = Account::from_encoded_storage(db::from_slice(current_record.value));
REQUIRE(decoded_account);
REQUIRE(decoded_account->nonce == 3);
auto receipts_cursor = txn.ro_cursor(db::table::kBlockReceipts);
REQUIRE(receipts_cursor->seek(db::to_slice(db::block_key(3))));

SECTION("Execution Unwind") {
// ---------------------------------------
// Unwind 3rd block and checks if state is second block
Expand All @@ -422,6 +444,57 @@ TEST_CASE("Sync Stages") {
evmc::bytes32 storage_key0{};
evmc::bytes32 storage0{buffer2.read_storage(contract_address, kDefaultIncarnation, storage_key0)};
CHECK(storage0 == 0x000000000000000000000000000000000000000000000000000000000000003e_bytes32);

// Check state after unwind of the 3rd block in database
plain_state_cursor = txn.ro_cursor(db::table::kPlainState);
REQUIRE(plain_state_cursor->seek(db::to_slice(sender)));
current_record = plain_state_cursor->current(/*throw_notfound=*/false);
REQUIRE(current_record);
REQUIRE(db::from_slice(current_record.key) == ByteView{sender.bytes});
decoded_account = Account::from_encoded_storage(db::from_slice(current_record.value));
REQUIRE(decoded_account);
REQUIRE(decoded_account->nonce == 2);
receipts_cursor = txn.ro_cursor(db::table::kBlockReceipts);
REQUIRE(receipts_cursor->seek(db::to_slice(db::block_key(2))));
}

SECTION("Execution failure and unwind") {
// Execute INVALID 4th block
block_number = 4;
block.header.number = block_number;
block.transactions[0].nonce = 2; // <- invalid, should be 3
block.transactions[0].value = 1000;
block.transactions[0].to = contract_address;

block_receipts.clear();
actual_validation_result = execute_block(block, buffer, node_settings.chain_config.value(), block_receipts);

// Execution stage *must* flush state+progress updates and commit even in case of kInvalidBlock error
REQUIRE_NOTHROW(buffer.insert_receipts(block_number, block_receipts));
REQUIRE_NOTHROW(buffer.write_to_db());
REQUIRE_NOTHROW(db::stages::write_stage_progress(txn, db::stages::kExecutionKey, 4)); // this was missing after PR #1511
txn.commit_and_renew(); // this was missing after PR #1511

// We need double parentheses here: https://github.com/conan-io/conan-center-index/issues/13993
REQUIRE((enum_name(actual_validation_result) == enum_name(ValidationResult::kWrongNonce)));

// Unwind 4th block and checks if state corresponds to 3rd block
stagedsync::SyncContext sync_context{};
sync_context.unwind_point.emplace(3);
stagedsync::Execution stage = make_execution_stage(&sync_context, node_settings);
REQUIRE(stage.unwind(txn) == stagedsync::Stage::Result::kSuccess);

plain_state_cursor = txn.ro_cursor(db::table::kPlainState);
REQUIRE(plain_state_cursor->seek(db::to_slice(sender)));
current_record = plain_state_cursor->current(/*throw_notfound=*/false);
REQUIRE(current_record);
REQUIRE(db::from_slice(current_record.key) == ByteView{sender.bytes});
decoded_account = Account::from_encoded_storage(db::from_slice(current_record.value));
REQUIRE(decoded_account);
REQUIRE(decoded_account->nonce == 3);
receipts_cursor = txn.ro_cursor(db::table::kBlockReceipts);
REQUIRE(receipts_cursor->seek(db::to_slice(db::block_key(3))));
REQUIRE(!receipts_cursor->seek(db::to_slice(db::block_key(4)))); // <- this fails after PR #1511
}

SECTION("Execution Prune Default") {
Expand Down
Loading