POC: use wrapper function for batch verification#5695
POC: use wrapper function for batch verification#5695cce wants to merge 4 commits intoalgorand:masterfrom
Conversation
algonautshant
left a comment
algonautshant
left a comment
There was a problem hiding this comment.
The alternative suggestions in my comments are implemented here:
#5700
| publicKeys []byte // contains a slice of concatenated public keys. Each individual public key is 32 bytes. | ||
| signatures []byte // contains a slice of concatenated signatures. Each individual signature is 64 bytes. |
There was a problem hiding this comment.
Why is this change from proper indexed type to byte?
What is wrong with the existing type?
| messages: make([]Hashable, 0, hint), | ||
| publicKeys: make([]SignatureVerifier, 0, hint), | ||
| signatures: make([]Signature, 0, hint), | ||
| messageHashReps: make([]byte, 0), // XXX can we get a better hint? |
There was a problem hiding this comment.
The hint is 16 for streamed transactions, and cannot be improved without guessing.
In the case of block validation, it is precise, since we know the number of transactions.
As for the message size, for blocks it can be calculated, but not for the case of streamed transactions.
| b.messages = append(b.messages, message) | ||
| b.publicKeys = append(b.publicKeys, sigVerifier) | ||
| b.signatures = append(b.signatures, sig) | ||
| msgHashRep := HashRep(message) |
There was a problem hiding this comment.
This is the opposite of what we need to do.
Instead of calling this here, as we are accumulating elements into the batch, we should call HashRep later on, so that we don't perform unnecessary data copies.
| publicKeys := make([]SignatureVerifier, len(b.publicKeys), len(b.publicKeys)*2) | ||
| signatures := make([]Signature, len(b.signatures), len(b.signatures)*2) | ||
| copy(messages, b.messages) | ||
| messageHashReps := make([]byte, len(b.messageHashReps), len(b.messageHashReps)*2) |
There was a problem hiding this comment.
Potentially, many more copies are made here, which were not done in the past. This is a regression, and will also impact the block validation.
| b.publicKeys = append(b.publicKeys, sigVerifier) | ||
| b.signatures = append(b.signatures, sig) | ||
| msgHashRep := HashRep(message) | ||
| b.messageHashReps = append(b.messageHashReps, msgHashRep...) |
There was a problem hiding this comment.
This is the first guaranteed extra copy we are doing here. In the current code, this copy does not exist. the HashRep call returned buffer is not copied, but passed by reference to C.
Here, it is getting copied to b.messageHashReps.
| int *valid) { | ||
| int ret; | ||
| const unsigned char **messages2D, **publicKeys2D, **signatures2D; | ||
| messages2D = malloc(num * sizeof(unsigned char *)); |
There was a problem hiding this comment.
These "same" operations are done in go. What is special about doing this in a C wrapper function?
|
Integrated into #5700 |
2 participants
Summary
This POC updates the batchVerifier and its enqueue function to build up a 1-dimensional byte slice, rather than a slice of slices or a slice of arrays, for passing to the C batch verification code. This removes the need to build them on demand in batchVerificationImpl. A new C function acts as a shim between Go and C.
Test Plan
Needs to pass existing tests and perhaps add new ones.