Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Batch signature verification #5023

Merged
merged 90 commits into from
Apr 16, 2020
Merged
Show file tree
Hide file tree
Changes from 75 commits
Commits
Show all changes
90 commits
Select commit Hold shift + click to select a range
a6a9e5c
create parallel tasks extension
NikVolf Mar 14, 2020
fdc73e2
make type system happy
NikVolf Feb 22, 2020
00cfe23
basic externalities
NikVolf Feb 22, 2020
0d5f934
test for dynamic extensions
NikVolf Feb 22, 2020
0b9d1d8
batching test
NikVolf Feb 22, 2020
cd0b459
remove premature verify_batch
NikVolf Feb 22, 2020
62d9dca
shnschnorrkel batch
NikVolf Feb 22, 2020
5ec6d6b
alter test
NikVolf Feb 22, 2020
8fba16a
shnschnorrkel test
NikVolf Feb 22, 2020
a25d32b
executive batching
NikVolf Feb 22, 2020
69d4e82
some docs
NikVolf Feb 22, 2020
31b2942
also multi/any signatgures
NikVolf Feb 22, 2020
3c54bc3
error propagation
NikVolf Feb 23, 2020
d8169a8
styling
NikVolf Feb 23, 2020
d01a788
make verification extension optional
NikVolf Feb 23, 2020
044126a
experimental ed25519 parallelization
NikVolf Feb 26, 2020
e7bc120
some merge fallout
NikVolf Mar 14, 2020
8c25cf0
utilize task executor
NikVolf Mar 14, 2020
3ba39cf
merge fallout
NikVolf Mar 16, 2020
4170f13
utilize task executor more
NikVolf Mar 16, 2020
6f29854
another merge fallout
NikVolf Mar 16, 2020
1d4f716
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Mar 16, 2020
2a358e3
feature-gate sp-io
NikVolf Mar 17, 2020
20ae706
arrange toml
NikVolf Mar 17, 2020
fd56193
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Mar 25, 2020
d90cf62
fix no-std
NikVolf Mar 25, 2020
b477ef9
sr25519 batching and refactoring
NikVolf Mar 25, 2020
1de6a2c
add docs
NikVolf Mar 25, 2020
ded9f5c
fix name
NikVolf Mar 25, 2020
1482b96
add newline
NikVolf Mar 25, 2020
5eb30aa
fix block import test
NikVolf Mar 25, 2020
46267fb
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Mar 25, 2020
6b83175
long sr25519 test
NikVolf Mar 26, 2020
f8e67ef
blocking instead of parking
NikVolf Mar 26, 2020
83b73f7
move everything in crypto
NikVolf Mar 26, 2020
e16798e
return batch_verify to check :)
NikVolf Mar 26, 2020
0c54540
use condvars
NikVolf Mar 26, 2020
b1847a3
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Mar 26, 2020
e439fed
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Mar 26, 2020
ffe2250
use multi-threaded executor for benches
NikVolf Mar 27, 2020
5285c2b
don't call via host interface
NikVolf Mar 27, 2020
e210b4b
try no spawning
NikVolf Mar 27, 2020
0701c19
add true
NikVolf Mar 27, 2020
86bdcbe
cleanup
NikVolf Apr 1, 2020
1abe308
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensio…
NikVolf Apr 1, 2020
63f9df3
straighten batching
NikVolf Apr 3, 2020
76a0689
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 3, 2020
4f835c6
remove signature check from this test (?)
NikVolf Apr 3, 2020
4d55306
remove now pointless test
NikVolf Apr 3, 2020
bbdec24
remove another now useless test
NikVolf Apr 3, 2020
bfafa25
fix warnings
NikVolf Apr 6, 2020
58e2cb1
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 6, 2020
65e86a8
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 6, 2020
70d3803
Revert "remove another now useless test"
NikVolf Apr 7, 2020
3eb55ff
rethink the sp-io-part
NikVolf Apr 7, 2020
87cf296
Revert "remove now pointless test"
NikVolf Apr 7, 2020
de66ee1
fix wording
NikVolf Apr 7, 2020
a593928
add wording
NikVolf Apr 7, 2020
52e5ada
add todo and fix
NikVolf Apr 7, 2020
baf967b
return check and fix
NikVolf Apr 7, 2020
4d5888e
add logging in sp-io
NikVolf Apr 8, 2020
394497d
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 8, 2020
d015c60
Update primitives/io/src/batch_verifier.rs
NikVolf Apr 9, 2020
c2c9d2d
address review and use std condvar
NikVolf Apr 9, 2020
25c1fc1
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 9, 2020
6ae4dc5
account for early exit
NikVolf Apr 13, 2020
b85e037
address reivew
NikVolf Apr 13, 2020
d68aebb
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 14, 2020
ab32c60
address review
NikVolf Apr 14, 2020
ba8ce75
more suggestions
NikVolf Apr 14, 2020
0db24f4
add docs for batch verification
NikVolf Apr 14, 2020
b35c3aa
remove unused
NikVolf Apr 14, 2020
3d10f24
more review suggestions
NikVolf Apr 14, 2020
34499e4
move to sp-runtime
NikVolf Apr 14, 2020
d522248
add expects
NikVolf Apr 14, 2020
0d9d09f
remove blocks
NikVolf Apr 14, 2020
6c4f6b5
use entry
NikVolf Apr 14, 2020
48fb7d6
Update primitives/io/src/batch_verifier.rs
NikVolf Apr 15, 2020
7f3ebae
Update primitives/externalities/src/extensions.rs
NikVolf Apr 15, 2020
766ec69
update overlooked note
NikVolf Apr 15, 2020
49e9c67
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 15, 2020
a3979f2
remove stupid return
NikVolf Apr 15, 2020
b4cb58f
Update primitives/io/src/lib.rs
NikVolf Apr 15, 2020
234ec99
Update primitives/io/src/lib.rs
NikVolf Apr 15, 2020
1db62b0
fix wording
NikVolf Apr 15, 2020
f7097d6
bump spec_version
NikVolf Apr 15, 2020
e823c63
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 15, 2020
056515e
Merge branch 'master' into nv-dynamic-extensions
NikVolf Apr 16, 2020
86d7efc
Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions
NikVolf Apr 16, 2020
0e2b455
Merge branch 'nv-dynamic-extensions' of github.com:paritytech/substra…
NikVolf Apr 16, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions bin/node/testing/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ sp-blockchain = { version = "2.0.0-alpha.5", path = "../../../primitives/blockch
log = "0.4.8"
tempfile = "3.1.0"
fs_extra = "1"
futures = "0.3.1"

[dev-dependencies]
criterion = "0.3.0"
Expand Down
39 changes: 35 additions & 4 deletions bin/node/testing/src/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ use node_runtime::{
AccountId,
Signature,
};
use sp_core::{ExecutionContext, blake2_256};
use sp_core::{ExecutionContext, blake2_256, traits::CloneableSpawn};
use sp_api::ProvideRuntimeApi;
use sp_block_builder::BlockBuilder;
use sp_inherents::InherentData;
Expand All @@ -57,6 +57,7 @@ use sc_client_api::{
};
use sp_core::{Pair, Public, sr25519, ed25519};
use sc_block_builder::BlockBuilderProvider;
use futures::{executor, task};

/// Keyring full of accounts for benching.
///
Expand Down Expand Up @@ -142,6 +143,36 @@ impl BlockType {
}
}

