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

IF: Further work of validating QC claim #2139

Merged
merged 4 commits into from
Jan 25, 2024
Merged

IF: Further work of validating QC claim #2139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5dd4a51
move integrate_received_qc_to_block to push_block so it is thread saf…
linh2931 Jan 25, 2024
6f33e26
use previous ID to search claimed ID
linh2931 Jan 25, 2024
530c66f
reword thread safe comments to make them accurate
linh2931 Jan 25, 2024
995d075
Merge branch 'hotstuff_integration' into reject_invalid_qc
linh2931 Jan 25, 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
2 changes: 1 addition & 1 deletion libraries/chain/block_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ void block_state::verify_qc(const valid_quorum_certificate& qc) const {
digests.emplace_back(std::vector<uint8_t>{weak_digest.data(), weak_digest.data() + weak_digest.data_size()});
}

// validate aggregayed signature
// validate aggregated signature
EOS_ASSERT( fc::crypto::blslib::aggregate_verify( pubkeys, digests, qc._sig ), block_validate_exception, "signature validation failed" );
}

Expand Down
88 changes: 56 additions & 32 deletions libraries/chain/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,6 +1005,7 @@ struct controller_impl {
});
}

// search on the branch of head
block_state_ptr fork_db_fetch_bsp_by_num(uint32_t block_num) const {
return fork_db.apply<block_state_ptr>(
overloaded{
Expand All @@ -1017,6 +1018,19 @@ struct controller_impl {
);
}

// search on the branch of given id
block_state_ptr fork_db_fetch_bsp_by_num(const block_id_type& id, uint32_t block_num) const {
return fork_db.apply<block_state_ptr>(
overloaded{
[](const fork_database_legacy_t&) -> block_state_ptr { return nullptr; },
arhag marked this conversation as resolved.
Show resolved Hide resolved
[&](const fork_database_if_t&forkdb) -> block_state_ptr {
auto bsp = forkdb.search_on_branch(id, block_num);
return bsp;
}
}
);
}

void pop_block() {
uint32_t prev_block_num = fork_db.apply<uint32_t>([&](auto& forkdb) {
return pop_block(forkdb);
Expand Down Expand Up @@ -3033,30 +3047,31 @@ struct controller_impl {
return block_handle{bsp};
}

void integrate_received_qc_to_block(const block_id_type& id, const signed_block_ptr& b) {
// expected to be called from application thread as it modifies bsp->valid_qc,
void integrate_received_qc_to_block(const block_state_ptr& bsp_in) {
// extract QC from block extension
const auto& block_exts = b->validate_and_extract_extensions();
const auto& block_exts = bsp_in->block->validate_and_extract_extensions();
if( block_exts.count(quorum_certificate_extension::extension_id()) == 0 ) {
return;
}
const auto& qc_ext = std::get<quorum_certificate_extension>(block_exts. lower_bound(quorum_certificate_extension::extension_id())->second);
const auto& new_qc = qc_ext.qc.qc;
const auto& received_qc = qc_ext.qc.qc;

const auto bsp = fork_db_fetch_bsp_by_num( qc_ext.qc.block_height );
const auto bsp = fork_db_fetch_bsp_by_num( bsp_in->previous(), qc_ext.qc.block_height );
if( !bsp ) {
return;
}

// Don't save the QC from block extension if the claimed block has a better valid_qc.
if (bsp->valid_qc && (bsp->valid_qc->is_strong() || new_qc.is_weak())) {
if (bsp->valid_qc && (bsp->valid_qc->is_strong() || received_qc.is_weak())) {
return;
}

// save the QC
bsp->valid_qc = new_qc;
// Save the QC. This is safe as the function is called by push_block from application thread.
bsp->valid_qc = received_qc;

// advance LIB if QC is strong and final_on_strong_qc_block_num has value
if( new_qc.is_strong() && bsp->core.final_on_strong_qc_block_num ) {
if( received_qc.is_strong() && bsp->core.final_on_strong_qc_block_num ) {
// We evaluate a block extension qc and advance lib if strong.
// This is done before evaluating the block. It is possible the block
// will not be valid or forked out. This is safe because the block is
Expand All @@ -3070,7 +3085,7 @@ struct controller_impl {
// and quorum_certificate_extension in block extension are valid.
// Called from net-threads. It is thread safe as signed_block is never modified
// after creation.
void verify_qc_claim( const signed_block_ptr& b, const block_header_state& prev ) {
void verify_qc_claim( const block_id_type& id, const signed_block_ptr& b, const block_header_state& prev ) {
// extract current block extension and previous header extension
auto block_exts = b->validate_and_extract_extensions();
std::optional<block_header_extension> prev_header_ext = prev.header.extract_header_extension(instant_finality_extension::extension_id());
Expand All @@ -3079,13 +3094,15 @@ struct controller_impl {
if( !header_ext ) {
EOS_ASSERT( block_exts.count(quorum_certificate_extension::extension_id()) == 0,
block_validate_exception,
"A block must have QC header extension if it provides QC block extension." );
"A block must have QC header extension if it provides QC block extension. Block number: ${b}",
("b", b->block_num()) );

// If the previous block has the QC header extension,
// then the current block must also have the header extension.
EOS_ASSERT( !prev_header_ext,
block_validate_exception,
"A block must have QC header extension because its previous block has the extension" );
"A block must have QC header extension because its previous block has the extension. Block number: ${b}",
("b", b->block_num()) );

// If header extension does not have instant_finality_extension,
// do not continue.
Expand All @@ -3096,7 +3113,8 @@ struct controller_impl {
if( !if_ext.qc_info ) {
EOS_ASSERT( block_exts.count(quorum_certificate_extension::extension_id()) == 0,
block_validate_exception,
"A block must have QC claim if it provides QC block extension" );
"A block must have QC claim if it provides QC block extension. Block number: ${b}",
("b", b->block_num()) );

// If header extension does not have QC claim,
// do not continue.
Expand All @@ -3110,7 +3128,8 @@ struct controller_impl {
// is prior to the transition to IF.
EOS_ASSERT( prev_header_ext,
block_validate_exception,
"Previous header extension must include instant_finality_extension " );
"Previous header extension must include instant_finality_extension. Block number: ${b}",
("b", b->block_num()) );

auto prev_if_ext = std::get<instant_finality_extension>(*prev_header_ext);
auto prev_qc_info = prev_if_ext.qc_info;
Expand All @@ -3120,15 +3139,16 @@ struct controller_impl {
// new claimed QC block nubmber cannot be smaller than previous block's
EOS_ASSERT( qc_claim.last_qc_block_num >= prev_qc_info->last_qc_block_num,
block_validate_exception,
"claimed last_qc_block_num (${n1}) must be equal to or greater than previous block's last_qc_block_num (${n2})",
("n1", qc_claim.last_qc_block_num)("n2", prev_qc_info->last_qc_block_num) );
"claimed last_qc_block_num (${n1}) must be equal to or greater than previous block's last_qc_block_num (${n2}). Block number: ${b}",
("n1", qc_claim.last_qc_block_num)("n2", prev_qc_info->last_qc_block_num)("b", b->block_num()) );

if( qc_claim.last_qc_block_num == prev_qc_info->last_qc_block_num ) {
if( qc_claim.is_last_qc_strong == prev_qc_info->is_last_qc_strong ) {
// QC block extension is redundant
EOS_ASSERT( block_exts.count(quorum_certificate_extension::extension_id()) == 0,
block_validate_exception,
"A block should not provide QC block extension if QC claim is the same as previous block" );
"A block should not provide QC block extension if QC claim is the same as previous block. Block number: ${b}",
("b", b->block_num()) );

// if previous block's header extension has the same claim, just return
// (previous block already validated the claim)
Expand All @@ -3138,8 +3158,8 @@ struct controller_impl {
// new claimed QC must be stricter than previous if block number is the same
EOS_ASSERT( qc_claim.is_last_qc_strong || !prev_qc_info->is_last_qc_strong,
block_validate_exception,
"claimed QC (${s1}) must be stricter than previous block's (${s2}) if block number is the same",
("s1", qc_claim.is_last_qc_strong)("s2", prev_qc_info->is_last_qc_strong) );
"claimed QC (${s1}) must be stricter than previous block's (${s2}) if block number is the same. Block number: ${b}",
("s1", qc_claim.is_last_qc_strong)("s2", prev_qc_info->is_last_qc_strong)("b", b->block_num()) );
}
}

Expand All @@ -3148,7 +3168,8 @@ struct controller_impl {
// in the previous block (checked earlier), QC proof must be provided
EOS_ASSERT( !qc_claim.is_last_qc_strong,
block_validate_exception,
"QC block extension must be provided if the claimed QC block is strong" );
"QC block extension must be provided if the claimed QC block is strong. Block number: ${b}",
("b", b->block_num()) );

// Conditions:
// * the claim is that the last QC is a weak QC,
Expand All @@ -3173,7 +3194,8 @@ struct controller_impl {
//
EOS_ASSERT( qc_claim.last_qc_block_num == prev.core.last_final_block_num,
block_validate_exception,
"QC block extension must be included if the claimed QC block is not current irreversible block" );
"QC block extension must be included if the claimed QC block is not current irreversible block. Block number: ${b}",
("b", b->block_num()) );

return;
}
Expand All @@ -3184,21 +3206,21 @@ struct controller_impl {
// Check QC information in header extension and block extension match
EOS_ASSERT( qc_proof.block_height == qc_claim.last_qc_block_num,
block_validate_exception,
"QC block number (${n1}) in block extension does not match last_qc_block_num (${n2}) in header extension",
("n1", qc_proof.block_height)("n2", qc_claim.last_qc_block_num) );
"QC block number (${n1}) in block extension does not match last_qc_block_num (${n2}) in header extension. Block number: ${b}",
("n1", qc_proof.block_height)("n2", qc_claim.last_qc_block_num)("b", b->block_num()) );

// Verify claimed strictness is the same as in proof
EOS_ASSERT( qc_proof.qc.is_strong() == qc_claim.is_last_qc_strong,
block_validate_exception,
"QC is_strong (${s1}) in block extension does not match is_last_qc_strong (${22}) in header extension",
("s1", qc_proof.qc.is_strong())("s2", qc_claim.is_last_qc_strong) );
"QC is_strong (${s1}) in block extension does not match is_last_qc_strong (${22}) in header extension. Block number: ${b}",
("s1", qc_proof.qc.is_strong())("s2", qc_claim.is_last_qc_strong)("b", b->block_num()) );

// find the claimed block's block state
auto bsp = fork_db_fetch_bsp_by_num( qc_claim.last_qc_block_num );
// find the claimed block's block state on branch of id
auto bsp = fork_db_fetch_bsp_by_num( prev.id, qc_claim.last_qc_block_num );
EOS_ASSERT( bsp,
block_validate_exception,
"Block state was not found in forkdb for block number ${b}",
("b", qc_claim.last_qc_block_num) );
"Block state was not found in forkdb for last_qc_block_num ${q}. Block number: ${b}",
("q", qc_claim.last_qc_block_num)("b", b->block_num()) );

// verify the QC proof against the claimed block
bsp->verify_qc(qc_proof.qc);
Expand All @@ -3209,7 +3231,7 @@ struct controller_impl {
// Verify claim made by instant_finality_extension in block header extension and
// quorum_certificate_extension in block extension are valid.
// This is the only place the evaluation is done.
verify_qc_claim(b, prev);
verify_qc_claim(id, b, prev);

auto trx_mroot = calculate_trx_merkle( b->transactions, true );
EOS_ASSERT( b->transaction_mroot == trx_mroot, block_validate_exception,
Expand All @@ -3230,9 +3252,6 @@ struct controller_impl {
EOS_ASSERT( id == bsp->id(), block_validate_exception,
"provided id ${id} does not match calculated block id ${bid}", ("id", id)("bid", bsp->id()) );

// integrate the received QC into the claimed block
integrate_received_qc_to_block(id, b);

return block_handle{bsp};
}

Expand Down Expand Up @@ -3281,6 +3300,11 @@ struct controller_impl {
const forked_callback_t& forked_branch_cb,
const trx_meta_cache_lookup& trx_lookup )
{
// Save the received QC as soon as possible, no matter whether the block itself is valid or not
if constexpr (std::is_same_v<BSP, block_state_ptr>) {
integrate_received_qc_to_block(bsp);
}

controller::block_status s = controller::block_status::complete;
EOS_ASSERT(!pending, block_validate_exception, "it is not valid to push a block when there is a pending block");

Expand Down
Loading