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

Build block without checking signatures #4916

Merged
merged 19 commits into from Feb 18, 2020
Merged

Build block without checking signatures #4916

merged 19 commits into from Feb 18, 2020

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Feb 13, 2020

  • we now assume that transactions come from the source that checks signatures before building the block

  • this adds another function to block building runtime-api, called apply_trusted_extrinsic, which will skip signature checking.

  • Checkable trait augmented so that check could skip signature verification part.

speculative part of #4908

@NikVolf NikVolf added the A0-please_review Pull request needs code review. label Feb 13, 2020
@NikVolf NikVolf force-pushed the nv-signatures branch 2 times, most recently from 4cab749 to 8bce98e Compare February 13, 2020 13:00
Copy link
Contributor

@marcio-diaz marcio-diaz left a comment

Choose a reason for hiding this comment

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

Some nits but looks good to me.

primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
client/block-builder/src/lib.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Feb 13, 2020

I think in general it looks like a direction we can follow.

Have you done any benchmarks?

@gavofyork WDYT?

frame/executive/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks interesting!

frame/executive/src/lib.rs Outdated Show resolved Hide resolved
@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 13, 2020

@bkchr

Have you done any benchmarks?

Haven't been doing any benchmarks for block construction so far, only for block import.
But I think signature verification should not be a bottleneck there (since it is always native?), so do not expect much gain. When profiling last time, wasmtime was a bottleneck, and it's memory growing.

@bkchr
Copy link
Member

bkchr commented Feb 13, 2020

@bkchr

Have you done any benchmarks?

Haven't been doing any benchmarks for block construction so far, only for block import.
But I think signature verification should not be a bottleneck there (since it is always native?), so do not expect much gain. When profiling last time, wasmtime was a bottleneck, and it's memory growing.

If that isn't the bottleneck, why do we even try to optimize it? :D

(The issue should not be rechecked, as we landed some optimizations to wasmtime)

@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 13, 2020

If that isn't the bottleneck, why do we even try to optimize it? :D

Well, it can become bottleneck, once block construction will approach native speed :)
If you recall, I wanted to start with batch verification for import, where signature verification takes ~20% time, and there are benchmarks.

@bkchr
Copy link
Member

bkchr commented Feb 13, 2020

Yeah, I trust you with these results on block import. I just was curios if we see any improvement on block production :) But as we are current not bound by it, benchmark results will not be that helpful.

@NikVolf NikVolf added A1-onice and removed A0-please_review Pull request needs code review. labels Feb 13, 2020
@burdges
Copy link

burdges commented Feb 13, 2020

I'd expect signature verification code should actually run native, so the runtime WASM code would check the block normally, but substrate would batch the signature verification itself, and then call verify_batch before finishing.

If signatures come from untrusted sources, like in block production, then you can use batch verification, but it requires either a traitor tracing algorithm, or just rerunning the construction with individual verification. We love batch verification in block verification because someone always spends some limited resource like VRF wins producing that block.

It's possible for this batch verification to become a mild DoS vector for block production though: An adversary fills a block producer's transaction pool with ECDSA ecRecover calls, which run slow and cannot be batched, and a single invalid Ed25519 signature, which we batch until the end. Now the block producers builds their block checking each good slow ECDSA ecRecover along the way, but they queue the one bad Ed25519 until then end, at which point the block construction fails, and they must start over verifying each transaction along the way. This results in twice the computational work.

@bkchr
Copy link
Member

bkchr commented Feb 14, 2020

We verify each transaction before it goes into the transaction pool and can be used for block production. So, you can not slow down the block production. You can Ddos the node with invalid transaction, but it will probably just ban you.

@burdges
Copy link

burdges commented Feb 14, 2020

I see. If different runtimes handle their transaction pool differently then you need the runtime to make the choice about whether it trusts signature or not, and it cannot be decided purely by substrate, yes? As you're already discussing, any interface to these pre-verified signature should be missuse resistant so that parachain authors do not accidentally use it in the wrong place.

