IF: make qc_info in instant_finality_extension non-optional #2145
Conversation
libraries/chain/controller.cpp
Outdated
| block_validate_exception, | ||
| "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()) ); | ||
| EOS_ASSERT( qc_proof.block_num == qc_claim.last_qc_block_num, block_validate_exception, |
There was a problem hiding this comment.
The original way placing block_validate_exception in a separate line looks better.
There was a problem hiding this comment.
I guess it is a matter of preference, I personally prefer the code to me more compact if possible, but as you originally wrote this code I'll switch back to using an extra line.
There was a problem hiding this comment.
FWIW, I personally prefer the more compact as well.
There was a problem hiding this comment.
@linh2931 I'll defer to you, would you be OK with me switching back to the more compact form?
I agree that 4 lines is more legible, but basically all the EOS_ASSERT follow the same pattern of 4 arguments, and typically the name of the exception is not critical to understanding what the assert does, so I feel that having it more compact is beneficial, as it allows to see more of the code in one screen.
There was a problem hiding this comment.
Originally I crammed all into one line. As Kevin pointed out, it did not look good. I think for shorter ones, a single line is better. But for longer ones, a separate lines are better. If we do separate lines, each argument should be on a separate line so it is easy to see.
There was a problem hiding this comment.
I think line 1 (the condition) and line 3 (the description) are important and nice when they stand out on separate lines.
There was a problem hiding this comment.
If we do separate lines, each argument should be on a separate line so it is easy to see.
I don't agree that we sould constrain ourselves to either a single line, or as many lines as we have arguments. I feel that we should try to make the code as nice to read as possible.
There was a problem hiding this comment.
That's why I said shorter ones single line, longer ones separate lines. Those are all personal preferences. I look at the code in different terminals. Separate ones do look better.
libraries/chain/controller.cpp
Outdated
| ("b", block_num) ); | ||
|
|
||
| EOS_ASSERT( !prev_header_ext, block_validate_exception, | ||
| "Block #${b} doesn't have a finality header extension even though its predecessor does. Block number: ${b}", |
There was a problem hiding this comment.
Block number: ${b} is redunant.
There was a problem hiding this comment.
Good catch, fixing.
| block_validate_exception, | ||
| "A block must have QC claim if it provides QC block extension. Block number: ${b}", | ||
| ("b", b->block_num()) ); | ||
| assert(header_ext); |
There was a problem hiding this comment.
Why this assert? The function already returned if (!header_ext).
There was a problem hiding this comment.
It is just a way to document (in a way that is checked at run time) that at this point in the function, we have a header_ext. Same as later I have assert(header_ext && prev_header_ext);.
Yes it is true that you can deduct it from reading the previous code, but I think this makes it nicely explicit.
| @@ -2854,16 +2854,17 @@ struct controller_impl { | |||
|
|
|||
| static std::optional<qc_data_t> extract_qc_data(const signed_block_ptr& b) { | |||
There was a problem hiding this comment.
Seems we should name this validate_and_extract_qc_data as it is calling the validate_and_extract methods of extensions.
There was a problem hiding this comment.
Hum, I think it could be misleading, it does the generic extension validation, but not the actual qc_data validation implemented in verify_qc_claim.
|
Note:start |
Resolves #2128.