This repository was archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Allow for customisation of chain selection systems #2240
Merged
Merged
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
1180420
move SelectChain trait out of client
gnunicorn b204874
Extend SelectChain, move longest chain implementation into it
gnunicorn 32c0c72
Bring SelectChain into service
gnunicorn 1bfeb83
implement LongestChain SelectChain
gnunicorn a46acc6
implement longest chain for node
gnunicorn 30bab5f
Merge remote-tracking branch 'origin/master' into ben-custom-chainsel…
gnunicorn 30f5528
update Cargo.lock's
gnunicorn 4cf2abf
in between erroring tests
gnunicorn 5ff112a
deprecate ::backend and ::import_lock
gnunicorn a2b0dd1
Remove unneded space
bkchr 2db6163
Remove unneded space
bkchr f1c4225
Fixes test compilation
bkchr d2f21a4
remove todo
gnunicorn 097905a
re-enable client test
gnunicorn 1c870e1
add doc
gnunicorn 59fbf03
fixing tests
gnunicorn 326a8c3
Clarify SelectChain Interface, intended implementation and usage
gnunicorn 5fb7a09
minor components cleanups
gnunicorn e6f8ed2
minor cleanups
gnunicorn d92d542
Update lock files
gnunicorn f45a8b7
Merge branch 'master' into ben-custom-chainselection-strategy
gnunicorn f9c94a5
Implement cleaner interface for SelectChain
gnunicorn 1de5c64
addressing comments
gnunicorn 2b52445
Updating tests
gnunicorn 38f8f65
bump node runtime impl version
gnunicorn c7bdc40
Merge branch 'master' into ben-custom-chainselection-strategy
gnunicorn a307363
Merge branch 'master' into ben-custom-chainselection-strategy
gavofyork 09bb8a8
address grumbles
gnunicorn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // Copyright 2019 Parity Technologies (UK) Ltd. | ||
| // This file is part of Substrate Consensus Common. | ||
|
|
||
| // Substrate Demo is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
|
|
||
| // Substrate Consensus Common is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
|
|
||
| // You should have received a copy of the GNU General Public License | ||
| // along with Substrate Consensus Common. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| use crate::error::Error; | ||
| use runtime_primitives::traits::{Block as BlockT, NumberFor}; | ||
|
|
||
|
|
||
| /// The SelectChain trait defines the strategy upon which the head is chosen | ||
| /// if multiple forks are present for an opaque definition of "best" in the | ||
| /// specific chain build. | ||
| /// | ||
| /// The Strategy can be customised for the two use cases of authoring new blocks | ||
| /// upon the best chain or which fork to finalise. Unless implemented differently | ||
| /// by default finalisation methods fall back to use authoring, so as a minimum | ||
| /// `_authoring`-functions must be implemented. | ||
| /// | ||
| /// Any particular user must make explicit, however, whether they intend to finalise | ||
| /// or author through the using the right function call, as these might differ in | ||
| /// some implementations. | ||
| /// | ||
| /// Non-deterministicly finalising chains may only use the `_authoring` functions. | ||
| pub trait SelectChain<Block: BlockT>: Sync + Send + Clone { | ||
|
|
||
| /// Get all leaves of the chain: block hashes that have no children currently. | ||
| /// Leaves that can never be finalized will not be returned. | ||
| fn leaves(&self) -> Result<Vec<<Block as BlockT>::Hash>, Error>; | ||
|
|
||
| /// Among those `leaves` deterministically pick one chain as the generally | ||
| /// best chain to author new blocks upon and probably finalize. | ||
| fn best_chain(&self) -> Result<<Block as BlockT>::Header, Error>; | ||
|
|
||
| /// Get the best ancestor of `target_hash` that we should attempt | ||
| /// to finalize next. | ||
| fn finality_target( | ||
| &self, | ||
| target_hash: <Block as BlockT>::Hash, | ||
| _maybe_max_number: Option<NumberFor<Block>> | ||
| ) -> Result<Option<<Block as BlockT>::Hash>, Error> { | ||
| Ok(Some(target_hash)) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, this breaks API. We will have to find a way to do this that doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this change will inevitably break the api - as it needs to be specified and at the service-intend-layer and somehow pushed down to the authoring and finalisation. Even if we tried to still have client facilitate this somehow, this would mean adding the same
SCon everyWhere C: Client<...SC>, which would also break this same call.I am afraid there is just no way around this breaking the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing we could do is have client implement the new trait as well and hard-code that to the
LongestChainimplementation and directly deprecate its usage. But then still, would the where clause later on have to change to the newtraitand thus, this would still break the API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are hacky ways to do it by stuffing the information into mutable state, such as a thread-local variable. None of those are good, but we can avoid future breaking changes by switching to the builder pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently the blocker for me on this PR. I am not sure how we even can resolve this without creating a huge other mess elsewhere.
The only way I can currently think of is by having a compatibility layer, that is
ClientimplementsLongestChainSC, however in that case, a) the where clause still needs to be extended (and that is "breaking the API", as it now constrains more than it was before) and b) it would mean this function doesn't actually allow any other implementation and thus isn't really implementing the ticket (or solving our problem).Not sure what to do about this?!?