/// Benchmarking task executor.
///
/// Uses multiple threads as the regular executable.
#[derive(Debug, Clone)]
pub struct TaskExecutor {
pool: executor::ThreadPool,
}

impl TaskExecutor {
fn new() -> Self {
Self {
pool: executor::ThreadPool::new()
.expect("Failed to create task executor")
}
}
}

impl task::Spawn for TaskExecutor {
fn spawn_obj(&self, future: task::FutureObj<'static, ()>)
-> Result<(), task::SpawnError> {
self.pool.spawn_obj(future)
}
}

impl CloneableSpawn for TaskExecutor {
fn clone(&self) -> Box<dyn CloneableSpawn> {
Box::new(Clone::clone(self))
}
}

impl BenchDb {
/// New immutable benchmarking database.
///
Expand All @@ -168,8 +199,8 @@ impl BenchDb {
/// and keep it there until struct is dropped.
///
/// You can `clone` this database or you can `create_context` from it
/// (which also do `clone`) to run actual operation against new database
/// which will be identical to this.
/// (which also does `clone`) to run actual operation against new database
/// which will be identical to the original.
pub fn new(keyring_length: usize) -> Self {
Self::with_key_types(keyring_length, KeyTypes::Sr25519)
}
Expand Down Expand Up @@ -197,7 +228,7 @@ impl BenchDb {
None,
None,
ExecutionExtensions::new(profile.into_execution_strategies(), None),
sp_core::tasks::executor(),
Box::new(TaskExecutor::new()),
None,
).expect("Should not fail");

Expand Down
5 changes: 2 additions & 3 deletions client/transaction-pool/src/testing/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use futures::executor::block_on;
use txpool::{self, Pool};
use sp_runtime::{
generic::BlockId,
transaction_validity::{ValidTransaction, InvalidTransaction, TransactionSource},
transaction_validity::{ValidTransaction, TransactionSource, InvalidTransaction},
};
use substrate_test_runtime_client::{
runtime::{Block, Hash, Index, Header, Extrinsic, Transfer},
Expand Down Expand Up @@ -263,7 +263,6 @@ fn should_not_retain_invalid_hashes_from_retracted() {
let event = block_event_with_retracted(1, vec![retracted_hash]);

block_on(pool.maintain(event));
// maintenance is in background
block_on(notifier.next());

assert_eq!(pool.status().ready, 0);
Expand Down Expand Up @@ -701,6 +700,6 @@ fn should_not_accept_old_signatures() {
Err(error::Error::Pool(
sp_transaction_pool::error::Error::InvalidTransaction(InvalidTransaction::BadProof)
)),
"Should be invalid transactiono with bad proof",
"Should be invalid transaction with bad proof",
);
}
3 changes: 2 additions & 1 deletion frame/executive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ frame-system = { version = "2.0.0-alpha.5", default-features = false, path = "..
serde = { version = "1.0.101", optional = true }
sp-runtime = { version = "2.0.0-alpha.5", default-features = false, path = "../../primitives/runtime" }
sp-std = { version = "2.0.0-alpha.5", default-features = false, path = "../../primitives/std" }
sp-io = { version = "2.0.0-alpha.5", default-features = false, path = "../../primitives/io" }

[dev-dependencies]
hex-literal = "0.2.1"
sp-core = { version = "2.0.0-alpha.5", path = "../../primitives/core" }
sp-io ={ path = "../../primitives/io", version = "2.0.0-alpha.5"}
pallet-indices = { version = "2.0.0-alpha.5", path = "../indices" }
pallet-balances = { version = "2.0.0-alpha.5", path = "../balances" }
pallet-transaction-payment = { version = "2.0.0-alpha.5", path = "../transaction-payment" }
Expand All @@ -34,6 +34,7 @@ std = [
"serde",
"sp-runtime/std",
"sp-std/std",
"sp-io/std",
]

[package.metadata.docs.rs]
Expand Down
4 changes: 4 additions & 0 deletions frame/executive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,13 @@ where
// any initial checks
Self::initial_checks(&block);

let batching_safeguard = sp_runtime::SignatureBatching::start();
// execute extrinsics
let (header, extrinsics) = block.deconstruct();
Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number());
if !sp_runtime::SignatureBatching::verify(batching_safeguard) {
panic!("Signature verification failed.");
}

// any final checks
Self::final_checks(&header);
Expand Down
4 changes: 3 additions & 1 deletion primitives/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ sha2 = { version = "0.8.0", default-features = false, optional = true }
hex = { version = "0.4", default-features = false, optional = true }
twox-hash = { version = "1.5.0", default-features = false, optional = true }
libsecp256k1 = { version = "0.3.2", default-features = false, features = ["hmac"], optional = true }
merlin = { version = "2.0", default-features = false, optional = true }

sp-runtime-interface = { version = "2.0.0-alpha.5", default-features = false, path = "../runtime-interface" }

Expand Down Expand Up @@ -94,7 +95,6 @@ std = [
"schnorrkel/std",
"regex",
"num-traits/std",
"libsecp256k1/std",
NikVolf marked this conversation as resolved.
Show resolved Hide resolved
"tiny-keccak",
"sp-debug-derive/std",
"sp-externalities",
Expand All @@ -103,6 +103,7 @@ std = [
"zeroize/alloc",
"futures",
"futures/thread-pool",
"libsecp256k1/std",
]

# This feature enables all crypto primitives for `no_std` builds like microcontrollers
Expand All @@ -118,6 +119,7 @@ full_crypto = [
"twox-hash",
"libsecp256k1",
"sp-runtime-interface/disable_target_static_assertions",
"merlin",
]

[package.metadata.docs.rs]
Expand Down
39 changes: 39 additions & 0 deletions primitives/core/src/sr25519.rs
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,45 @@ impl CryptoType for Pair {
type Pair = Pair;
}

/// Batch verification.
NikVolf marked this conversation as resolved.
Show resolved Hide resolved
///
/// `messages`, `signatures` and `pub_keys` should all have equal length.
///
/// Returns `true` if all signatures are correct, `false` otherwise.
#[cfg(feature = "std")]
pub fn verify_batch(
messages: Vec<&[u8]>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make these 3 parameters impl Iterator<&[u8]> etc, we could prevent some allocations.

Copy link
Contributor Author

@NikVolf NikVolf Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but schnorrkel expects slice and IntoIterator :(

https://docs.rs/schnorrkel/0.9.1/schnorrkel/fn.verify_batch.html

signatures: Vec<&Signature>,
pub_keys: Vec<&Public>,
) -> bool {
let mut sr_pub_keys = Vec::with_capacity(pub_keys.len());
for pub_key in pub_keys {
match schnorrkel::PublicKey::from_bytes(pub_key.as_ref()) {
Ok(pk) => sr_pub_keys.push(pk),
Err(_) => return false,
};
}

let mut sr_signatures = Vec::with_capacity(signatures.len());
for signature in signatures {
match schnorrkel::Signature::from_bytes(signature.as_ref()) {
Ok(s) => sr_signatures.push(s),
Err(_) => return false
};
}

let mut messages: Vec<merlin::Transcript> = messages.into_iter().map(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be cool to avoid adding merlin dependency, but maybe it is not doable without changing upstream schnorkell.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed I can't see how it is possible

|msg| signing_context(SIGNING_CTX).bytes(msg)
).collect();

schnorrkel::verify_batch(
&mut messages,
&sr_signatures,
&sr_pub_keys,
true,
).is_ok()
}

#[cfg(test)]
mod compatibility_test {
use super::*;
Expand Down
31 changes: 31 additions & 0 deletions primitives/externalities/src/extensions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
//! It is required that each extension implements the [`Extension`] trait.

use std::{collections::HashMap, any::{Any, TypeId}, ops::DerefMut};
use crate::Error;

/// Marker trait for types that should be registered as [`Externalities`](crate::Externalities) extension.
///
Expand Down Expand Up @@ -87,6 +88,16 @@ pub trait ExtensionStore {
/// It is advised to use [`ExternalitiesExt::extension`](crate::ExternalitiesExt::extension)
/// instead of this function to get type system support and automatic type downcasting.
fn extension_by_type_id(&mut self, type_id: TypeId) -> Option<&mut dyn Any>;

/// Register extension `extension` with speciifed `type_id`.
///
/// It should return error if extension is already registered.
fn register_extension_with_type_id(&mut self, type_id: TypeId, extension: Box<dyn Extension>) -> Result<(), Error>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why we need specific register extension methods here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you suggest the change that allows to avoid it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be using a VerificationExt the same way you got a TaskExecutorExt, (init will be in StateMachine::new).
The content of VerificationExt will be optional. So it can be activated/instantiated on start_verify in the same way it is currently activated by registering an new extension.
This means, as you mention, that in some case a VerificationExt will be accessible to context where we do not want it but it will not do much as it will be None.
So I think the new register functions allow to implement this kind of mechanism in a more generic way, that can be fine, I am just not sure if it fits the design for Extension (@tomusdrw or someone else more familiar with extension design could say).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, dynamic extensions (which you can register and deregister, and do not need to bookkeep every one of them in StateMachine) is one of the stretch goals of the PR (see branch name :) )

NikVolf marked this conversation as resolved.
Show resolved Hide resolved

/// Deregister extension with speicifed 'type_id' and drop it.
///
/// It should return error if extension is not registered.
fn deregister_extension_by_type_id(&mut self, type_id: TypeId) -> Result<(), Error>;
}

/// Stores extensions that should be made available through the externalities.
Expand All @@ -95,6 +106,12 @@ pub struct Extensions {
extensions: HashMap<TypeId, Box<dyn Extension>>,
}

impl std::fmt::Debug for Extensions {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "Extensions: ({})", self.extensions.len())
}
}

impl Extensions {
/// Create new instance of `Self`.
pub fn new() -> Self {
Expand All @@ -106,10 +123,24 @@ impl Extensions {
self.extensions.insert(ext.type_id(), Box::new(ext));
}

/// Register extension `ext`.
pub fn register_with_type_id(&mut self, type_id: TypeId, extension: Box<dyn Extension>) -> Result<(), Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not have a separate type_id parameter. Instead, we should make Extension a subtrait of Any.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide an example?

if self.extensions.contains_key(&type_id) {
NikVolf marked this conversation as resolved.
Show resolved Hide resolved
return Err(Error::ExtensionAlreadyRegistered);
}
self.extensions.insert(type_id, extension);
Ok(())
}

/// Return a mutable reference to the requested extension.
pub fn get_mut(&mut self, ext_type_id: TypeId) -> Option<&mut dyn Any> {
self.extensions.get_mut(&ext_type_id).map(DerefMut::deref_mut).map(Extension::as_mut_any)
}

/// Deregister extension of type `E`.
pub fn deregister(&mut self, type_id: TypeId) -> Option<Box<dyn Extension>> {
self.extensions.remove(&type_id)
}
}

#[cfg(test)]
Expand Down
30 changes: 30 additions & 0 deletions primitives/externalities/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ pub use extensions::{Extension, Extensions, ExtensionStore};
mod extensions;
mod scope_limited;

/// Externalities error.
#[derive(Debug)]
pub enum Error {
/// Same extension cannot be registered twice.
ExtensionAlreadyRegistered,
/// Extensions are not supported.
ExtensionsAreNotSupported,
/// Extension `TypeId` is not registered.
ExtensionIsNotRegistered(TypeId),
}

/// The Substrate externalities.
///
/// Provides access to the storage and to other registered extensions.
Expand Down Expand Up @@ -198,10 +209,29 @@ pub trait Externalities: ExtensionStore {
pub trait ExternalitiesExt {
/// Tries to find a registered extension and returns a mutable reference.
fn extension<T: Any + Extension>(&mut self) -> Option<&mut T>;

/// Register extension `ext`.
///
/// Should return error if extension is already registered or extensions are not supported.
fn register_extension<T: Extension>(&mut self, ext: T) -> Result<(), Error>;
NikVolf marked this conversation as resolved.
Show resolved Hide resolved

/// Deregister and drop extension of `T` type.
///
/// Should return error if extension of type `T` is not registered or
/// extensions are not supported.
fn deregister_extension<T: Extension>(&mut self) -> Result<(), Error>;
}

impl ExternalitiesExt for &mut dyn Externalities {
fn extension<T: Any + Extension>(&mut self) -> Option<&mut T> {
self.extension_by_type_id(TypeId::of::<T>()).and_then(Any::downcast_mut)
}

fn register_extension<T: Extension>(&mut self, ext: T) -> Result<(), Error> {
self.register_extension_with_type_id(TypeId::of::<T>(), Box::new(ext))
}

fn deregister_extension<T: Extension>(&mut self) -> Result<(), Error> {
self.deregister_extension_by_type_id(TypeId::of::<T>())
}
}
4 changes: 4 additions & 0 deletions primitives/io/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ sp-runtime-interface = { version = "2.0.0-alpha.5", default-features = false, pa
sp-trie = { version = "2.0.0-alpha.5", optional = true, path = "../../primitives/trie" }
sp-externalities = { version = "0.8.0-alpha.5", optional = true, path = "../externalities" }
log = { version = "0.4.8", optional = true }
futures = { version = "0.3.1", features = ["thread-pool"], optional = true }
parking_lot = { version = "0.10.0", optional = true }

[features]
default = ["std"]
Expand All @@ -37,6 +39,8 @@ std = [
"sp-externalities",
"sp-wasm-interface/std",
"log",
"futures",
"parking_lot",
]

# These two features are used for `no_std` builds for the environments which already provides
Expand Down
Loading