Conversation
NikVolf
added
A0-please_review
NikVolf
added
the
A1-needs_burnin
|
Put this on-ice as we need to wait until all validators are updated properly. |
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
Can you please also change the signature batching function in primitives/io to panic, if there is no batching registered.
| } | ||
| } | ||
|
|
||
| impl Verify for sp_core::ecdsa::Signature { |
There was a problem hiding this comment.
ecdsa is a bit tricky, since it dominates the verification (takes much more time compared to transaction execution)
Node can be forced to do much more work before first ecdsa is resolved as invalid
There was a problem hiding this comment.
The other signatures probably also take more time to verify than a simple transfer. This feature is ridiculous if you draw the line here. Than we don't need async verification at all. If this would be a problem in the future, we could disable async signature verification when importing blocks on the tip of the chain. However, for syncing we want the best performance.
There was a problem hiding this comment.
To me, you can trick node to verify 1000 or so heavy signatures for free until first one is resolved and possibly completely stop some validators, not going to add it here until some proof that this is safe provided.
There was a problem hiding this comment.
I can also force you to verify 3000 sr25519 signatures. Where it the difference?
There was a problem hiding this comment.
Okay, you basically just want early exit of the scheduled verifications? So that we abort all, if one fails? Should not be that hard to implement, just make the future select on an exit signal.
And ECDSA is also not orders of magnitudes slower, on my machine it was factor 3 slower.
There was a problem hiding this comment.
We can support Ed25519 batch verification eventually I think, but not worth the time right now.
There was a problem hiding this comment.
Nevertheless, I like the idea of adding an early bail out for these verification tasks if one failed. This should prevent the problems you have described here.
There was a problem hiding this comment.
I added implementation for standalone ECDSA, but I think the way it works it can't be used in MultiSignature, since it uses AccountId32, which can store only blake2(public), but not public itself
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
…bstrate into nv-batch-verification-agai
How this supposed to work in block production? |
Good point :( |
|
closing because of inactivity. |
|
We want "batch verifiers" for basically all crypto, including both fancy future stuff like Groth16 and Plonk and ideally even our basic merkle tree proof checks, so that parachains do their heavy work off their main thread using algorithms with predictable runtimes. This helps enormously with preventing incorrect slashing in paritytech/polkadot#1656 |
4 participants
No description provided.