-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Batch signature verification #5023
Changes from 1 commit
a6a9e5c
fdc73e2
00cfe23
0d5f934
0b9d1d8
cd0b459
62d9dca
5ec6d6b
8fba16a
a25d32b
69d4e82
31b2942
3c54bc3
d8169a8
d01a788
044126a
e7bc120
8c25cf0
3ba39cf
4170f13
6f29854
1d4f716
2a358e3
20ae706
fd56193
d90cf62
b477ef9
1de6a2c
ded9f5c
1482b96
5eb30aa
46267fb
6b83175
f8e67ef
83b73f7
e16798e
0c54540
b1847a3
e439fed
ffe2250
5285c2b
e210b4b
0701c19
86bdcbe
1abe308
63f9df3
76a0689
4f835c6
4d55306
bbdec24
bfafa25
58e2cb1
65e86a8
70d3803
3eb55ff
87cf296
de66ee1
a593928
52e5ada
baf967b
4d5888e
394497d
d015c60
c2c9d2d
25c1fc1
6ae4dc5
b85e037
d68aebb
ab32c60
ba8ce75
0db24f4
b35c3aa
3d10f24
34499e4
d522248
0d9d09f
6c4f6b5
48fb7d6
7f3ebae
766ec69
49e9c67
a3979f2
b4cb58f
234ec99
1db62b0
f7097d6
e823c63
056515e
86d7efc
0e2b455
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -476,15 +476,30 @@ pub trait Crypto { | |
| } | ||
| } | ||
|
|
||
| /// Start verification extension. | ||
| fn start_batch_verify(&mut self) { | ||
| let scheduler = self.extension::<TaskExecutorExt>() | ||
| .expect("Task executor extension is required!") | ||
NikVolf marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| .0 | ||
| .clone(); | ||
|
|
||
| self.register_extension(VerificationExt(BatchVerifier::new(scheduler))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to register a new extension each time or statically have one could be enough, start would simply set a new batch verifier in this static extension (by static I mean in the same way as TaskExecutorExt, for instance in StateMachine::new). CC\ @tomusdrw who will probably have a better opinion than mine.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need this extension in other situations (runtime api calls, transaction validation, block construction), so this is the idea to create it dynamically on demand :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and on_demand is 'start_verify' call from frame executive, starts to make sense (we need a way to know if batch start, and batch start means it is a valid context, so registering extension is a generic way to check that). |
||
| .expect("Failed to register required extension: VerificationExt"); | ||
NikVolf marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /// Batch-verify signatures. | ||
| /// | ||
| /// Verify all signatures which were previously pushed in batch. | ||
| /// Use `batch_push_ed25519`/`batch_push_sr25519` to push collection of signatures | ||
NikVolf marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// to the host, and then use this function to verify them all at once. | ||
| fn batch_verify(&mut self) -> bool { | ||
| self.extension::<VerificationExt>() | ||
| fn finish_batch_verify(&mut self) -> bool { | ||
| let result = self.extension::<VerificationExt>() | ||
| .expect("No verification extension in current context!") | ||
NikVolf marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| .verify_and_clear() | ||
| .verify_and_clear(); | ||
|
|
||
| self.deregister_extension::<VerificationExt>(); | ||
|
|
||
| result | ||
| } | ||
|
|
||
| /// Returns all `sr25519` public keys for the given key id from the keystore. | ||
|
|
@@ -630,25 +645,6 @@ sp_externalities::decl_extension! { | |
| pub struct VerificationExt(BatchVerifier); | ||
| } | ||
|
|
||
NikVolf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// Interface for activating dynamic extensions. | ||
| #[runtime_interface] | ||
| pub trait Extensions { | ||
| /// Start verification extension. | ||
| fn start_verification_extension(&mut self) { | ||
| let scheduler = self.extension::<TaskExecutorExt>() | ||
| .expect("Task executor extension is required!") | ||
| .0 | ||
| .clone(); | ||
|
|
||
| self.register_extension(VerificationExt(BatchVerifier::new(scheduler))) | ||
| .expect("Failed to register required extension: VerificationExt"); | ||
| } | ||
|
|
||
| /// Remove and drop verification extension. | ||
| fn drop_verification_extension(&mut self) { | ||
| self.deregister_extension::<VerificationExt>(); | ||
| } | ||
| } | ||
|
|
||
| /// Interface that provides functions to access the offchain functionality. | ||
| #[runtime_interface] | ||
|
|
@@ -1025,7 +1021,6 @@ pub type SubstrateHostFunctions = ( | |
| logging::HostFunctions, | ||
| sandbox::HostFunctions, | ||
| crate::trie::HostFunctions, | ||
| extensions::HostFunctions, | ||
| ); | ||
|
|
||
| #[cfg(test)] | ||
|
|
@@ -1101,13 +1096,13 @@ mod tests { | |
| fn dynamic_extensions_work() { | ||
| let mut ext = BasicExternalities::with_tasks_executor(); | ||
| ext.execute_with(|| { | ||
| extensions::start_verification_extension(); | ||
| crypto::start_batch_verify(); | ||
| }); | ||
|
|
||
| assert!(ext.extensions().get_mut(TypeId::of::<VerificationExt>()).is_some()); | ||
|
|
||
| ext.execute_with(|| { | ||
| extensions::drop_verification_extension(); | ||
| crypto::finish_batch_verify(); | ||
| }); | ||
|
|
||
| assert!(ext.extensions().get_mut(TypeId::of::<VerificationExt>()).is_none()); | ||
|
|
@@ -1117,7 +1112,7 @@ mod tests { | |
| fn long_sr25519_batching_works() { | ||
| let mut ext = BasicExternalities::with_tasks_executor(); | ||
| ext.execute_with(|| { | ||
| extensions::start_verification_extension(); | ||
| crypto::start_batch_verify(); | ||
| let pair = sr25519::Pair::generate_with_phrase(None).0; | ||
| for it in 0..70 { | ||
| let msg = format!("Schnorrkel {}!", it); | ||
|
|
@@ -1131,32 +1126,33 @@ mod tests { | |
| &Vec::new(), | ||
| &Default::default(), | ||
| ); | ||
| assert!(!crypto::batch_verify()); | ||
| assert!(!crypto::finish_batch_verify()); | ||
|
|
||
| crypto::start_batch_verify(); | ||
| for it in 0..70 { | ||
| let msg = format!("Schnorrkel {}!", it); | ||
| let signature = pair.sign(msg.as_bytes()); | ||
| crypto::batch_push_sr25519(&signature, msg.as_bytes(), &pair.public()); | ||
| } | ||
| assert!(crypto::batch_verify()); | ||
| assert!(crypto::finish_batch_verify()); | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| fn batching_works() { | ||
| let mut ext = BasicExternalities::with_tasks_executor(); | ||
| ext.execute_with(|| { | ||
| extensions::start_verification_extension(); | ||
|
|
||
| crypto::start_batch_verify(); | ||
| // invalid ed25519 signature | ||
| crypto::batch_push_ed25519( | ||
| &Default::default(), | ||
| &Vec::new(), | ||
| &Default::default(), | ||
| ); | ||
| assert!(!crypto::batch_verify()); | ||
| assert!(!crypto::finish_batch_verify()); | ||
|
|
||
| // 2 valid ed25519 signatures | ||
| crypto::start_batch_verify(); | ||
| let pair = ed25519::Pair::generate_with_phrase(None).0; | ||
| let msg = b"Important message"; | ||
| let signature = pair.sign(msg); | ||
|
|
@@ -1167,9 +1163,10 @@ mod tests { | |
| let signature = pair.sign(msg); | ||
| crypto::batch_push_ed25519(&signature, msg, &pair.public()); | ||
|
|
||
| assert!(crypto::batch_verify()); | ||
| assert!(crypto::finish_batch_verify()); | ||
|
|
||
| // 1 valid, 1 invalid ed25519 signature | ||
| crypto::start_batch_verify(); | ||
| let pair = ed25519::Pair::generate_with_phrase(None).0; | ||
| let msg = b"Important message"; | ||
| let signature = pair.sign(msg); | ||
|
|
@@ -1180,9 +1177,10 @@ mod tests { | |
| &Vec::new(), | ||
| &Default::default(), | ||
| ); | ||
| assert!(!crypto::batch_verify()); | ||
| assert!(!crypto::finish_batch_verify()); | ||
|
|
||
| // 1 valid ed25519, 2 valid sr25519 | ||
| crypto::start_batch_verify(); | ||
| let pair = ed25519::Pair::generate_with_phrase(None).0; | ||
| let msg = b"Ed25519 batching"; | ||
| let signature = pair.sign(msg); | ||
|
|
@@ -1198,9 +1196,10 @@ mod tests { | |
| let signature = pair.sign(msg); | ||
| crypto::batch_push_sr25519(&signature, msg, &pair.public()); | ||
|
|
||
| assert!(crypto::batch_verify()); | ||
| assert!(crypto::finish_batch_verify()); | ||
|
|
||
| // 1 valid sr25519, 1 invalid sr25519 | ||
| crypto::start_batch_verify(); | ||
| let pair = sr25519::Pair::generate_with_phrase(None).0; | ||
| let msg = b"Schnorrkel!"; | ||
| let signature = pair.sign(msg); | ||
|
|
@@ -1211,7 +1210,7 @@ mod tests { | |
| &Vec::new(), | ||
| &Default::default(), | ||
| ); | ||
| assert!(!crypto::batch_verify()); | ||
| assert!(!crypto::finish_batch_verify()); | ||
| }); | ||
| } | ||
| } | ||
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 really needs better justification. Obviously, this panic is reachable, so there needs to be an explanation for why panicking here is justified.
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.
It should panic in runtime AFAIK
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 code looks very much strange comparing to the rest of the runtime, but if I have understood correctly ant this is only being used for block execution (not validation and production) then if we are trying to import a block with even a single invalid signature, then yes panic it is.