Introduce notion of finality to substrate by rphmeier · Pull Request #760 · paritytech/substrate

rphmeier · 2018-09-17T19:49:33Z

Partially addresses #746

This rewrites the blockchain DB to handle non-instant finality and chain reorganizations.
Pruning and CHT generation is done after finality now.

rphmeier · 2018-09-17T19:53:04Z

I am not quite sure how to handle the finality case where we finalize a block that the "best" chain doesn't contain. One solution I had was to prevent indexing by number for non-finalized blocks, but that would probably hamstring the UX a little too much.

arkpar · 2018-09-18T10:03:42Z

I've noticed this changes DB keys for block data to use hashes instead of numbers. Using numbers is quite beneficial for DB performance. Blocks tend to be requested in groups by number when syncing and it helps a lot when they are located close in the DB. Geth did that optimization a while a go and the improvement was significant. I'd prefer to keep the current design. Additional blocks sharing the same number could be suffixed with an extra index and the suffix would be removed on finalization.

rphmeier · 2018-09-18T21:25:17Z

I'd rather have correctness than optimization for the moment, so let's re-optimize the DB once this is merged and the new consensus is integrated. Could you make a proposal for a new database schema that is efficient and handles reorgs/non-instant finality and file it?

svyatonik

Looked mostly at light + db-related changes (haven't touched state-db changes at all) to resolve #765 later - seems good. The only important thing I haven't understood - why not to fail when trying to revert finalized block (left a comment) - could you please elaborate?

svyatonik · 2018-09-19T07:32:09Z

+		// TODO: ensure this doesn't conflict with old finalized block.
+		let meta = self.blockchain.meta.read();
+		let f_num = f_header.number().clone();
+		let number_u64: u64 = f_num.as_().into();


Looks like .into() here is redundant.

svyatonik · 2018-09-19T07:33:06Z

+		}
+
+		let mut canonicalize_state = |canonical_hash| {
+			trace!(target: "db", "Canonicalize block #{} ({:?})", number_u64 - self.pruning_window, canonical_hash);


Isn't the number_u64 - self.pruning_window expression is constant? I mean this will print the same number, but different hashes

svyatonik · 2018-09-19T07:42:54Z

+					for retracted in tree_route.retracted() {
+						if retracted.hash == meta.finalized_hash {
+							// TODO: can we recover here?
+							warn!("Safety failure: reverting finalized block {:?}",


Why don't we return an error here? I thought that retracting finalized blocks should be impossible.

in general yes, but after having one experience with authorities signing a bad block I thought this would leave things easier to recover from in the future.

svyatonik · 2018-09-19T07:55:53Z

+/// ```
+///
+/// ```ignore
+/// Tree route from C to E. Retracted empty. Common is C, enacted [E1, E2]


"Tree route from C to E" -> "Tree route from C to E2"?

arkpar · 2018-09-20T12:48:04Z

I am not quite sure how to handle the finality case where we finalize a block that the "best" chain doesn't contain. One solution I had was to prevent indexing by number for non-finalized blocks, but that would probably hamstring the UX a little too much.

The way I see it only the best chain can be finalized. If there's a need to finalize some other chain the best chain should be reorganized to that chain first. I.e. it should become the new best

gavofyork · 2018-09-20T15:39:43Z

"best" here just means longest?

arkpar · 2018-09-20T16:09:39Z

"best" here just means longest?

Currently yes, although the blockchain backend does not enforce this. The "is_new_best" flag is provided by the client.

gavofyork · 2018-09-21T12:44:46Z

If the finalised leaf is on the non-best-chain, then we'll need to reorg first.

rphmeier · 2018-09-21T12:52:35Z

Doing this correctly depends on #740 -- the logic for handling finality on a block which is not an ancestor of the current best block requires finding the best_chain_containing(newly_finalized). Noted this in #746

arkpar · 2018-09-21T13:26:32Z

tree_route could use some tests. Looks good otherwise.

rphmeier · 2018-09-21T13:32:43Z

I'll add tree-route tests in the follow-up PR

* Update referral_or_council_of accordingly * Nit * Fix rls warnings in tokens/src/lib.rs * Fix tokens/src/mock.rs * Keep original asset_power and add internal_asset_power * Fix staking/src/mock.rs * Build wasm * Nits * Finish cross_chain_assets threshold test * Build wasm

…paritytech#760) * begin porting over traits; remove Config use of Hash * port over the Header bits that we need * sp_core_hashing where possible, move Verify to PairSigner, remove unused errors * tidy up Config things and move related bits into one place * fix codegen * copy Era over * move AccountId, Address, Signer to Signer trait and a pass over fixing examples * impl MultiAddress, MultiSignature, AccountId32 and add back to Config (for decoding later) * Copy over StorageKey, StorageData, StorageChangeSet * subxt core compiling with no sp_core or sp_runtime * Get examples compiling * pass over fixing tests * cargo fmt * clippy tweaks and update polkadot.rs * fix codegen docs * port over special DigestItem encoding/decoding * clippy and doc fixes * cargo fmt and example fix * more cargo fmt-ing... * substrate-extra to substrate-compat * cargo.toml comments * simplify PairSigner trait bounds * move RPC types to a separate file * fix docs * Add some tests for things and other PR feedback * bump to latest sp deps * avoid needing substrate-compat feature in a test

rphmeier added 4 commits September 14, 2018 17:04

finalization for in_mem

b42aa34

fetch last finalized block

5f44cd9

pruning: use canonical term instead of final

dacdc93

finalize blocks in full node

3962b06

rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 17, 2018

rphmeier added this to the 1.0 beta milestone Sep 17, 2018

rphmeier added 4 commits September 17, 2018 17:09

begin to port light client DB

bf10c02

add tree-route

959ea6e

keep number index consistent in full nodes

33d1c84

fix tests

b304925

rphmeier added 2 commits September 18, 2018 16:07

disable cache and finish porting light client

4427a29

add AsMut to system module

90a77fc

rphmeier mentioned this pull request Sep 18, 2018

Handle reorganizations in light client cache #765

Closed

final leaf is always best

0a692c7

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

rphmeier mentioned this pull request Sep 18, 2018

Non-instant finality and consensus abstraction #746

Closed

rphmeier added 2 commits September 18, 2018 22:53

Merge branch 'master' into rh-finality

9d1e0e0

fix all tests

00defb3

rphmeier mentioned this pull request Sep 19, 2018

best_containing operations (issue 603) #740

Merged

gavofyork removed this from the 1.0 beta milestone Sep 19, 2018

svyatonik reviewed Sep 19, 2018

View reviewed changes

rphmeier added 2 commits September 19, 2018 07:34

Fix comment and trace

57fc2d8

removed unused Into call

363b6c2

arkpar mentioned this pull request Sep 20, 2018

Index blocks by number in the DB #780

Closed

rphmeier mentioned this pull request Sep 21, 2018

Rewards & slashes proportional to the least-slashable validator #781

Merged

gavofyork 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 Sep 21, 2018

add comment on behavior of finalize_block

ebb89d9

rphmeier added A8-looksgood and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 21, 2018

arkpar approved these changes Sep 21, 2018

View reviewed changes

gavofyork merged commit 3cb5d88 into master Sep 21, 2018

gavofyork deleted the rh-finality branch September 21, 2018 13:56

Conversation

rphmeier commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rphmeier commented Sep 17, 2018

Uh oh!

arkpar commented Sep 18, 2018

Uh oh!

rphmeier commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svyatonik left a comment

Choose a reason for hiding this comment

Uh oh!

svyatonik Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

svyatonik Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

svyatonik Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

rphmeier Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

svyatonik Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

arkpar commented Sep 20, 2018

Uh oh!

gavofyork commented Sep 20, 2018

Uh oh!

arkpar commented Sep 20, 2018

Uh oh!

gavofyork commented Sep 21, 2018

Uh oh!

rphmeier commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkpar commented Sep 21, 2018

Uh oh!

rphmeier commented Sep 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rphmeier commented Sep 17, 2018 •

edited

Loading

rphmeier commented Sep 18, 2018 •

edited

Loading

rphmeier commented Sep 21, 2018 •

edited

Loading