Cumulative fixes to make working with consensus-pow easier #3617
Conversation
sorpaas
added
A0-please_review
sorpaas
changed the title
| @@ -78,6 +78,12 @@ pub trait FinalityProofProvider<Block: BlockT>: Send + Sync { | |||
| fn prove_finality(&self, for_block: Block::Hash, request: &[u8]) -> Result<Option<Vec<u8>>, Error>; | |||
| } | |||
|
|
|||
| impl<Block: BlockT> FinalityProofProvider<Block> for () { | |||
There was a problem hiding this comment.
I think the finality proof provider is optional when creating the service.
There was a problem hiding this comment.
I'll try to provide a reproducible test case, but I think what I had trouble with is that if I remove .with_finality_proof_provider for the service builder, the whole type resolution will fail. But maybe we should fix that instead.
There was a problem hiding this comment.
Yeah I see, it will fail to infer the generic type of the optional finality proof provider.
|
I made |
The reason why it was there is because when mine_loop returns, it means an error happened. In that case, we'd better sleep for a moment before trying again, because immediately trying would most likely just fail.
So that it does not mine when major syncing
Instead of hardcode it as previously 100ms.
For PoW, older blocks are secured by the work, and can mostly be considered to be finalized. Thus we can save both code complexity and validation time by skipping checking inherents for them.
To make things faster
core/consensus/pow/src/lib.rs
Outdated
| header, | ||
| BlockId::Hash(parent_hash), | ||
| )?; | ||
| aux.total_difficulty = aux.total_difficulty.saturating_add(difficulty); | ||
| aux.difficulty = difficulty; | ||
| aux.total_difficulty.add(difficulty); |
There was a problem hiding this comment.
Why was this changed from a saturating_add()?
There was a problem hiding this comment.
This is the add function from TotalDifficulty trait. The idea here is that some Difficulty definitions may want to use its own customized method for calculating the total difficulty from current difficulty. Most of the time TotalDifficulty::add will still be implemented as saturating_add.
Each engine can use its own Rng source.
sorpaas
removed
the
A2-insubstantial
sorpaas
changed the title
core/consensus/pow/src/lib.rs
Outdated
| header, | ||
| BlockId::Hash(parent_hash), | ||
| )?; | ||
| aux.total_difficulty = aux.total_difficulty.saturating_add(difficulty); | ||
| aux.difficulty = difficulty; | ||
| aux.total_difficulty.add(difficulty); |
There was a problem hiding this comment.
A small nit: my brain treats add as a function which takes &self and returns Self. For this case, it seems increase(&mut self, other: Self) → () would be more appropriate, don't you think so?
| @@ -157,6 +173,10 @@ impl<C, Algorithm> PowVerifier<C, Algorithm> { | |||
| { | |||
| const MAX_TIMESTAMP_DRIFT_SECS: u64 = 60; | |||
|
|
|||
| if *block.header().number() < self.check_inherents_after { | |||
There was a problem hiding this comment.
Why did we need to add this? Do we have to deal with any existing deployment?
There was a problem hiding this comment.
I added this parameter because runtime upgrades may break older inherent data check. For example, when the inherent data format is changed completely or an old inherent module is removed. In those cases, some chains might want to completely skip the check, because the older the block is, the more "finalized" it is because of PoW built on top of it.
This branch can be disabled by simply setting check_inherents_after to 0.
Co-Authored-By: André Silva <andre.beat@gmail.com>
This PR contains several cumulative fixes to make working with consensus-pow easier:
PowAux. The reason for this is that nearly all difficulty adjustment algorithms need this information.TimestampApiinpow-primitivesso that consensus engine can query timestamp data.FinalityProofProviderfor(), which always returnNone. This seems to be needed because we don't currently have a "dummy" finality proof provider for those engines that do not need it.Difficulty, removing assumption that it isu128.