From fde93156b1a8e75c0d297a636bfdeffea2cb1a1b Mon Sep 17 00:00:00 2001 From: Yawning Angel Date: Fri, 30 Apr 2021 10:04:14 +0000 Subject: [PATCH] crypto: Change the batch verification API to be not-stupid Having to redo quite a bit of computation in the event of a batch failure if the caller is actually interested in which signature failed, is rather sub-optimal. Change the batch verification interface to expose a one-shot batch verify + fallback call, so that sensible libraries can handle this case faster. This commit also leverages the failure information so that validation will only ever call one of `verifyCommitBatch` or `verifyCommitSingle`. --- crypto/batch/batch.go | 11 ++++ crypto/crypto.go | 8 ++- crypto/ed25519/bench_test.go | 2 +- crypto/ed25519/ed25519.go | 4 +- crypto/ed25519/ed25519_test.go | 3 +- crypto/sr25519/batch.go | 55 ++++++++++++++-- crypto/sr25519/bench_test.go | 2 +- crypto/sr25519/sr25519_test.go | 24 ++++++- types/validation.go | 114 ++++++++++++++++----------------- 9 files changed, 148 insertions(+), 75 deletions(-) diff --git a/crypto/batch/batch.go b/crypto/batch/batch.go index e53ceb319477..dbd11373c62b 100644 --- a/crypto/batch/batch.go +++ b/crypto/batch/batch.go @@ -20,3 +20,14 @@ func CreateBatchVerifier(pk crypto.PubKey) (crypto.BatchVerifier, bool) { // case where the key does not support batch verification return nil, false } + +// SupportsBatchVerifier checks if a key type implements the batch verifier +// interface. +func SupportsBatchVerifier(pk crypto.PubKey) bool { + switch pk.Type() { + case ed25519.KeyType, sr25519.KeyType: + return true + } + + return false +} diff --git a/crypto/crypto.go b/crypto/crypto.go index 5d68b578b759..8d44b82f50e8 100644 --- a/crypto/crypto.go +++ b/crypto/crypto.go @@ -46,7 +46,9 @@ type Symmetric interface { type BatchVerifier interface { // Add appends an entry into the BatchVerifier. Add(key PubKey, message, signature []byte) error - // Verify verifies all the entries in the BatchVerifier. - // If the verification fails it is unknown which entry failed and each entry will need to be verified individually. - Verify() bool + // Verify verifies all the entries in the BatchVerifier, and returns + // if every signature in the batch is valid, and a vector of bools + // indicating the verification status of each signature (in the order + // that signatures were added to the batch). + Verify() (bool, []bool) } diff --git a/crypto/ed25519/bench_test.go b/crypto/ed25519/bench_test.go index 8d9f58138fd1..056ebc966645 100644 --- a/crypto/ed25519/bench_test.go +++ b/crypto/ed25519/bench_test.go @@ -59,7 +59,7 @@ func BenchmarkVerifyBatch(b *testing.B) { require.NoError(b, err) } - if !v.Verify() { + if ok, _ := v.Verify(); !ok { b.Fatal("signature set failed batch verification") } } diff --git a/crypto/ed25519/ed25519.go b/crypto/ed25519/ed25519.go index ce9e65799b30..50f1aaac3d6f 100644 --- a/crypto/ed25519/ed25519.go +++ b/crypto/ed25519/ed25519.go @@ -215,6 +215,6 @@ func (b *BatchVerifier) Add(key crypto.PubKey, msg, signature []byte) error { return nil } -func (b *BatchVerifier) Verify() bool { - return b.BatchVerifier.VerifyBatchOnly(crypto.CReader()) +func (b *BatchVerifier) Verify() (bool, []bool) { + return b.BatchVerifier.Verify(crypto.CReader()) } diff --git a/crypto/ed25519/ed25519_test.go b/crypto/ed25519/ed25519_test.go index 6a139e9e23e6..63c781a3b668 100644 --- a/crypto/ed25519/ed25519_test.go +++ b/crypto/ed25519/ed25519_test.go @@ -50,5 +50,6 @@ func TestBatchSafe(t *testing.T) { require.NoError(t, err) } - require.True(t, v.Verify()) + ok, _ := v.Verify() + require.True(t, ok) } diff --git a/crypto/sr25519/batch.go b/crypto/sr25519/batch.go index 9f8665668664..d66df990aa36 100644 --- a/crypto/sr25519/batch.go +++ b/crypto/sr25519/batch.go @@ -8,19 +8,31 @@ import ( "github.com/tendermint/tendermint/crypto" ) -var _ crypto.BatchVerifier = BatchVerifier{} +var _ crypto.BatchVerifier = &BatchVerifier{} // BatchVerifier implements batch verification for sr25519. // https://github.com/ChainSafe/go-schnorrkel is used for batch verification type BatchVerifier struct { *schnorrkel.BatchVerifier + + // The go-schnorrkel API is terrible for performance if the chance + // that a batch fails is non-negligible, exact information about + // which signature failed is something that is desired, and the + // system is not resource constrained. + // + // The tendermint case meets all of the criteria, so emulate a + // one-shot with fallback API so that libraries that do provide + // sensible behavior aren't penalized. + pubKeys []crypto.PubKey + messages [][]byte + signatures [][]byte } func NewBatchVerifier() crypto.BatchVerifier { - return BatchVerifier{schnorrkel.NewBatchVerifier()} + return &BatchVerifier{schnorrkel.NewBatchVerifier(), nil, nil, nil} } -func (b BatchVerifier) Add(key crypto.PubKey, msg, sig []byte) error { +func (b *BatchVerifier) Add(key crypto.PubKey, msg, sig []byte) error { var sig64 [SignatureSize]byte copy(sig64[:], sig) signature := new(schnorrkel.Signature) @@ -34,9 +46,40 @@ func (b BatchVerifier) Add(key crypto.PubKey, msg, sig []byte) error { var pk [PubKeySize]byte copy(pk[:], key.Bytes()) - return b.BatchVerifier.Add(signingContext, signature, schnorrkel.NewPublicKey(pk)) + err = b.BatchVerifier.Add(signingContext, signature, schnorrkel.NewPublicKey(pk)) + if err == nil { + b.pubKeys = append(b.pubKeys, key) + b.messages = append(b.messages, msg) + b.signatures = append(b.signatures, sig) + } + + return err } -func (b BatchVerifier) Verify() bool { - return b.BatchVerifier.Verify() +func (b *BatchVerifier) Verify() (bool, []bool) { + // Explicitly mimic ed25519consensus/curve25519-voi behavior. + if len(b.pubKeys) == 0 { + return false, nil + } + + // Optimistically assume everything will succeed. + valid := make([]bool, len(b.pubKeys)) + for i := range valid { + valid[i] = true + } + + // Fast path, every signature may be valid. + if b.BatchVerifier.Verify() { + return true, valid + } + + // Fall-back to serial verification. + allValid := true + for i := range b.pubKeys { + pk, msg, sig := b.pubKeys[i], b.messages[i], b.signatures[i] + valid[i] = pk.VerifySignature(msg, sig) + allValid = allValid && valid[i] + } + + return allValid, valid } diff --git a/crypto/sr25519/bench_test.go b/crypto/sr25519/bench_test.go index 6c3b4e21d6e4..d87ef7c947b1 100644 --- a/crypto/sr25519/bench_test.go +++ b/crypto/sr25519/bench_test.go @@ -43,7 +43,7 @@ func BenchmarkVerifyBatch(b *testing.B) { } // NOTE: dividing by n so that metrics are per-signature for i := 0; i < b.N/n; i++ { - if !v.Verify() { + if ok, _ := v.Verify(); !ok { b.Fatal("signature set failed batch verification") } } diff --git a/crypto/sr25519/sr25519_test.go b/crypto/sr25519/sr25519_test.go index 60c7a29995d3..e292855f6b78 100644 --- a/crypto/sr25519/sr25519_test.go +++ b/crypto/sr25519/sr25519_test.go @@ -11,7 +11,6 @@ import ( ) func TestSignAndValidateSr25519(t *testing.T) { - privKey := sr25519.GenPrivKey() pubKey := privKey.PubKey() @@ -32,6 +31,7 @@ func TestSignAndValidateSr25519(t *testing.T) { func TestBatchSafe(t *testing.T) { v := sr25519.NewBatchVerifier() + vFail := sr25519.NewBatchVerifier() for i := 0; i <= 38; i++ { priv := sr25519.GenPrivKey() pub := priv.PubKey() @@ -48,9 +48,27 @@ func TestBatchSafe(t *testing.T) { err = v.Add(pub, msg, sig) require.NoError(t, err) + + switch i % 2 { + case 0: + err = vFail.Add(pub, msg, sig) + case 1: + msg[2] ^= byte(0x01) + err = vFail.Add(pub, msg, sig) + } + require.NoError(t, err) + } + + ok, valid := v.Verify() + require.True(t, ok, "failed batch verification") + for i, ok := range valid { + require.Truef(t, ok, "sig[%d] should be marked valid", i) } - if !v.Verify() { - t.Error("failed batch verification") + ok, valid = vFail.Verify() + require.False(t, ok, "succeeded batch verification (invalid batch)") + for i, ok := range valid { + expected := (i % 2) == 0 + require.Equalf(t, expected, ok, "sig[%d] should be %v", i, expected) } } diff --git a/types/validation.go b/types/validation.go index ce91711a6365..21d6f56082d8 100644 --- a/types/validation.go +++ b/types/validation.go @@ -9,6 +9,12 @@ import ( tmmath "github.com/tendermint/tendermint/libs/math" ) +const batchVerifyThreshold = 2 + +func shouldBatchVerify(vals *ValidatorSet, commit *Commit) bool { + return len(commit.Signatures) >= batchVerifyThreshold && batch.SupportsBatchVerifier(vals.GetProposer().PubKey) +} + // VerifyCommit verifies +2/3 of the set had signed the given commit. // // It checks all the signatures! While it's safe to exit as soon as we have @@ -18,7 +24,6 @@ import ( // with a bonus for including more than +2/3 of the signatures. func VerifyCommit(chainID string, vals *ValidatorSet, blockID BlockID, height int64, commit *Commit) error { - // run a basic validation of the arguments if err := verifyBasicValsAndCommit(vals, commit, height, blockID); err != nil { return err @@ -35,18 +40,14 @@ func VerifyCommit(chainID string, vals *ValidatorSet, blockID BlockID, count := func(c CommitSig) bool { return c.ForBlock() } // attempt to batch verify - cacheSignBytes, success, err := tryVerifyCommitBatch( - chainID, vals, commit, votingPowerNeeded, ignore, count, true, true) - if err != nil { - return err - } - if success { - return nil + if shouldBatchVerify(vals, commit) { + return verifyCommitBatch(chainID, vals, commit, + votingPowerNeeded, ignore, count, true, true) } // if verification failed or is not supported then fallback to single verification return verifyCommitSingle(chainID, vals, commit, votingPowerNeeded, - cacheSignBytes, ignore, count, true, true) + ignore, count, true, true) } // LIGHT CLIENT VERIFICATION METHODS @@ -57,7 +58,6 @@ func VerifyCommit(chainID string, vals *ValidatorSet, blockID BlockID, // signatures. func VerifyCommitLight(chainID string, vals *ValidatorSet, blockID BlockID, height int64, commit *Commit) error { - // run a basic validation of the arguments if err := verifyBasicValsAndCommit(vals, commit, height, blockID); err != nil { return err @@ -73,19 +73,14 @@ func VerifyCommitLight(chainID string, vals *ValidatorSet, blockID BlockID, count := func(c CommitSig) bool { return true } // attempt to batch verify - cacheSignBytes, success, err := tryVerifyCommitBatch( - chainID, vals, commit, votingPowerNeeded, ignore, count, false, true) - if err != nil { - return err - } - if success { - return nil + if shouldBatchVerify(vals, commit) { + return verifyCommitBatch(chainID, vals, commit, + votingPowerNeeded, ignore, count, false, true) } // if verification failed or is not supported then fallback to single verification return verifyCommitSingle(chainID, vals, commit, votingPowerNeeded, - cacheSignBytes, ignore, count, false, true) - + ignore, count, false, true) } // VerifyCommitLightTrusting verifies that trustLevel of the validator set signed @@ -124,18 +119,14 @@ func VerifyCommitLightTrusting(chainID string, vals *ValidatorSet, commit *Commi // attempt to batch verify commit. As the validator set doesn't necessarily // correspond with the validator set that signed the block we need to look // up by address rather than index. - cacheSignBytes, success, err := tryVerifyCommitBatch( - chainID, vals, commit, votingPowerNeeded, ignore, count, false, false) - if err != nil { - return err - } - if success { - return nil + if shouldBatchVerify(vals, commit) { + return verifyCommitBatch(chainID, vals, commit, + votingPowerNeeded, ignore, count, false, false) } // attempt with single verification return verifyCommitSingle(chainID, vals, commit, votingPowerNeeded, - cacheSignBytes, ignore, count, false, false) + ignore, count, false, false) } // ValidateHash returns an error if the hash is not empty, but its @@ -152,12 +143,13 @@ func ValidateHash(h []byte) error { // Batch verification -// tryVerifyCommitBatch attempts to batch verify. If it is not supported or -// verification fails it returns false. If there is an error in the signatures -// or the way that they are counted an error is returned. A cache of all the -// commits in byte form is returned in case it needs to be used again for single -// verification -func tryVerifyCommitBatch( +// verifyCommitBatch batch verifies commites. This routine is equivalent +// to verifyCommitSingle in behavior, just faster iff every signature in the +// batch is valid. +// +// Note: The caller is responsible for checking to see if this routine is +// usable via `shouldVerifyBatch(vals, commit)`. +func verifyCommitBatch( chainID string, vals *ValidatorSet, commit *Commit, @@ -166,21 +158,20 @@ func tryVerifyCommitBatch( countSig func(CommitSig) bool, countAllSignatures bool, lookUpByIndex bool, -) (map[string][]byte, bool, error) { +) error { var ( val *Validator valIdx int32 seenVals = make(map[int32]int, len(commit.Signatures)) + batchSigIdxs = make([]int, 0, len(commit.Signatures)) talliedVotingPower int64 = 0 - // we keep a cache of the signed bytes to make it quicker to verify - // individually if we need to - cacheSignBytes = make(map[string][]byte, len(commit.Signatures)) ) // attempt to create a batch verifier bv, ok := batch.CreateBatchVerifier(vals.GetProposer().PubKey) - // check if batch verification is supported - if !ok || len(commit.Signatures) < 2 { - return cacheSignBytes, false, nil + // re-check if batch verification is supported + if !ok || len(commit.Signatures) < batchVerifyThreshold { + // This should *NEVER* happen. + return fmt.Errorf("unsupported signature algorithm or insufficient signatures for batch verification") } for idx, commitSig := range commit.Signatures { @@ -206,20 +197,19 @@ func tryVerifyCommitBatch( // that the same validator doesn't commit twice if firstIndex, ok := seenVals[valIdx]; ok { secondIndex := idx - return cacheSignBytes, false, fmt.Errorf("double vote from %v (%d and %d)", val, firstIndex, secondIndex) + return fmt.Errorf("double vote from %v (%d and %d)", val, firstIndex, secondIndex) } seenVals[valIdx] = idx } // Validate signature. voteSignBytes := commit.VoteSignBytes(chainID, int32(idx)) - // cache the signBytes in case batch verification fails - cacheSignBytes[string(val.PubKey.Bytes())] = voteSignBytes // add the key, sig and message to the verifier if err := bv.Add(val.PubKey, voteSignBytes, commitSig.Signature); err != nil { - return cacheSignBytes, false, err + return err } + batchSigIdxs = append(batchSigIdxs, idx) // If this signature counts then add the voting power of the validator // to the tally @@ -237,18 +227,32 @@ func tryVerifyCommitBatch( // ensure that we have batched together enough signatures to exceed the // voting power needed else there is no need to even verify if got, needed := talliedVotingPower, votingPowerNeeded; got <= needed { - return cacheSignBytes, false, ErrNotEnoughVotingPowerSigned{Got: got, Needed: needed} + return ErrNotEnoughVotingPowerSigned{Got: got, Needed: needed} } - // attempt to verify the batch. If this fails, fall back to single - // verification - if bv.Verify() { + // attempt to verify the batch. + ok, validSigs := bv.Verify() + if ok { // success - return cacheSignBytes, true, nil + return nil + } + + // one or more of the signatures is invalid, find and return the first + // invalid signature. + for i, ok := range validSigs { + if !ok { + // go back from the batch index to the commit.Signatures index + idx := batchSigIdxs[i] + sig := commit.Signatures[idx] + return fmt.Errorf("wrong signature (#%d): %X", idx, sig) + } } - // verification failed - return cacheSignBytes, false, nil + // execution reaching here is a bug, and one of the following has + // happened: + // * non-zero tallied voting power, empty batch (impossible?) + // * bv.Verify() returned `false, []bool{true, ..., true}` (BUG) + return fmt.Errorf("BUG: batch verification failed with no invalid signatures") } // Single Verification @@ -263,7 +267,6 @@ func verifyCommitSingle( vals *ValidatorSet, commit *Commit, votingPowerNeeded int64, - cachedVals map[string][]byte, ignoreSig func(CommitSig) bool, countSig func(CommitSig) bool, countAllSignatures bool, @@ -303,12 +306,7 @@ func verifyCommitSingle( seenVals[valIdx] = idx } - // Check if we have the validator in the cache - if cachedVote, ok := cachedVals[string(val.PubKey.Bytes())]; !ok { - voteSignBytes = commit.VoteSignBytes(chainID, int32(idx)) - } else { - voteSignBytes = cachedVote - } + voteSignBytes = commit.VoteSignBytes(chainID, int32(idx)) if !val.PubKey.VerifySignature(voteSignBytes, commitSig.Signature) { return fmt.Errorf("wrong signature (#%d): %X", idx, commitSig.Signature)