This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Batch signature verification #5023
Batch signature verification #5023
Changes from 67 commits
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
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
If we make these 3 parameters
impl Iterator<&[u8]>
etc, we could prevent some allocations.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.
but schnorrkel expects slice and IntoIterator :(
https://docs.rs/schnorrkel/0.9.1/schnorrkel/fn.verify_batch.html
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.
would be cool to avoid adding merlin dependency, but maybe it is not doable without changing upstream schnorkell.
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.
indeed I can't see how it is possible
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.
maybe a comment on the signification of true parameter could be usefull (it is not very obvious from the schnorkel crate either).
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.
I am not sure why we need specific register extension methods here.
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.
Can you suggest the change that allows to avoid it?
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.
That would be using a
VerificationExt
the same way you got aTaskExecutorExt
, (init will be in StateMachine::new).The content of
VerificationExt
will be optional. So it can be activated/instantiated onstart_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 beNone
.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).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.
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 :) )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.
We should not have a separate
type_id
parameter. Instead, we should makeExtension
a subtrait ofAny
.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.
Can you provide an example?