Allow BabeConsensusDataProvider fork existing chain #7078

seunlanlege · 2020-09-10T14:41:01Z

This allows BabeConsensusDataProvider build blocks on top of an existing chain where the Alice key isn't part of the authorities.

tomusdrw

Looks reasonable. Again the context here is to be able to create "fake" babe blocks in the test environment with ManualSeal.

client/consensus/babe/src/lib.rs

client/consensus/babe/src/aux_schema.rs

client/consensus/manual-seal/src/consensus/babe.rs

bkchr · 2020-09-18T11:29:00Z

primitives/io/src/lib.rs

@@ -408,6 +408,105 @@ pub trait Misc {
 	}
 }

+/// for times you want your chain to be useless
+#[runtime_interface]
+pub trait UselessCrypto {


Ideally this should not live in Substrate.

If it needs to live in Substrate, it should go into a different crate. Where it is directly understandable that this is for testing.

tomusdrw · 2020-09-22T10:27:23Z

primitives/io/src/lib.rs

@@ -460,7 +498,8 @@ pub trait Crypto {
 		msg: &[u8],
 		pub_key: &ed25519::Public,
 	) -> bool {
-		ed25519::Pair::verify(sig, msg, pub_key)
+		// ed25519::Pair::verify(sig, msg, pub_key)


Please change the state of this PR to Draft since it seems it's being used to experiment with stuff and for sure is not ready to be merged to Substrate.

tomusdrw · 2020-09-24T10:19:51Z

client/api/src/execution_extensions.rs

@@ -97,6 +99,7 @@ impl<Block: traits::Block> Default for ExecutionExtensions<Block> {
 		Self {
 			strategies: Default::default(),
 			keystore: None,
+			crypto_extension: RwLock::new(Arc::new(DefaultCryptoImpl)),


I think it shouldn't be required since it's going to be rarely customized. We should either add it not as an extension but rather to Externalities trait (with default sane implementations) or we should allow overriding via extensions_factory - i.e. we register the default extension only if it's not present in the Extensions returned by extensions_factory invocation.

tomusdrw

looks good to me, @bkchr could you take a look? I remember you didn't like the idea of putting crypto stuff into an extension, but we couldn't make it work with replace_implementation, since that one has to be called from within the runtime (and we want to keep the existing runtime and only alter the host environment).

tomusdrw · 2020-10-01T08:31:47Z

primitives/core/src/traits.rs

+
+sp_externalities::decl_extension! {
+	/// The crypto extension to register/retrieve from the externalities.
+	pub struct CryptoExtension(Box<dyn CryptoExt>);


To prevent a virtual call in the default case, we could perhaps do:

enum CryptoExt { Default(DefaultCryptoImpl), Custom(Box<dyn CryptoExt>), }

Alternatively we could change the method implementation so that in case CryptoExt is not there we fall back to the default impl (i.e. CryptoExtension would not be required to be registered for this thing to work), but I feel current approach feels cleaner.

bkchr · 2020-10-01T09:01:50Z

looks good to me, @bkchr could you take a look? I remember you didn't like the idea of putting crypto stuff into an extension, but we couldn't make it work with replace_implementation, since that one has to be called from within the runtime (and we want to keep the existing runtime and only alter the host environment).

I never said that @seunlanlege should use replace_implementation. I described it to him, that the host function that comes first is chosen and I also had shown him where he needs to change the order to make this work...

bkchr · 2020-10-01T09:03:11Z

client/api/src/execution_extensions.rs

@@ -160,6 +167,11 @@ impl<Block: traits::Block> ExecutionExtensions<Block> {

 		let mut extensions = self.extensions_factory.read().extensions_for(capabilities);

+		// alas! there's no CryptoExtension in the extension factory.
+		if let None = extensions.get_mut(TypeId::of::<CryptoExtension>()) {


Suggested change

if let None = extensions.get_mut(TypeId::of::<CryptoExtension>()) {

if extensions.get_mut(TypeId::of::<CryptoExtension>()).is_none() {

bkchr · 2020-10-01T09:07:42Z

primitives/core/src/traits.rs

+	///
+	/// Returns `true` when the verification in successful regardless of
+	/// signature version.
+	fn sr25519_verify_deprecated(&self, sig: &sr25519::Signature, msg: &[u8], pubkey: &sr25519::Public) -> bool {


This function is only required for Kusama to sync. We don't require it here.

bkchr · 2020-10-01T09:08:38Z

primitives/core/src/traits.rs

@@ -195,6 +195,48 @@ pub trait BareCryptoStore: Send + Sync {
 	) -> Result<VRFSignature, Error>;
 }

+/// Something that generates, stores and provides access to keys.


Docs are not correct.

bkchr · 2020-10-01T09:09:35Z

primitives/core/src/traits.rs

@@ -195,6 +195,48 @@ pub trait BareCryptoStore: Send + Sync {
 	) -> Result<VRFSignature, Error>;
 }

+/// Something that generates, stores and provides access to keys.
+pub trait CryptoExt: Send + Sync {


What is with the batching stuff, why isn't that here? Where do we draw the line?

Good catch, we should defintely move all crypto stuff here to make it consistent.

batching eventually calls methods on CryptoExt

bkchr · 2020-10-01T09:10:40Z

@NikVolf is your benchmark bot still working? Could you please run it here.

bkchr · 2020-10-01T11:31:43Z

@seunlanlege please explain me what the problem was with overwriting the host functions?

seunlanlege · 2020-10-01T12:23:03Z

Asides from the fact that it didn't work for me for some unknown reason, We needed a way to override signature verification without forking substrate and modifying the order in which host functions are initialized.

bkchr · 2020-10-01T12:53:25Z

Asides from the fact that it didn't work for me for some unknown reason,

That is no "explanation" I will accept here.

We needed a way to override signature verification without forking substrate and modifying the order in which host functions are initialized.

You already have done the changes here: https://github.com/paritytech/substrate/pull/7078/files#diff-b447ad296633d7d693790a1fb7ce8b5f ?

And you modify Substrate here anyway?

tomusdrw

Seems that crypto overrides can be better done with a host function overrides than execution extension, so let's implement that instead.

author Seun Lanlege <seunlanlege@gmail.com> 1599568164 +0100 committer Seun Lanlege <seunlanlege@gmail.com> 1604321289 +0100 gpgsig -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEECvQ02MnjnssnSbjr3HzzEhjN254FAl+gAAkACgkQ3HzzEhjN 254soAv+KO5JA0HXSe0R0XS5TnwA3IxYsW+UvdF5dXFeC3jFdGTMvor818uoBePD dxzYEsUK6gjsNcM9+hpFhoy5JnUrUPInd2BZ7pmZiDuXmYJrHi0s7K5qL0EYDoe0 m1egPNNyRR125ozJ24M+09c3OQsi3bvTx1TJaV9Aov8hK4So8UmlJTHWpkLw97ku HuTre2IPSFbV4GwJE40V+KNuDVHxaKL7zrInYScqbr6/hOTqBCvFn4ib3CjpF5HG zDAA5S2PrcbL9NQOothVcVB/TZr3IkhglCFqEjVyCX80IL0JkNZkw8jAh0B8uqXx Ug/c1/Mssa8F1jLZMmW45Cway60txqVbcWntPJAymGJbrRErOO/++oUrV0u1C65u LW7gXAaIJWQTX9KnX0SEyejNod7ubZktBz7n5WfkJAPIzdw5wtJalhLa673YTgQ9 zyTPKiWjJj2myCq1AYrJvlK8hSsIBqbBFcUf1zX4SzZWKS+5mtp51o4gfVzcCRPd z/6/iPbB =g5tx -----END PGP SIGNATURE----- BabeConsensusDataProvider works with existing chains remove integer-sqrt from node-runtime fix epoch changes

frame/system/src/lib.rs

client/api/src/execution_extensions.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

tomusdrw · 2020-11-04T13:52:25Z

@seunlanlege this PR doesn't have a single approval, can you justify why it was merged?

seunlanlege · 2020-11-04T13:55:02Z

this PR doesn't have a single approval

weird, could've sworn @bkchr approved it, which was why i was able to merge

bkchr · 2020-11-04T15:12:21Z

this PR doesn't have a single approval

weird, could've sworn @bkchr approved it, which was why i was able to merge

I did not approve anything here.

seunlanlege · 2020-11-04T15:14:55Z

Huh, merge button was green for some other reason then.

bkchr · 2020-11-04T15:22:17Z

Yeah, but that is no excuse for merging this.

seunlanlege · 2020-11-04T15:24:49Z

Aren't approvals are a requirement for merging?

* parent ba8e812 author Seun Lanlege <seunlanlege@gmail.com> 1599568164 +0100 committer Seun Lanlege <seunlanlege@gmail.com> 1604321289 +0100 gpgsig -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEECvQ02MnjnssnSbjr3HzzEhjN254FAl+gAAkACgkQ3HzzEhjN 254soAv+KO5JA0HXSe0R0XS5TnwA3IxYsW+UvdF5dXFeC3jFdGTMvor818uoBePD dxzYEsUK6gjsNcM9+hpFhoy5JnUrUPInd2BZ7pmZiDuXmYJrHi0s7K5qL0EYDoe0 m1egPNNyRR125ozJ24M+09c3OQsi3bvTx1TJaV9Aov8hK4So8UmlJTHWpkLw97ku HuTre2IPSFbV4GwJE40V+KNuDVHxaKL7zrInYScqbr6/hOTqBCvFn4ib3CjpF5HG zDAA5S2PrcbL9NQOothVcVB/TZr3IkhglCFqEjVyCX80IL0JkNZkw8jAh0B8uqXx Ug/c1/Mssa8F1jLZMmW45Cway60txqVbcWntPJAymGJbrRErOO/++oUrV0u1C65u LW7gXAaIJWQTX9KnX0SEyejNod7ubZktBz7n5WfkJAPIzdw5wtJalhLa673YTgQ9 zyTPKiWjJj2myCq1AYrJvlK8hSsIBqbBFcUf1zX4SzZWKS+5mtp51o4gfVzcCRPd z/6/iPbB =g5tx -----END PGP SIGNATURE----- BabeConsensusDataProvider works with existing chains Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

seunlanlege added the A0-please_review Pull request needs code review. label Sep 10, 2020

seunlanlege requested a review from andresilva as a code owner September 10, 2020 14:41

seunlanlege changed the title ~~Seun babe manual seal~~ Allow BabeConsensusDataProvider fork existing chain Sep 10, 2020

seunlanlege added the C1-low PR touches the given topic and has a low impact on builders. label Sep 10, 2020

github-actions bot added the A7-needspolkadotpr label Sep 11, 2020

seunlanlege added the B3-apinoteworthy label Sep 14, 2020

seunlanlege requested a review from tomusdrw September 14, 2020 07:54

tomusdrw reviewed Sep 14, 2020

View reviewed changes

github-actions bot added A7-needspolkadotpr and removed A7-needspolkadotpr labels Sep 14, 2020

bkchr reviewed Sep 18, 2020

View reviewed changes

tomusdrw suggested changes Sep 22, 2020

View reviewed changes

seunlanlege marked this pull request as draft September 22, 2020 11:58

tomusdrw reviewed Sep 24, 2020

View reviewed changes

tomusdrw approved these changes Oct 1, 2020

View reviewed changes

bkchr reviewed Oct 1, 2020

View reviewed changes

tomusdrw suggested changes Oct 1, 2020

View reviewed changes

seunlanlege added 2 commits November 2, 2020 13:49

allow BabeConsensusDataProvider fork existing chains

737f7bf

seunlanlege force-pushed the seun-babe-manual-seal branch from e227bb7 to 737f7bf Compare November 2, 2020 14:04

revert unneccessary changes

5a05bdd

seunlanlege marked this pull request as ready for review November 2, 2020 18:26

seunlanlege requested a review from athei as a code owner November 2, 2020 18:26

paritytech-ci added the D2-breaksapi label Nov 2, 2020

athei removed their request for review November 3, 2020 06:22

adds extensions method on execution extensions

24e5a1a

github-actions bot removed the A7-needspolkadotpr label Nov 3, 2020

bkchr reviewed Nov 3, 2020

View reviewed changes

frame/system/src/lib.rs Outdated Show resolved Hide resolved

client/api/src/execution_extensions.rs Outdated Show resolved Hide resolved

seunlanlege and others added 2 commits November 3, 2020 12:37

override host functions

0a88c0a

Apply suggestions from code review

8f70f05

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

seunlanlege merged commit f7a8b10 into master Nov 4, 2020

seunlanlege deleted the seun-babe-manual-seal branch November 4, 2020 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow BabeConsensusDataProvider fork existing chain #7078

Allow BabeConsensusDataProvider fork existing chain #7078

seunlanlege commented Sep 10, 2020

tomusdrw left a comment

bkchr Sep 18, 2020

tomusdrw Sep 22, 2020

tomusdrw Sep 24, 2020

tomusdrw left a comment

tomusdrw Oct 1, 2020

bkchr commented Oct 1, 2020

bkchr Oct 1, 2020

bkchr Oct 1, 2020

bkchr Oct 1, 2020

bkchr Oct 1, 2020

tomusdrw Oct 1, 2020

seunlanlege Oct 1, 2020

bkchr commented Oct 1, 2020

bkchr commented Oct 1, 2020

seunlanlege commented Oct 1, 2020

bkchr commented Oct 1, 2020

tomusdrw left a comment

tomusdrw commented Nov 4, 2020

seunlanlege commented Nov 4, 2020

bkchr commented Nov 4, 2020

seunlanlege commented Nov 4, 2020

bkchr commented Nov 4, 2020

seunlanlege commented Nov 4, 2020

	if let None = extensions.get_mut(TypeId::of::<CryptoExtension>()) {
	if extensions.get_mut(TypeId::of::<CryptoExtension>()).is_none() {

Allow BabeConsensusDataProvider fork existing chain #7078

Allow BabeConsensusDataProvider fork existing chain #7078

Conversation

seunlanlege commented Sep 10, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr commented Oct 1, 2020

bkchr commented Oct 1, 2020

seunlanlege commented Oct 1, 2020

bkchr commented Oct 1, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw commented Nov 4, 2020

seunlanlege commented Nov 4, 2020

bkchr commented Nov 4, 2020

seunlanlege commented Nov 4, 2020

bkchr commented Nov 4, 2020

seunlanlege commented Nov 4, 2020