light-client-verifier: avoid checking signatures multiple times #1410
Conversation
|
@romac, could you give a quick review of this code? We’re continuing to hit performance limits and so need to continue trying to optimise the code. With this change I’m trying to make it so that a) no signature is verified twice and b) signatures are verified from one with most power so hopefully fewer signatures overall will need verification. One thing to keep in mind is that this is API breaking. |
Introduce VotingPowerCalculator::voting_power_in_sets method.
|
Thanks for the PR! These are some quite substantial changes to the logic so I unfortunately won't be able to properly review them this week, sorry about that. At first glance this looks alright to me though, I and don't mind the API changes. I'll take a closer look first thing next week. |
|
Thanks. One more thing that may be noteworthy is that I’ve filtered out non-commit votes. They didn’t contribute to voting power so I figured it’s fine to just ignore them. |
Co-authored-by: Romain Ruetschi <[email protected]>
Co-authored-by: Romain Ruetschi <[email protected]>
This reverts commit 92ff746.
Sorry, forgot about this question. At this moment I don’t have concrete numbers. We were testing various optimisations to get things working on Solana and that was one of them. At the moment I’ve no numbers for this change in isolation. I’ll try to get some numbers today but cannot promise. |
|
@mina86 Great work, thanks a lot for the care you've put into this PR and for bearing with me, much appreciated! |
in informalsystems#1410, alterations to the `PredicateVerifier<P, C, V>` to improve performance when verifying commits. specifically, a call to `verify_commit_against_trusted(untrusted, trusted, options)` will now make use of new internal interfaces that check validator signatures and signer overlap at the same time. this is a worthwhile performance improvement, but in the process, it made changes to the public interface of `PredicateVerifier<P, C, V>` that break some use cases. as i understand it, there is no strict need to remove the other public interface from the verifier. those that call `verify_commit_against_trusted` can still receive a performance boost, but calling `verify_commit` on just the untrusted state should still work after those changes. so, this commit restores `verify_commit(untrusted)`, and keeps `verify_commit_against_trusted(untrusted, trusted, options)` under its previous name.
in informalsystems#1410, alterations were made to the `PredicateVerifier<P, C, V>` to improve performance when verifying commits. specifically, a call to `verify_commit_against_trusted(untrusted, trusted, options)` will now make use of new internal interfaces that check validator signatures and signer overlap at the same time. this is a worthwhile performance improvement, but in the process, it made changes to the public interface of `PredicateVerifier<P, C, V>` that break some use cases. as i understand it, there is no strict need to remove the other public interface from the verifier. those that call `verify_commit_against_trusted` can still receive a performance boost, but calling `verify_commit` on just the untrusted state should still work after those changes. so, this commit restores `verify_commit(untrusted)`, and keeps `verify_commit_against_trusted(untrusted, trusted, options)` under its previous name.
in informalsystems#1410, alterations were made to the `PredicateVerifier<P, C, V>` to improve performance when verifying commits. specifically, a call to `verify_commit_against_trusted(untrusted, trusted, options)` will now make use of new internal interfaces that check validator signatures and signer overlap at the same time. this is a worthwhile performance improvement, but in the process, it made changes to the public interface of `PredicateVerifier<P, C, V>` that break some use cases. as i understand it, there is no strict need to remove the other public interface from the verifier. those that call `verify_commit_against_trusted` can still receive a performance boost, but calling `verify_commit` on just the untrusted state should still work after those changes. so, this commit restores `verify_commit(untrusted)`, and keeps `verify_commit_against_trusted(untrusted, trusted, options)` under its previous name.
Rework VerificationPredicates and VotingPowerCalculator by introducing
methods which check validators and signers overlap at once. The
motivation of this is to avoid checking the same signature multiple
times.
Consider a validator is in old and new set. Previously their
signature would be verified twice. Once by call to
has_sufficient_validators_overlapmethod and second time by call tohas_sufficient_signers_overlapmethod.With the new interface,
has_sufficient_validators_and_signers_overlapis called and it canbe implemented to remember which signatures have been verified.
As a side effect of those changes, signatures are now verified in the
order of validator’s power which may further reduce number of
signatures which need to be verified.
Furthermore, this commit also changes the signature verification such
that it completely ignores signatures which don’t contribute to voting
power, i.e. nil votes. This means that now an invalid nil signature
won’t cause an error.
.changelog/