You could still have three or four modes for signature verification from substrate's side:

  • individual verification - our current approach but maybe still useful for bench marking
  • batch verified signatures - 40% faster than individual verification
  • all signatures pre-verified - requires care to avoid parachain authors never missuse this
  • cached signature verification - faster than batch verification, also more missuse resistant and slower than pre-verified, but requires nasty cache management.

We could maybe even disable pre-verified entirely on the substrate site for polkadot validators running parachain runtimes?

@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 14, 2020

I see. If different runtimes handle their transaction pool differently then you need the runtime to make the choice about whether it trusts signature or not, and it cannot be decided purely by substrate, yes?

Currently block proposing uses only transaction pool as interface, and transaction pool is always checking signatures.

But it is true, in general, block proposer interface to transaction source needs to be altered, simplified, and made more general, and then this current assumption will no longer hold. But by this time we will have batch verification - same as for block import.

cached signature verification - faster than batch verification, also more missuse resistant and slower than pre-verified, but requires nasty cache management.

I am in favour of this approach actually, for block proposing. Cache management seems rather trivial.

@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 18, 2020

@bkchr @tomusdrw addressed suggestions, thanks!

primitives/runtime/src/testing.rs Outdated Show resolved Hide resolved
primitives/runtime/src/testing.rs Outdated Show resolved Hide resolved
primitives/runtime/src/generic/mod.rs Show resolved Hide resolved
primitives/runtime/src/generic/mod.rs Outdated Show resolved Hide resolved
primitives/block-builder/src/lib.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
client/block-builder/src/lib.rs Outdated Show resolved Hide resolved
client/block-builder/src/lib.rs Outdated Show resolved Hide resolved
client/block-builder/src/lib.rs Show resolved Hide resolved
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Looks good! Ty :)

client/block-builder/src/lib.rs Outdated Show resolved Hide resolved
@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 18, 2020

@gnunicorn I am not sure if runtime impl/spec version should be bumped here

@gnunicorn
Copy link
Contributor

gnunicorn commented Feb 18, 2020

doesn't this mean the API of the node runtime is different now? Then spec_version should have a bump – and usually we bump impl_version along with it...

@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 18, 2020

What could be invoked before hasn't changed, but I will just bump spec_veriosn anyway... :)
impl_version should be zeroed afaik, not bumped along with it

@bkchr bkchr merged commit dc92587 into master Feb 18, 2020
@bkchr bkchr deleted the nv-signatures branch February 18, 2020 22:34
NikVolf added a commit that referenced this pull request Feb 19, 2020
* in executive

* in other places

* to UnsafeResult

* move doc comment

* apply suggestions

* allow validity mocking for TestXt

* add test

* augment checkable instead of another trait

* fix im online test

* blockbuilder dihotomy

* review suggestions

* update test

* Update client/block-builder/src/lib.rs

* updae spec_version

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Feb 20, 2020
* in executive

* in other places

* to UnsafeResult

* move doc comment

* apply suggestions

* allow validity mocking for TestXt

* add test

* augment checkable instead of another trait

* fix im online test

* blockbuilder dihotomy

* review suggestions

* update test

* Update client/block-builder/src/lib.rs

* updae spec_version

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
bkchr added a commit that referenced this pull request Mar 6, 2020
gnunicorn pushed a commit that referenced this pull request Mar 6, 2020
* Revert "Build block without checking signatures (#4916)"

This reverts commit dc92587.

* Some further clean ups
General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Mar 7, 2020
…aritytech#5159)

* Revert "Build block without checking signatures (paritytech#4916)"

This reverts commit dc92587.

* Some further clean ups
General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Mar 17, 2020
…aritytech#5159)

* Revert "Build block without checking signatures (paritytech#4916)"

This reverts commit dc92587.

* Some further clean ups
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants