Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Default fork choice value and intermediates for block import parameters#4652

Merged
sorpaas merged 6 commits intomasterfrom
sp-consensus-intermediate
Jan 23, 2020
Merged

Default fork choice value and intermediates for block import parameters#4652
sorpaas merged 6 commits intomasterfrom
sp-consensus-intermediate

Conversation

@sorpaas
Copy link
Copy Markdown
Contributor

@sorpaas sorpaas commented Jan 16, 2020

Addresses some of #3623.

This PR tries to improve several issues in the consensus engines:

  • Currently, if users pass on an incompatible BlockImport with a Verifier, consensus will silently fail.
  • Because Verifier does not assume parents, it is not always possible for it to return the correct fork choice rule, but currently it is forced to do so.
  • Methods to pipe-line data downwards from Verifier to all subsequent BlockImports is not obvious.

In this PR:

  • Changed ForkChoiceStrategy to be Option. If None, it indicates that the BlockImport or Verifier cannot currently make a decision on the fork choice rule. Subsequent BlockImport should properly modify the value. If the value is passed all the way down to the bottom BlockImport, then the block will not be imported.
  • Added intermediates in BlockImportParams. This indicates values that should be processed by subsequent BlockImports. If a value in intermediates is not processed, the block import fails. As an example usage of this, in PoW engine, Verifier uses intermediates to pass the calculated difficulty (if available) and the full block hash down to BlockImport, who will then use them to calculate the block auxiliary.
  • Updated PoW engine to have proper separation of BlockImport and Verifier.

@sorpaas sorpaas added the A0-please_review Pull request needs code review. label Jan 16, 2020
@sorpaas sorpaas marked this pull request as ready for review January 16, 2020 21:32
@sorpaas sorpaas requested a review from rphmeier January 20, 2020 05:52
Comment thread primitives/consensus/common/src/block_import.rs Outdated
Comment thread primitives/consensus/common/src/block_import.rs Outdated
Comment thread primitives/consensus/common/src/block_import.rs

let intermediate = PowIntermediate::<B, Algorithm::Difficulty> {
hash: hash,
difficulty: None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not verify the difficulty here in the Verifier, as that's the parallelizable operation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Difficulty may not be parallelizable operation. The mining algorithm is always fixed (or can only be chosen from a set of validated ones), but for difficulty adjustment algorithms, they can be defined in runtime. In those cases, block importer will have to do a runtime call on the parent block to fetch the difficulty. Because parent block may not be available in Verifier, we can only set it to None here, meaning we do not yet know the value.

However, in mining routine, the value of difficulty is indeed available because we mine on local best block. In that case, difficulty is set to Some so we can avoid double calculation.

@rphmeier rphmeier added A6-mustntgrumble and removed A0-please_review Pull request needs code review. labels Jan 20, 2020
@rphmeier
Copy link
Copy Markdown
Contributor

LGTM except for question above

@sorpaas sorpaas requested a review from rphmeier January 21, 2020 09:18
Copy link
Copy Markdown
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM. When we want to parallelize the Verifier we might need to make the pipeline more fine-grained though (depending on consensus algorithm). For epoch based consensus protocols we'll only be able to parallelize verification of blocks within the same epoch.

@andresilva
Copy link
Copy Markdown
Contributor

andresilva commented Jan 22, 2020

For babe we might also be able to use intermediates to pass epoch data from verifier to block import (to avoid fetching it twice from the tree). I think this can be done on a follow-up PR though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants