Introduce inherent digests by Demi-Marie · Pull Request #2466 · paritytech/substrate

Demi-Marie · 2019-05-03T15:20:54Z

What This Is

Inherent digests are digests that are provided to the runtime by the consensus algorithm. They are inherent because the runtime trusts them implicitly, although the consensus algorithm does verify them.

This PR passes the digests in the header to the runtime via initialize_block. It does not, however, provide any means for runtimes to actually use the information. This will be provided later.

Changes to digest items

The DigestItem::Seal variant had been deprecated in an earlier PR. This PR removes the deprecation, and changes it to contain ConsensusEngineId and a SealSignature. It also adds the variant DigestItem::PreRuntime for pre-runtime digests.

A consequence of these changes is that the following invariants must be upheld:

Runtimes MUST NOT produce a DigestItem::Seal or DigestItem::PreRuntime. Only consensus engines can produce these varients.
Consensus engines MUST NOT produce any variants other than DigestItem::Seal or DigestItem::PreRuntime. Furthermore, there MUST be exactly one DigestItem::Seal per combination of block and consensus engine, and they must come after all other digests.
The SRML reserves DigestItem::ChangesTreeRoot for itself. Runtimes that use the SRML MUST NOT produce any DigestItem variant except DigestItem::AuthoritiesChange and DigestItem::Consensus.

These invarients are currently not checked. However, I plan on improving the API to catch violations of them at compile-time, by making illegal states unrepresentable.

Backwards compatibility

There isn’t any. Versions of Substrate that include this PR are 100% incompatible with versions that came before. Attempting to combine incompatible versions will result in blocks failing to import.

If necessary, I can include read-only backwards-compatibility support. This will allow nodes including this PR to import blocks generated by older nodes, but not create blocks that can be imported by older nodes.

`Cargo.lock` changes

Much of the changes are to Cargo.lock files, as some of the code depends on newer versions of dependencies. Reverting these dependencies is trivial, but I believe that upgrading dependencies is a good thing, so that Substrate can take advantage of upstream bug-fixes. This is especially true of libraries like ring, which do not do any backports at all.

Fixes #2232

All tests pass. There are still limitations: 1. The runtime strips out inherent digests, so BABE must re-add them. 2. The test runtime checks that it can re-compute all digests. It can’t, so I had to comment out that test.

Seals were not imported correctly: the pre-digest was imported twice, instead of both it and the seal being imported. Also, other parts of the code did not compile due to incomplete refactoring.

core/basic-authorship/src/basic_authorship.rs

core/client/src/block_builder/block_builder.rs

This enforces that inherent digests are added first.

Suggested-by: Robert Habermeier <robert@parity.io>

Demi-Marie

Review addressed.

Demi-Marie · 2019-05-25T00:59:42Z

core/consensus/babe/src/lib.rs

 	authorities: &[Public],
 	threshold: u64,
-) -> Result<CheckedHeader<B::Header, DigestItemFor<B>>, String>
+) -> Result<CheckedHeader<B::Header, (DigestItemFor<B>, DigestItemFor<B>)>, String>


That said, I believe that would belong in a separate PR.

It wasn’t needed

I also did some reformatting and cleanup.

gavofyork · 2019-05-28T15:02:42Z

Some conflicts. If there's nothing left, it'd be good to get this merged. Feel free to bother some reviewers about it.

andresilva

Minor nits.

core/basic-authorship/src/basic_authorship.rs

core/client/src/client.rs

core/consensus/aura/src/digest.rs

core/sr-primitives/src/generic/digest.rs

andresilva · 2019-05-28T15:39:15Z

This will break the flaming fir network, I'm not sure whether we should have some backwards-compatibility support (just to import old seals), or whether we should reset flaming fir after we deploy this. I don't have a very strong opinion on it, but I also don't want us to spend too much effort dealing with backwards compatibility on flaming fir (the idea of the network is to run master with no stability guarantees).

Reformatting Co-Authored-By: André Silva <andre.beat@gmail.com>

Also, remove some needless unsafe code.

With the removal of the deprecated `Seal` variant, these are not needed.

RLS did not tell me that I hadn’t fixed `babe/lib.rs`, so I missed it.

gavofyork · 2019-05-29T12:12:44Z

i'm fine with reseting it.

This reverts commit 6e253c8.

