Batch signature verification #5023

NikVolf · 2020-02-22T19:35:14Z

polkadot companion: paritytech/polkadot#939

NikVolf · 2020-02-22T20:33:14Z

/bench

parity-benchapp · 2020-02-22T20:33:15Z

Finished benchmark for branch: nv-dynamic-extensions

MASTER RESULT

import block/Wasm time:
[58.580 ms 58.687 ms 58.795 ms]

import block/Native time:
[20.870 ms 20.958 ms 21.057 ms]

BRANCH RESULT

import block/Wasm time:
[54.505 ms 54.596 ms 54.683 ms]
change:
[-7.3724% -7.0200% -6.6699%] (p = 0.00 < 0.05)
Performance has improved.

import block/Native time:
[18.150 ms 18.226 ms 18.298 ms]
change:
[-13.665% -12.992% -12.310%] (p = 0.00 < 0.05)
Performance has improved.

burdges · 2020-02-23T02:47:28Z

We should put some benchmarks in schnorrkel for this so that we have a comparison. Can I ask @demimarie-parity to fix the existing ones?

burdges · 2020-02-23T03:00:03Z

As discussed in dalek-cryptography/ed25519-dalek#117 we seemingly need deterministic batch verification for ed25519 since you can craft invalid blocks that's invalid for only some validators using its current form. We avoid this problem in schnorrkel by avoiding the cofactor. Yay Decaf!

parity-benchapp · 2020-02-23T06:20:11Z

Finished benchmark for branch: nv-dynamic-extensions

MASTER RESULT

import block/Wasm time:
[58.193 ms 58.269 ms 58.339 ms]

import block/Native time:
[20.671 ms 20.762 ms 20.845 ms]

BRANCH RESULT

import block/Wasm time:
[54.525 ms 54.595 ms 54.668 ms]
change:
[-6.4276% -6.1534% -5.8591%] (p = 0.00 < 0.05)
Performance has improved.

import block/Native time:
[18.018 ms 18.118 ms 18.227 ms]
change:
[-14.066% -13.136% -12.297%] (p = 0.00 < 0.05)
Performance has improved.

NikVolf · 2020-02-23T08:04:00Z

NikVolf deleted a comment from parity-benchapp

that was bench before ab94831 where batching was effectively activated

frame/executive/src/lib.rs

bkchr · 2020-02-25T13:22:06Z

Just for testing, I think it would be nice to see a version that "dispatches" the verification to a background thread. The background thread could just verify signature by signature. However, as this is done in parallel to the execution of the transaction. We may can get even a better performance than blocking at the end for batch verify.

NikVolf · 2020-02-25T15:02:39Z

Just for testing, I think it would be nice to see a version that "dispatches" the verification to a background thread. The background thread could just verify signature by signature. However, as this is done in parallel to the execution of the transaction. We may can get even a better performance than blocking at the end for batch verify.

For sr25519/ed25519 part, I'm thinking of maybe even better approach - figure out optimal batch size (it is variable afaik, so 100 tx can be more cheap to batch-verify per-tx than 200, for example - @burdges ?), and then send batch of optimal size to the background thread once it is reached. I think it will be more robust on low-tier machines and will cost less computing power.

For ECDSA yes, I will add what you are proposing

bkchr · 2020-02-25T15:15:45Z

What happens when we don't reach this batch size? I think the batch size should be much smaller to make sure we really dispatch it multiple times.

NikVolf · 2020-02-25T15:27:08Z

What happens when we don't reach this batch size? I think the batch size should be much smaller to make sure we really dispatch it multiple times.

It will just block-verify with suboptimal batch size at the end. It will be suboptimal in terms of time than doing everything at the background immediately, but it means that block are very far from full, and we don't need to waste computing power to try to import them as fast as possible?

bkchr · 2020-02-25T15:35:45Z

The whole work isn't required when we talk about the current, almost empty blocks :D This optimizations will just be really useful for full blocks.

In general I just wanted to know what would be the difference when we import in the background while we execute the transactions. Maybe it is total useless and don't bring that much.

NikVolf · 2020-02-25T15:42:14Z

The whole work isn't required when we talk about the current, almost empty blocks :D This optimizations will just be really useful for full blocks.

This is my point also, yes :)

In general I just wanted to know what would be the difference when we import in the background while we execute the transactions. Maybe it is total useless and don't bring that much.

