IF: Add block extension which contains latest HotStuff commitment known to proposer #1607
Conversation
greg7mdp
changed the title
greg7mdp
changed the title
…rom Kevin's PR.
| } | ||
| } | ||
| signed_block_ptr sb = blk->block; | ||
| std::optional<block_extension> ext = sb->extract_extension(hs_commitment_extension::extension_id()); |
There was a problem hiding this comment.
If block_num is less than active commitment(s) block number this can be skipped. It is only used for syncing so we should see if it is relevant before processing.
There was a problem hiding this comment.
I'm not sure that it is not needed, since we need to track the finalizer_set with a delay (when a commitment seen in accepted blocks made it irreversible), which is different from the active finalizer_set of hotstuff. Also, the optimization is not significant because most blocks will not have commitments.
| void qc_chain::send_hs_new_block_msg(const hs_new_block_message & msg){ | ||
| fc_tlog(_logger, " === broadcast_hs_new_block ==="); | ||
| _pacemaker->send_hs_new_block_msg(msg, _id); | ||
| std::optional<hs_commitment> qc_chain::send_hs_msg(const hs_message& msg) { |
There was a problem hiding this comment.
I think we should avoid unneeded copies of hs_message. 4 similar methods to avoid copies I think is worth it.
There was a problem hiding this comment.
Where is it copied?
There was a problem hiding this comment.
But yes the variant is larger for some messages than otherwise.I still think this is much cleaner and the cost is insignificant compared to the rest of what we do.
There was a problem hiding this comment.
When you create a variant from a type it is copied unless you move into the construction. Might want to take by value and move into it to avoid the copy. Although, most of the data in the messages is likely not movable.
libraries/hotstuff/qc_chain.cpp
Outdated
| // issue #1522: HotStuff finality should drive LIB in controller | ||
| // no need to do anything here. `qc_chain::update()` will return the `hs_commitment` where b.block_id is final | ||
| // and chain_pacemaker will notify the controller. The controller will notify `fork_database` so that | ||
| // this block and its ancestors are marked irreversible and moved out of `fork_database`. |
There was a problem hiding this comment.
We can comment out this for loop then.
There was a problem hiding this comment.
I doubt commenting an empty loop changes anything in the generated code.
There was a problem hiding this comment.
True, more for documentation than efficiency.
| @@ -123,6 +123,7 @@ add_library( eosio_chain | |||
|
|
|||
There was a problem hiding this comment.
There are conflicts with branch that need to be resolved.
There was a problem hiding this comment.
Yes, I was waiting for gnome's PR to be merged so I don't resolve conflicts twice.
|
Overcome by events |
Resolves #1586.
Proposer takes the latest irreversible HotStuff commitment proof (the latest block proposal with the final_on_qc pointing to an irreversible block along with QC for it) known to it and includes it in a new block extension for the block that it constructs. This is the irreversible HotStuff proofs which means they always are relevant for advancing LIB on the irreversible blockchain.