* Introduce inherent digests * Implement inherent digests * fix silly error * Implementation of inherent digests in BABE All tests pass. There are still limitations: 1. The runtime strips out inherent digests, so BABE must re-add them. 2. The test runtime checks that it can re-compute all digests. It can’t, so I had to comment out that test. * Fix compilation and seal import Seals were not imported correctly: the pre-digest was imported twice, instead of both it and the seal being imported. Also, other parts of the code did not compile due to incomplete refactoring. * Remove bogus assertion * Fix testsuite compilation * Remove unused import * Fix compiler diagnostics * Add inherent digest parameters to block constructors This enforces that inherent digests are added first. * Fixup Cargo.lock * Fix build errors * Re-add an incorrectly removed import * Bump primitive-types version * Update Cargo.lock * Refactoring * Use inherent digests for AuRa They do reach the runtime, but get stripped. I have not figured out where. * Fix compilation errors * Fix compilation errors due to incorrect types * Fix whitespace Suggested-by: Tomasz Drwiega <tomasz@parity.io> * Add preamble Suggested-by: Tomasz Drwiega <tomasz@parity.io> * Fix silly compile error * Refactor pre-digest finding code into a separate function * Remove unwanted assertion It is too likely to bring down the entire blockchain. Suggested-by: Tomasz Drwiega <tomasz@parity.io> * Use `find_pre_digest` after runtime, too Also, use `Member` trait rather than rolling our own requirements. Suggested-by: Tomasz Drwiega <tomasz@parity.io> * Fix various warnings mostly due to upgrading the dependency on `error_chain`. * Pre-digests nearly complete This nearly completes the implementation of pre-runtime digests. * `Seal2` → `Seal` and fix test suite * Try to fix the storage error * Try to fix storage (again) * Fix tests * Hopefully finish pre-runtime digests The key is to pass *only* the pre-runtime digests to the runtime. The others must be stripped out by `initialize_block`. * Fix silly typo * Fix another silly mistake * Remove unnecessary filtering of BABE pre-digests We no longer get duplicate BABE pre-digests, so if they appear, the header should be rejected outright. * Update Cargo.lock files * Reformatting * Fix silly typo in inherent digest code Also, revert `error.rs` files that contained calls to the `error_chain!` macro. * Try to keep the runtime from stripping pre-digests Currently runs into the “Storage root must match that calculated” assertion. * Don’t compute storage root until storage changes are done. Also, fix a compilation error. * Fix compile-time error * Fix compilation errors * Fix more compile errors * Hopefully it compiles this time… * Fix compilation and add docs * Prevent BABE from adding duplicate pre-runtime digests Found by comparing with the AuRa code. I also did some refactoring. * Respond to review and fix some warnings * Delete some dead code introduced earlier * More dead code goes away * `ref mut` → `&mut` * Respond to review and fix some warnings * Fix compilation error * Remove unneeded `HashT` type parameter Suggested-by: Robert Habermeier <robert@parity.io> * Remove spurious #[allow(deprecated)] * Document inherent digest parameter to `build_block` * Delete `Simple` trait It wasn’t needed * delete wrongly added files * Fix trait bounds * Digest serialization tests I also did some reformatting and cleanup. * Apply suggestions from code review Reformatting Co-Authored-By: André Silva <andre.beat@gmail.com> * Swap two arguments to `propose` and `propose_with` Also, remove some needless unsafe code. * Remove bogus `#![allow(deprecated)]` annotations With the removal of the deprecated `Seal` variant, these are not needed. * Add a missing `#[allow(deprecated)]` in the AuRa tests * Fix silly compile error * Fix silly compiler error RLS did not tell me that I hadn’t fixed `babe/lib.rs`, so I missed it. * Fixes made automatically by Cargo

Introduce inherent digests

7862bac

Demi-Marie requested review from ascjones, sorpaas and tomusdrw May 3, 2019 15:20

Demi-Marie added 2 commits May 3, 2019 14:38

Implement inherent digests

cf596d0

fix silly error

4fbf981

gavofyork added the A0-please_review Pull request needs code review. label May 4, 2019

Demi-Marie added 2 commits May 4, 2019 19:08

Merge branch 'master' into demi-babe-progress

66369b4

Implementation of inherent digests in BABE

7709cf6

All tests pass. There are still limitations: 1. The runtime strips out inherent digests, so BABE must re-add them. 2. The test runtime checks that it can re-compute all digests. It can’t, so I had to comment out that test.

devops-parity added the B2-breaksapi label May 6, 2019

Demi-Marie added 3 commits May 6, 2019 09:50

Fix compilation and seal import

93c6199

Seals were not imported correctly: the pre-digest was imported twice, instead of both it and the seal being imported. Also, other parts of the code did not compile due to incomplete refactoring.

