Conversation
| /// filter. | ||
| fn filter_uncle(header: &Header, acc: Self::Accumulator) | ||
| -> Result<(Option<Author>, Self::Accumulator), &'static str>; | ||
| fn filter_uncle(header: &Header, acc: &mut Self::Accumulator) |
There was a problem hiding this comment.
Changed acc to be a mutable reference because we also need it in case of error so that filtering might be able to skip invalid uncle and continue. Returning acc for both Ok and Err results is way less readable and more verbose code.
core/client/src/client.rs
Outdated
| Client::uncles(self, target_hash, max_generation)? | ||
| .into_iter() | ||
| .map(|hash| Client::header(self, &BlockId::Hash(hash)) | ||
| .map(|maybe_header| maybe_header.expect("Each uncle exists in the database; qed"))) |
There was a problem hiding this comment.
it would be much safer to filter_map. this expectation doesn't prove to me why the uncle will exist (in the future, it might race against block pruning or something similar)
core/consensus/babe/src/lib.rs
Outdated
| pub use babe_primitives::AuthorityId; | ||
|
|
||
| /// Maximum uncles generations we may provide to the runtime. | ||
| const MAX_UNCLE_GENERATIONS: u32 = 8; |
There was a problem hiding this comment.
this should be drawn from the runtime instead
There was a problem hiding this comment.
Runtime calls are expensive. And this will be called for each block verification. My reasoning is that it is better to pick a hard limit, pass a few extra headers and let the runtime sort it out later rather than make another call into WASM.
There was a problem hiding this comment.
it will be called for every block creation only. but yeah, this is fine for now. The downside is that we'll do extra work looking for uncles and can't support more than 8 generations. This will be a barrier to having ultra-fast block time chains.
There was a problem hiding this comment.
From what I see InherentData is populated for verification as well.
core/consensus/babe/src/lib.rs
Outdated
| } | ||
|
|
||
| /// Register uncles inherent data provider, if not registered already. | ||
| fn register_uncles_inherent_data_provider<B, C, SC>( |
There was a problem hiding this comment.
putting this in the BABE crate doesn't seem right - other block production systems will also want uncles
srml/authorship/src/lib.rs
Outdated
|
|
||
| if duplicate || in_chain { return Err("uncle already included") } | ||
| fn create_inherent(data: &InherentData) -> Option<Self::Call> { | ||
| let uncles = data.uncles().expect("Gets and decodes final number inherent data"); |
There was a problem hiding this comment.
Panic happens when the data has invalid encoding, not when it is missing.
There was a problem hiding this comment.
yes. still wrong IMO. what proves that the data will have correct encoding?
you could maybe make a case that the provider provides data that's correct, but it's a far-reaching assumption, since it can be broken by a change in provider implementation later.
since it can't be proven that the correct data is supplied, the panicker should be removed
rphmeier
left a comment
rphmeier
left a comment
There was a problem hiding this comment.
looks good except for grumbles
arkpar
added
A3-in_progress
arkpar
added
A0-please_review
gavofyork
added
A7-looksgoodcantmerge
and removed
A0-please_review
* Include uncles * Filter missing uncles * Moved inherent registration to a new crate * Ignore invalid inherent encoding
4 participants
Closes #2956
Inherent data is populated with all possible uncles for the best block. When authoring this list is additionally filtered to remove all uncles that the runtime considers invalid.
There a small issue with this implementation. Currently inherents are populated before the block to propose on is chosen. If canonical block changes after uncles are collected we might end up with no uncles included for this block.