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.

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

Conversation

linh2931
Copy link
Member

@linh2931 linh2931 commented Feb 12, 2025

For Savanna blocks, add finality mroot validation to verify_basic_proper_block_invariants() as a part of header validation.

#730 called for validating block ID as as part of block header validation. While investigating the issue, it is discovered that new_qc_claims block_num and finality mroot could be validated during the header validation. new_qc_claim's block_num validation was done by #995. Block ID validation is not possible at header validation stage as new transaction_mroot is not available before transactions are applied.

This PR adds finality mroot validation and a unit test for the validation.

Resolves #730

@linh2931 linh2931 requested review from heifner and greg7mdp February 12, 2025 18:25
@@ -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

std::optional<valid_t> valid;
is for Finality Merkle Tree while validated
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:

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

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

Even outside Savanna transition it is modified here:

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.

…difying confirmed not action_mroot, as action_mroot is checked by block header validation
@linh2931 linh2931 changed the title Validate action_mroot of Savanna blocks when block is received Validate action_mroot of Savanna blocks as a part of header validation Feb 13, 2025
Comment on lines 4064 to 4066
auto computed_finality_mroot = prev.get_finality_mroot_claim(new_qc_claim);
const auto& supplied_action_mroot = b->action_mroot;
EOS_ASSERT( computed_finality_mroot == supplied_action_mroot, block_validate_exception,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably use either finality_mroot or action_mroot, but not both.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will change to use finality_mroot as it is a Savanna thing.

@linh2931 linh2931 changed the title Validate action_mroot of Savanna blocks as a part of header validation Validate finality mroot of Savanna blocks as a part of header validation Feb 13, 2025
@linh2931 linh2931 merged commit d29c1f4 into main Feb 13, 2025
36 checks passed
@linh2931 linh2931 deleted the validate_action_mroot branch February 13, 2025 22:30
@ericpassmore
Copy link
Contributor

Note:start
category: Other
component: Internal
summary: Validate finality mroot of Savanna blocks as a part of header validation.
Note:end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify block id as part of block header validation for savanna blocks
4 participants