Merge branch 'master' into demi-babe-progress

8f174f0

Remove bogus assertion

89eef07

Demi-Marie requested review from andresilva, gnunicorn and rphmeier and removed request for ascjones, sorpaas and tomusdrw May 6, 2019 15:52

Fix testsuite compilation

f82648f

gnunicorn added the A3-needsresolving label May 8, 2019

Demi-Marie and others added 3 commits May 8, 2019 08:48

Merge branch 'master' into demi-babe-progress

d12a415

Remove unused import

4438a9c

Fix compiler diagnostics

668db27

rphmeier reviewed May 9, 2019

View reviewed changes

core/basic-authorship/src/basic_authorship.rs Outdated Show resolved Hide resolved

rphmeier reviewed May 9, 2019

View reviewed changes

core/client/src/block_builder/block_builder.rs Outdated Show resolved Hide resolved

rphmeier added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 9, 2019

Demi-Marie and others added 2 commits May 9, 2019 11:52

Merge branch 'master' into demi-babe-progress

ee80e54

Add inherent digest parameters to block constructors

20efe4a

This enforces that inherent digests are added first.

Demi-Marie removed the A3-needsresolving label May 24, 2019

Demi-Marie added 2 commits May 24, 2019 15:55

Remove unneeded HashT type parameter

6c74371

Suggested-by: Robert Habermeier <robert@parity.io>

Remove spurious #[allow(deprecated)]

ea4295e

Demi-Marie commented May 25, 2019

View reviewed changes

Demi-Marie added 6 commits May 25, 2019 08:01

Document inherent digest parameter to build_block

1ccdc65

Delete Simple trait

3371a40

It wasn’t needed

delete wrongly added files

4a5ddc7

Fix trait bounds

713753c

Digest serialization tests

19bda9d

I also did some reformatting and cleanup.

Merge branch 'master' into demi-babe-progress

8dd3b1b

andresilva reviewed May 28, 2019

View reviewed changes

Demi-Marie and others added 8 commits May 28, 2019 13:52

Apply suggestions from code review

4a72d10

Reformatting Co-Authored-By: André Silva <andre.beat@gmail.com>

Swap two arguments to propose and propose_with

8ef2aa6

Also, remove some needless unsafe code.

Remove bogus #![allow(deprecated)] annotations

e4661c6

With the removal of the deprecated `Seal` variant, these are not needed.

Add a missing #[allow(deprecated)] in the AuRa tests

83ea1e4

Merge remote-tracking branch 'origin/master' into demi-babe-progress

7ce8413

Fix silly compile error

229e1a7

Fix silly compiler error

d3c6a58

RLS did not tell me that I hadn’t fixed `babe/lib.rs`, so I missed it.

Fixes made automatically by Cargo

dbdd214

andresilva mentioned this pull request May 29, 2019

derive codec for inherent data #2714

Merged

gavofyork merged commit 6e253c8 into master May 29, 2019

gavofyork deleted the demi-babe-progress branch May 29, 2019 12:13

Demi-Marie mentioned this pull request May 30, 2019

PreRuntime support in runtimes #2738

Closed

Demi-Marie added a commit that referenced this pull request Jun 1, 2019

Revert "Introduce inherent digests (#2466)"

5defb00

This reverts commit 6e253c8.

rphmeier mentioned this pull request Jun 20, 2019

PreRuntime Digests: Extract authorship information on the runtime side #2918

Closed

Demi-Marie mentioned this pull request Jun 23, 2019

BABE Randomness using PreRuntime digests #2929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce inherent digests#2466

Introduce inherent digests#2466
gavofyork merged 81 commits intomasterfrom
demi-babe-progress

Demi-Marie commented May 3, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Demi-Marie left a comment

Uh oh!

Demi-Marie May 25, 2019

Uh oh!

gavofyork commented May 28, 2019

Uh oh!

andresilva left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andresilva commented May 28, 2019

Uh oh!

gavofyork commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Conversation

Demi-Marie commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What This Is

Changes to digest items

Backwards compatibility

Cargo.lock changes

Uh oh!

Uh oh!

Uh oh!

Demi-Marie left a comment

Choose a reason for hiding this comment

Uh oh!

Demi-Marie May 25, 2019

Choose a reason for hiding this comment

Uh oh!

gavofyork commented May 28, 2019

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andresilva commented May 28, 2019

Uh oh!

gavofyork commented May 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Demi-Marie commented May 3, 2019 •

edited

Loading

`Cargo.lock` changes