schnorr signature batch verification #2961
Conversation
chain/src/txhashset/txhashset.rs
Outdated
| @@ -1323,28 +1323,39 @@ impl<'a> Extension<'a> { | |||
|
|
|||
| let mut kern_count = 0; | |||
| let total_kernels = pmmr::n_leaves(self.kernel_pmmr.unpruned_size()); | |||
| let mut tx_kernels: Vec<TxKernel> = Vec::with_capacity(1_024); | |||
There was a problem hiding this comment.
I think we can simplify this by just using an iterator and chunks.
https://doc.rust-lang.org/std/primitive.slice.html#method.chunks
That way we don't need to hand code the chunking logic.
There was a problem hiding this comment.
sorry for late response, let me finish this chunks today.
There was a problem hiding this comment.
Oops, the chunks is useful for splitting an existing big vector, but here I need create and fill a new vector, and I don't want to create a huge vector and later to split it with chunks. So I think it's better to keep it like this.
Pls let me know if thought wrongly.
core/src/core/transaction.rs
Outdated
| let secp = secp.lock(); | ||
|
|
||
| for tx_kernel in tx_kernels { | ||
| if tx_kernel.is_coinbase() && tx_kernel.fee != 0 |
There was a problem hiding this comment.
Can we split the fee and lock_height verification out from this batch signature verification?
There are some pending changes in #2859 that rework this and these don't need to be batched up.
We can just do 2 passes over the kernels -
- verify fee and lock_height
- then batch verify the signatures
There was a problem hiding this comment.
Sure, I can split it out and will rework it.
BTW, when do you plan to complete #2859?
There was a problem hiding this comment.
will try the splitting today and request a new review on this.
|
Rebased to master. |
|
@antiochp Updated. Could you take a review again? thanks. |
|
Oops, just saw the So, let me rework it again to remove the |
|
@antiochp I have a related question on https://github.com/mimblewimble/grin/pull/2859/files#r318043781 . Could you please have a look? |
|
Testing this locally. |
| for x in &kernels { | ||
| x.verify()?; | ||
| } | ||
| TxKernel::batch_sig_verify(&kernels)?; |
There was a problem hiding this comment.
I'm guessing we are fine just batching them all up like this in the context of a block.
But is there a limit to how many we can batch?
There was a problem hiding this comment.
The limit should be big enough. At least 100,000 batching is OK:
running 1 test
Verifying aggsig batch of 100000
100000 signatures verified in 6408.601ms
test aggsig::tests::test_aggsig_batch ... ok
|
This looks significantly faster than 50% speedup to me. More like 5x - |
|
Shouldn't be more than 50%, unless we were doing something really inefficient before. Fingers crossed that we were 😂 |
* schnorr signature batch verification * Split the fee and lock_height verification out from the batch signature verification * Remove the new added fee_height_verify to comply with mimblewimble#2859 * fix: the last n could not be leaf?
To complete the schnorr signature batch verification feature.
The related PRs:
secp256k1-zkprepo as Merging schnorrsig module from Elements. secp256k1-zkp#47rust-secp256k1-zkprepo as Adding verify_batch wrapper around secp256k1_schnorrsig_verify_batch. rust-secp256k1-zkp#56.This should bring us over 50% performance improvement on schnorr signature verification: