-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
libraries/chain/controller.cpp
Outdated
@@ -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) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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; |
validated
copyable_atomic<bool> validated{false}; // We have executed the block's trxs and verified that action merkle root (block id) matches. |
valid
structure, it should be already constructed. I don't see a thread safe problem.
There was a problem hiding this comment.
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
.
…difying confirmed not action_mroot, as action_mroot is checked by block header validation
libraries/chain/controller.cpp
Outdated
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Note:start |
For Savanna blocks, add
finality mroot
validation toverify_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_claim
sblock_num
andfinality mroot
could be validated during the header validation.new_qc_claim
'sblock_num
validation was done by #995. Block ID validation is not possible at header validation stage as newtransaction_mroot
is not available before transactions are applied.This PR adds
finality mroot
validation and a unit test for the validation.Resolves #730