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

Validate finality mroot of Savanna blocks as a part of header validation #1164

Merged
merged 10 commits into from
Feb 13, 2025
Merged

Validate finality mroot of Savanna blocks as a part of header validation #1164

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e44fdb6
Validate action_mroot when block header is valiated
linh2931 Feb 12, 2025
10e07b5
Update fork_with_bad_block_savanna test that corrupts the block by mo…
linh2931 Feb 13, 2025
53ad484
Remove an unnecessary copy of supplied action_mroot
linh2931 Feb 13, 2025
d33169b
Use is_valid() to check presence to avoid threading safety issue
linh2931 Feb 13, 2025
6db0303
Include block ID in error message
linh2931 Feb 13, 2025
ae22ee1
Change to use const auto& supplied_action_mroot instead
linh2931 Feb 13, 2025
386c416
Merge branch 'main' into validate_action_mroot
linh2931 Feb 13, 2025
f037983
Update invalid_action_mroot_test to new way of create_signed_block
linh2931 Feb 13, 2025
3a087e4
Merge branch 'main' into validate_action_mroot
linh2931 Feb 13, 2025
b2608a0
Use consistent name for finality mroot
linh2931 Feb 13, 2025
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
11 changes: 10 additions & 1 deletion libraries/chain/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3971,7 +3971,7 @@ struct controller_impl {
// Called from net-threads. It is thread safe as signed_block is never modified after creation.
// -----------------------------------------------------------------------------
std::optional<qc_t> verify_basic_proper_block_invariants( const block_id_type& id, const signed_block_ptr& b,
const block_header_state& prev ) {
const block_state& prev ) {
assert(b->is_proper_svnn_block());

auto qc_ext_id = quorum_certificate_extension::extension_id();
Expand Down Expand Up @@ -4066,6 +4066,15 @@ struct controller_impl {
"QC is_strong (${s1}) in block extension does not match is_strong_qc (${s2}) in header extension. Block number: ${b}",
("s1", qc_proof.is_strong())("s2", new_qc_claim.is_strong_qc)("b", block_num) );

if (prev.valid) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have missed something here, but this doesn't look thread safe. This can be filled in when validating the block which could be happening at the same time this is running on the net thread. Can you instead check validated and then assert(prev.valid)? Is it correct that prev.valid is set when validated==true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid

spring/libraries/chain/include/eosio/chain/block_state.hpp

Line 77 in e43fd1e

std::optional<valid_t> valid;
is for Finality Merkle Tree while validated

spring/libraries/chain/include/eosio/chain/block_state.hpp

Line 81 in e43fd1e

copyable_atomic<bool> validated{false}; // We have executed the block's trxs and verified that action merkle root (block id) matches.
is mostly for forkbd. They are independent. Since I am using previous block's valid structure, it should be already constructed. I don't see a thread safe problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During the transition to Savanna it is modified here:

spring/libraries/chain/controller.cpp

Line 1385 in 53ad484

// irreversible apply was just done, calculate new_valid here instead of in transition_to_savanna()

spring/libraries/chain/controller.cpp

Line 1406 in 53ad484

new_bsp->valid = prev->new_valid(*new_bsp, *legacy->action_mroot_savanna, new_bsp->strong_digest);

Even outside Savanna transition it is modified here:

spring/libraries/chain/controller.cpp

Line 804 in 53ad484

validating_bsp->valid = bb.parent.new_valid(bhs, action_mroot, validating_bsp->strong_digest);

This can happen while the net threads are accessing prev.valid.

I believe it is correct that valid will be filled in when validated is true. Since validated is atomic you could check that value here instead of valid.

// compute finality mroot using previous block state and new qc claim
auto computed_finality_mroot = prev.get_finality_mroot_claim(new_qc_claim);
auto supplied_action_mroot = b->action_mroot;
EOS_ASSERT( computed_finality_mroot == supplied_action_mroot, block_validate_exception,
"computed finality mroot (${computed}) does not match supplied action mroot ${supplied} by header extension. Block number: ${b}",
("computed", computed_finality_mroot)("supplied", supplied_action_mroot)("b", block_num) );
}

return std::optional{qc_proof};
}

Expand Down
24 changes: 24 additions & 0 deletions unittests/block_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,4 +419,28 @@ BOOST_FIXTURE_TEST_CASE( invalid_qc_claim_block_num_test, validating_tester ) {
}) ;
}

// Verify that a block with an invalid action mroot is rejected
BOOST_FIXTURE_TEST_CASE( invalid_action_mroot_test, tester )
{
// Create a block with transaction
create_account("newacc"_n);
auto b = produce_block();

// Make a copy of the block and corrupt its action mroot
auto copy_b = std::make_shared<signed_block>(b->clone());
copy_b->action_mroot = digest_type::hash("corrupted");

// Re-sign the block
copy_b->producer_signature = get_private_key(config::system_account_name, "active").sign(copy_b->calculate_id());

// Push the block containing corruptted action mroot. It should fail
BOOST_REQUIRE_EXCEPTION(push_block(copy_b),
fc::exception,
[] (const fc::exception &e)->bool {
return e.code() == block_validate_exception::code_value &&
e.to_detail_string().find("computed finality mroot") != std::string::npos &&
e.to_detail_string().find("does not match supplied action mroot") != std::string::npos;
});
}

BOOST_AUTO_TEST_SUITE_END()
Loading