I will use your approach for ed25519 (it's batching is buggy anyway) and make a benchmarking run just for that. I don't think it is useless, I think theoretical answer to that that it will effectively remove signature verification from the main pipeline and give time performance boost of ~20% instead of current 13%.

Demi-Marie · 2020-02-25T19:59:48Z

I filed w3f/schnorrkel#54 to fix the benchmarks @burdges.

Demi-Marie · 2020-02-25T22:12:31Z

frame/executive/src/lib.rs

 		// execute extrinsics
 		let (header, extrinsics) = block.deconstruct();
 		Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number());

+		if !sp_io::crypto::batch_verify() {
+			panic!("Signature verification failed.")


This really needs better justification. Obviously, this panic is reachable, so there needs to be an explanation for why panicking here is justified.

It should panic in runtime AFAIK

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.

Demi-Marie · 2020-02-25T22:48:24Z

primitives/externalities/src/extensions.rs

@@ -106,10 +117,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> {


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

Can you provide an example?

primitives/io/src/batch_verifier.rs

primitives/io/src/lib.rs

burdges · 2020-02-28T07:58:20Z

An auditor noticed nodes gossiping bad transactions consumes too much CPU. Among the solutions is rate limiting gossiped transactions per gossip source. If we know the gossip source, and can assume the source checked all signatures, then we can batch verify transaction signatures gossiped from the same source. We require the Ed25519 batch_deterministic feature for this too.

We could benefit even more with other gossip messages signed by different nodes. We might want a more general infrastructure for this, ala https://www.zfnd.org/blog/futures-batch-verification/

bkchr · 2020-02-28T08:21:22Z

An auditor noticed nodes gossiping bad transactions consumes too much CPU. Among the solutions is rate limiting gossiped transactions per gossip source. If we know the gossip source, and can assume the source checked all signatures, then we can batch verify transaction signatures gossiped from the same source. We require the Ed25519 batch_deterministic feature for this too.

We could benefit even more with other gossip messages signed by different nodes. We might want a more general infrastructure for this, ala https://www.zfnd.org/blog/futures-batch-verification/

I'm not sure about the current implementation, but we should use the same reputation system as it is used for sync. If a node sends a transaction with an invalid signature, it should be instantly disconnected.

NikVolf · 2020-02-28T08:48:01Z

An auditor noticed nodes gossiping bad transactions consumes too much CPU.

Can I read this auditor report?

NikVolf · 2020-02-28T11:11:31Z

/bench ed25519

parity-benchapp · 2020-02-28T11:11:32Z

Finished benchmark for branch: nv-dynamic-extensions

Master: Wasm: 62.26 ms (+/- 0.14 ms)
Master: Native: 22.85 ms (+/- 0.11 ms)
Branch: Wasm: 56.60 ms (+/- 0.13 ms)
Branch: Native: 18.22 ms (+/- 0.08 ms)
Change: Wasm: -9.09%
Change: Native: -20.28%

NikVolf · 2020-02-28T12:02:04Z

Finished benchmark for branch: nv-dynamic-extensions

Master: Wasm: 62.26 ms (+/- 0.14 ms)
Master: Native: 22.85 ms (+/- 0.11 ms)
Branch: Wasm: 56.60 ms (+/- 0.13 ms)
Branch: Native: 18.22 ms (+/- 0.08 ms)
Change: Wasm: -9.09%
Change: Native: -20.28%

as predicted @bkchr 😅

NikVolf · 2020-02-28T12:08:04Z

/bench import

parity-benchapp · 2020-02-28T12:08:06Z

Finished benchmark for branch: nv-dynamic-extensions

Benchmark: Import Benchmark (random transfers)

Master: Wasm: 61.76 ms (+/- 0.07 ms)
Master: Native: 23.48 ms (+/- 0.08 ms)
Branch: Wasm: 57.09 ms (+/- 0.17 ms)
Branch: Native: 20.08 ms (+/- 0.08 ms)
Change: Wasm: -7.56%
Change: Native: -14.48%

primitives/io/src/lib.rs

bkchr

Some last nitpicks and than we are good to go ;)

primitives/externalities/src/extensions.rs

primitives/io/src/batch_verifier.rs

primitives/io/src/lib.rs

bkchr · 2020-04-14T21:57:43Z

primitives/core/src/sr25519.rs

+/// Returns `true` if all signatures are correct, `false` otherwise.
+#[cfg(feature = "std")]
+pub fn verify_batch(
+	messages: Vec<&[u8]>,


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

but schnorrkel expects slice and IntoIterator :(

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

Co-Authored-By: Bastian Köcher <[email protected]>

# Conflicts: # frame/executive/Cargo.toml

primitives/io/src/lib.rs

Co-Authored-By: Bastian Köcher <[email protected]>

# Conflicts: # bin/node/runtime/src/lib.rs # frame/executive/Cargo.toml

# Conflicts: # bin/node/runtime/src/lib.rs

…te into nv-dynamic-extensions # Conflicts: # bin/node/runtime/src/lib.rs

* create parallel tasks extension * make type system happy * basic externalities * test for dynamic extensions * batching test * remove premature verify_batch * shnschnorrkel batch * alter test * shnschnorrkel test * executive batching * some docs * also multi/any signatgures * error propagation * styling * make verification extension optional * experimental ed25519 parallelization * some merge fallout * utilize task executor * merge fallout * utilize task executor more * another merge fallout * feature-gate sp-io * arrange toml * fix no-std * sr25519 batching and refactoring * add docs * fix name * add newline * fix block import test * long sr25519 test * blocking instead of parking * move everything in crypto * return batch_verify to check :) * use condvars * use multi-threaded executor for benches * don't call via host interface * try no spawning * add true * cleanup * straighten batching * remove signature check from this test (?) * remove now pointless test * remove another now useless test * fix warnings * Revert "remove another now useless test" This reverts commit bbdec24. * rethink the sp-io-part * Revert "remove now pointless test" This reverts commit 4d55306. * fix wording * add wording * add todo and fix * return check and fix * add logging in sp-io * Update primitives/io/src/batch_verifier.rs Co-Authored-By: cheme <[email protected]> * address review and use std condvar * account for early exit * address reivew * address review * more suggestions * add docs for batch verification * remove unused * more review suggestions * move to sp-runtime * add expects * remove blocks * use entry * Update primitives/io/src/batch_verifier.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update primitives/externalities/src/extensions.rs Co-Authored-By: Bastian Köcher <[email protected]> * update overlooked note * remove stupid return * Update primitives/io/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update primitives/io/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * fix wording * bump spec_version Co-authored-by: cheme <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> # Conflicts: # bin/node/runtime/src/lib.rs # frame/executive/Cargo.toml

primitives/io/src/batch_verifier.rs

NikVolf added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 22, 2020

paritytech deleted a comment from parity-benchapp bot Feb 23, 2020

kianenigma reviewed Feb 24, 2020

View reviewed changes

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

Demi-Marie reviewed Feb 25, 2020

View reviewed changes

NikVolf force-pushed the nv-dynamic-extensions branch 3 times, most recently from 78525b3 to 10f7356 Compare February 27, 2020 13:11

paritytech deleted a comment from parity-benchapp bot Feb 27, 2020

gnunicorn assigned bkchr and NikVolf and unassigned bkchr Apr 14, 2020

NikVolf added 2 commits April 14, 2020 18:18

move to sp-runtime

34499e4

add expects

d522248

kianenigma reviewed Apr 14, 2020

View reviewed changes

primitives/io/src/lib.rs Outdated Show resolved Hide resolved

NikVolf added 2 commits April 14, 2020 18:38

remove blocks

0d9d09f

use entry

6c4f6b5

bkchr approved these changes Apr 14, 2020

View reviewed changes

NikVolf and others added 3 commits April 15, 2020 09:25

Update primitives/io/src/batch_verifier.rs

48fb7d6

Co-Authored-By: Bastian Köcher <[email protected]>

Update primitives/externalities/src/extensions.rs

7f3ebae

Co-Authored-By: Bastian Köcher <[email protected]>

update overlooked note

766ec69

NikVolf force-pushed the nv-dynamic-extensions branch from 3b7b925 to 766ec69 Compare April 15, 2020 06:34

NikVolf added 2 commits April 15, 2020 09:37

Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions

49e9c67

# Conflicts: # frame/executive/Cargo.toml

remove stupid return

a3979f2

bkchr reviewed Apr 15, 2020

View reviewed changes

primitives/io/src/lib.rs Outdated Show resolved Hide resolved

primitives/io/src/lib.rs Outdated Show resolved Hide resolved

NikVolf and others added 4 commits April 15, 2020 13:14

Update primitives/io/src/lib.rs

b4cb58f

Co-Authored-By: Bastian Köcher <[email protected]>

Update primitives/io/src/lib.rs

234ec99

Co-Authored-By: Bastian Köcher <[email protected]>

fix wording

1db62b0

bump spec_version

f7097d6

tomusdrw removed their request for review April 15, 2020 10:35

Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions

e823c63

# Conflicts: # bin/node/runtime/src/lib.rs # frame/executive/Cargo.toml

NikVolf mentioned this pull request Apr 16, 2020

Companion PR for 5023 paritytech/polkadot#939

Merged

NikVolf added 3 commits April 16, 2020 12:37

Merge branch 'master' into nv-dynamic-extensions

056515e

Merge remote-tracking branch 'origin/master' into nv-dynamic-extensions

86d7efc

# Conflicts: # bin/node/runtime/src/lib.rs

Merge branch 'nv-dynamic-extensions' of github.com:paritytech/substra…

0e2b455

…te into nv-dynamic-extensions # Conflicts: # bin/node/runtime/src/lib.rs

bkchr merged commit d615043 into master Apr 16, 2020

bkchr deleted the nv-dynamic-extensions branch April 16, 2020 19:40

arkpar reviewed Apr 27, 2020

View reviewed changes

primitives/io/src/batch_verifier.rs Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch signature verification #5023

Batch signature verification #5023

NikVolf commented Feb 22, 2020 •

edited

Loading

NikVolf commented Feb 22, 2020

parity-benchapp bot commented Feb 22, 2020 •

edited

Loading

burdges commented Feb 23, 2020 •

edited

Loading

burdges commented Feb 23, 2020

parity-benchapp bot commented Feb 23, 2020 •

edited

Loading

NikVolf commented Feb 23, 2020 •

edited

Loading

bkchr commented Feb 25, 2020

NikVolf commented Feb 25, 2020

bkchr commented Feb 25, 2020

NikVolf commented Feb 25, 2020

bkchr commented Feb 25, 2020

NikVolf commented Feb 25, 2020

Demi-Marie commented Feb 25, 2020

Demi-Marie Feb 25, 2020

NikVolf Mar 25, 2020

kianenigma Apr 14, 2020

Demi-Marie Feb 25, 2020

NikVolf Mar 25, 2020

burdges commented Feb 28, 2020

bkchr commented Feb 28, 2020

NikVolf commented Feb 28, 2020

NikVolf commented Feb 28, 2020

parity-benchapp bot commented Feb 28, 2020 •

edited

Loading

NikVolf commented Feb 28, 2020

NikVolf commented Feb 28, 2020

parity-benchapp bot commented Feb 28, 2020 •

edited

Loading

bkchr left a comment

bkchr Apr 14, 2020

NikVolf Apr 15, 2020 •

edited

Loading

Batch signature verification #5023

Batch signature verification #5023

Conversation

NikVolf commented Feb 22, 2020 • edited Loading

NikVolf commented Feb 22, 2020

parity-benchapp bot commented Feb 22, 2020 • edited Loading

MASTER RESULT

BRANCH RESULT

burdges commented Feb 23, 2020 • edited Loading

burdges commented Feb 23, 2020

parity-benchapp bot commented Feb 23, 2020 • edited Loading

MASTER RESULT

BRANCH RESULT

NikVolf commented Feb 23, 2020 • edited Loading

bkchr commented Feb 25, 2020

NikVolf commented Feb 25, 2020

bkchr commented Feb 25, 2020

NikVolf commented Feb 25, 2020

bkchr commented Feb 25, 2020

NikVolf commented Feb 25, 2020

Demi-Marie commented Feb 25, 2020

Demi-Marie Feb 25, 2020

Choose a reason for hiding this comment

NikVolf Mar 25, 2020

Choose a reason for hiding this comment

kianenigma Apr 14, 2020

Choose a reason for hiding this comment

Demi-Marie Feb 25, 2020

Choose a reason for hiding this comment

NikVolf Mar 25, 2020

Choose a reason for hiding this comment

burdges commented Feb 28, 2020

bkchr commented Feb 28, 2020

NikVolf commented Feb 28, 2020

NikVolf commented Feb 28, 2020

parity-benchapp bot commented Feb 28, 2020 • edited Loading

NikVolf commented Feb 28, 2020

NikVolf commented Feb 28, 2020

parity-benchapp bot commented Feb 28, 2020 • edited Loading

bkchr left a comment

Choose a reason for hiding this comment

bkchr Apr 14, 2020

Choose a reason for hiding this comment

NikVolf Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

NikVolf commented Feb 22, 2020 •

edited

Loading

parity-benchapp bot commented Feb 22, 2020 •

edited

Loading

burdges commented Feb 23, 2020 •

edited

Loading

parity-benchapp bot commented Feb 23, 2020 •

edited

Loading

NikVolf commented Feb 23, 2020 •

edited

Loading

parity-benchapp bot commented Feb 28, 2020 •

edited

Loading

parity-benchapp bot commented Feb 28, 2020 •

edited

Loading

NikVolf Apr 15, 2020 •

edited

Loading