Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VerifyCommitLight and VerifyCommitLightTrusting _never_ check all signatures #1750

Merged
merged 5 commits into from
Dec 11, 2023
Merged

VerifyCommitLight and VerifyCommitLightTrusting _never_ check all signatures #1750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d0b52c4
[e2e] Repro evidence bug: not checking all signatures
jmalicevic Nov 22, 2023
4180575
_Only_ callsites in the light client should be skipping verification …
sergio-mena Dec 4, 2023
0ca684b
Address PR comments
sergio-mena Dec 7, 2023
56f6371
Rename UT
sergio-mena Dec 7, 2023
0a6272b
Merge branch 'main' into sergio/1749-evidence_check_all_sigs
sergio-mena Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/blocksync/reactor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ FOR_LOOP:
// first.Hash() doesn't verify the tx contents, so MakePartSet() is
// currently necessary.
// TODO(sergio): Should we also validate against the extended commit?
err = state.Validators.VerifyCommitLight(
err = state.Validators.VerifyCommitLightAllSignatures(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Being pedantic) I actually don't think this is right. This line is using the received second block's commit, whose voteset isn't finalized.

Agreed that every other usage is correct (and important to fix)!

So it should suffice for correctness here that 2/3rds of signatures are valid.

However for reasons noted in #1830 , I think the new change is better, and we refactor the logic to have the next two calls not verify these signatures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValarDragon Not pedantic at all. Spot on, I'd say.

The reason why we are using VerifyCommitLightAllSignatures here is that we decided to leave the previous behavior (i.e., skipping signatures) only in the light client logic, for the rest we took a conservative approach (bear in mind that the raison d'être of this PR was that skipping signatures was problematic when dealing with evidence verification).

However, after careful consideration of your proposal, we realize that the usage of VerifyCommitLight* in blocksync can be seen as a light client-ish verification as well, in that it does not produce or verify any evidence of misbehavior. So we'll open a PR to change this shortly. Bear with us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1858

chainID, firstID, first.Height, second.LastCommit)

if err == nil {
Expand Down
10 changes: 5 additions & 5 deletions internal/evidence/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,19 +133,19 @@ func (evpool *Pool) Update(state sm.State, ev types.EvidenceList) {

// AddEvidence checks the evidence is valid and adds it to the pool.
func (evpool *Pool) AddEvidence(ev types.Evidence) error {
evpool.logger.Debug("Attempting to add evidence", "ev", ev)
evpool.logger.Info("Attempting to add evidence", "ev", ev)

// We have already verified this piece of evidence - no need to do it again
if evpool.isPending(ev) {
evpool.logger.Debug("Evidence already pending, ignoring this one", "ev", ev)
evpool.logger.Info("Evidence already pending, ignoring this one", "ev", ev)
return nil
}

// check that the evidence isn't already committed
if evpool.isCommitted(ev) {
// this can happen if the peer that sent us the evidence is behind so we shouldn't
// punish the peer.
evpool.logger.Debug("Evidence was already committed, ignoring this one", "ev", ev)
evpool.logger.Info("Evidence was already committed, ignoring this one", "ev", ev)
return nil
}

Expand Down Expand Up @@ -514,13 +514,13 @@ func (evpool *Pool) processConsensusBuffer(state sm.State) {

// check if we already have this evidence
if evpool.isPending(dve) {
evpool.logger.Debug("evidence already pending; ignoring", "evidence", dve)
evpool.logger.Info("evidence already pending; ignoring", "evidence", dve)
continue
}

// check that the evidence is not already committed on chain
if evpool.isCommitted(dve) {
evpool.logger.Debug("evidence already committed; ignoring", "evidence", dve)
evpool.logger.Info("evidence already committed; ignoring", "evidence", dve)
continue
}

Expand Down
5 changes: 3 additions & 2 deletions internal/evidence/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ func (evpool *Pool) verify(evidence types.Evidence) error {
// the conflicting header's commit
// - 2/3+ of the conflicting validator set correctly signed the conflicting block
// - the nodes trusted header at the same height as the conflicting header has a different hash
// - all signatures must be checked as this will be used as evidence
//
// CONTRACT: must run ValidateBasic() on the evidence before verifying
//
Expand All @@ -120,7 +121,7 @@ func VerifyLightClientAttack(
// In the case of lunatic attack there will be a different commonHeader height. Therefore the node perform a single
// verification jump between the common header and the conflicting one
if commonHeader.Height != e.ConflictingBlock.Height {
err := commonVals.VerifyCommitLightTrusting(trustedHeader.ChainID, e.ConflictingBlock.Commit, light.DefaultTrustLevel)
err := commonVals.VerifyCommitLightTrustingAllSignatures(trustedHeader.ChainID, e.ConflictingBlock.Commit, light.DefaultTrustLevel)
if err != nil {
return ErrConflictingBlock{fmt.Errorf("skipping verification of conflicting block failed: %w", err)}
}
Expand All @@ -132,7 +133,7 @@ func VerifyLightClientAttack(
}

// Verify that the 2/3+ commits from the conflicting validator set were for the conflicting header
if err := e.ConflictingBlock.ValidatorSet.VerifyCommitLight(trustedHeader.ChainID, e.ConflictingBlock.Commit.BlockID,
if err := e.ConflictingBlock.ValidatorSet.VerifyCommitLightAllSignatures(trustedHeader.ChainID, e.ConflictingBlock.Commit.BlockID,
e.ConflictingBlock.Height, e.ConflictingBlock.Commit); err != nil {
return ErrConflictingBlock{fmt.Errorf("invalid commit from conflicting block: %w", err)}
}
Expand Down
10 changes: 10 additions & 0 deletions test/e2e/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,16 @@ func (app *Application) FinalizeBlock(_ context.Context, req *abci.FinalizeBlock
txs[i] = &abci.ExecTxResult{Code: kvstore.CodeTypeOK}
}

for _, ev := range req.Misbehavior {
app.logger.Info("Misbehavior. Slashing validator",
"validator_address", ev.GetValidator().Address,
"type", ev.GetType(),
"height", ev.GetHeight(),
"time", ev.GetTime(),
"total_voting_power", ev.GetTotalVotingPower(),
)
}

valUpdates, err := app.validatorUpdates(uint64(req.Height))
if err != nil {
panic(err)
Expand Down
42 changes: 33 additions & 9 deletions test/e2e/runner/evidence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
// 1 in 4 evidence is light client evidence, the rest is duplicate vote evidence
const lightClientEvidenceRatio = 4

// InjectEvidence takes a running testnet and generates an amount of valid
// InjectEvidence takes a running testnet and generates an amount of valid/invalid
// evidence and broadcasts it to a random node through the rpc endpoint `/broadcast_evidence`.
// Evidence is random and can be a mixture of LightClientAttackEvidence and
// DuplicateVoteEvidence.
Expand Down Expand Up @@ -87,10 +87,12 @@ func InjectEvidence(ctx context.Context, r *rand.Rand, testnet *e2e.Testnet, amo
}

var ev types.Evidence
for i := 1; i <= amount; i++ {
for i := 0; i < amount; i++ {
validEv := true
if i%lightClientEvidenceRatio == 0 {
validEv = i%(lightClientEvidenceRatio*2) != 0 // Alternate valid and invalid evidence
ev, err = generateLightClientAttackEvidence(
ctx, privVals, evidenceHeight, valSet, testnet.Name, blockRes.Block.Time,
ctx, privVals, evidenceHeight, valSet, testnet.Name, blockRes.Block.Time, validEv,
)
} else {
var dve *types.DuplicateVoteEvidence
Expand All @@ -110,7 +112,15 @@ func InjectEvidence(ctx context.Context, r *rand.Rand, testnet *e2e.Testnet, amo
}

_, err := client.BroadcastEvidence(ctx, ev)
if err != nil {
if !validEv {
// The tests will count committed evidences later on,
// and only valid evidences will make it
amount++
}
if validEv != (err == nil) {
if err == nil {
return errors.New("submitting invalid evidence didn't return an error")
}
return err
}
}
Expand Down Expand Up @@ -155,6 +165,7 @@ func generateLightClientAttackEvidence(
vals *types.ValidatorSet,
chainID string,
evTime time.Time,
validEvidence bool,
) (*types.LightClientAttackEvidence, error) {
// forge a random header
forgedHeight := height + 2
Expand All @@ -164,7 +175,7 @@ func generateLightClientAttackEvidence(

// add a new bogus validator and remove an existing one to
// vary the validator set slightly
pv, conflictingVals, err := mutateValidatorSet(ctx, privVals, vals)
pv, conflictingVals, err := mutateValidatorSet(ctx, privVals, vals, !validEvidence)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the nop parameter in mutateValidatorSet is a no-op, then maybe it would make sense to keep mutateValidatorSet as is and do (?)

if validEvidence {
    pv, conflictingVals, err := mutateValidatorSet(ctx, privVals, vals)
} else {
   // reuse privVals and vals
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, what I'd need in the else is inside mutateValidatorSet. So I decided to modify mutateValidatorSet instead

if err != nil {
return nil, err
}
Expand All @@ -179,6 +190,11 @@ func generateLightClientAttackEvidence(
return nil, err
}

// malleate the last signature of the commit by adding one to its first byte
if !validEvidence {
sergio-mena marked this conversation as resolved.
Show resolved Hide resolved
commit.Signatures[len(commit.Signatures)-1].Signature[0]++
}

ev := &types.LightClientAttackEvidence{
ConflictingBlock: &types.LightBlock{
SignedHeader: &types.SignedHeader{
Expand Down Expand Up @@ -292,18 +308,26 @@ func makeBlockID(hash []byte, partSetSize uint32, partSetHash []byte) types.Bloc
}
}

func mutateValidatorSet(ctx context.Context, privVals []types.MockPV, vals *types.ValidatorSet,
func mutateValidatorSet(
ctx context.Context,
privVals []types.MockPV,
vals *types.ValidatorSet,
nop bool,
) ([]types.PrivValidator, *types.ValidatorSet, error) {
newVal, newPrivVal, err := test.Validator(ctx, 10)
if err != nil {
return nil, nil, err
}

var newVals *types.ValidatorSet
if vals.Size() > 2 {
newVals = types.NewValidatorSet(append(vals.Copy().Validators[:vals.Size()-1], newVal))
if nop {
newVals = types.NewValidatorSet(vals.Copy().Validators)
} else {
newVals = types.NewValidatorSet(append(vals.Copy().Validators, newVal))
if vals.Size() > 2 {
newVals = types.NewValidatorSet(append(vals.Copy().Validators[:vals.Size()-1], newVal))
} else {
newVals = types.NewValidatorSet(append(vals.Copy().Validators, newVal))
}
}

// we need to sort the priv validators with the same index as the validator set
Expand Down
80 changes: 71 additions & 9 deletions types/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,40 @@ func VerifyCommit(chainID string, vals *ValidatorSet, blockID BlockID,

// VerifyCommitLight verifies +2/3 of the set had signed the given commit.
//
// This method is primarily used by the light client and does not check all the
// This method is primarily used by the light client and does NOT check all the
// signatures.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "does not check all the signatures." because this now depends on countAllSignatures. Similarly for VerifyCommitLightTrusting below.

func VerifyCommitLight(chainID string, vals *ValidatorSet, blockID BlockID,
height int64, commit *Commit) error {
func VerifyCommitLight(
chainID string,
vals *ValidatorSet,
blockID BlockID,
height int64,
commit *Commit,
) error {
return verifyCommitLightInternal(chainID, vals, blockID, height, commit, false)
}

// VerifyCommitLightAllSignatures verifies +2/3 of the set had signed the given commit.
//
// This method is primarily used by the light client and DOES check all the
// signatures.
func VerifyCommitLightAllSignatures(
chainID string,
vals *ValidatorSet,
blockID BlockID,
height int64,
commit *Commit,
) error {
return verifyCommitLightInternal(chainID, vals, blockID, height, commit, true)
}

func verifyCommitLightInternal(
chainID string,
vals *ValidatorSet,
blockID BlockID,
height int64,
commit *Commit,
countAllSignatures bool,
sergio-mena marked this conversation as resolved.
Show resolved Hide resolved
) error {
// run a basic validation of the arguments
if err := verifyBasicValsAndCommit(vals, commit, height, blockID); err != nil {
return err
Expand All @@ -76,12 +106,12 @@ func VerifyCommitLight(chainID string, vals *ValidatorSet, blockID BlockID,
// attempt to batch verify
if shouldBatchVerify(vals, commit) {
return verifyCommitBatch(chainID, vals, commit,
votingPowerNeeded, ignore, count, false, true)
votingPowerNeeded, ignore, count, countAllSignatures, true)
}

// if verification failed or is not supported then fallback to single verification
return verifyCommitSingle(chainID, vals, commit, votingPowerNeeded,
ignore, count, false, true)
ignore, count, countAllSignatures, true)
}

// VerifyCommitLightTrusting verifies that trustLevel of the validator set signed
Expand All @@ -90,9 +120,41 @@ func VerifyCommitLight(chainID string, vals *ValidatorSet, blockID BlockID,
// NOTE the given validators do not necessarily correspond to the validator set
// for this commit, but there may be some intersection.
//
// This method is primarily used by the light client and does not check all the
// This method is primarily used by the light client and does NOT check all the
// signatures.
func VerifyCommitLightTrusting(
chainID string,
vals *ValidatorSet,
commit *Commit,
trustLevel cmtmath.Fraction,
) error {
return verifyCommitLightTrustingInternal(chainID, vals, commit, trustLevel, false)
}

// VerifyCommitLightTrustingAllSignatures verifies that trustLevel of the validator
// set signed this commit.
//
// NOTE the given validators do not necessarily correspond to the validator set
// for this commit, but there may be some intersection.
//
// This method is primarily used by the light client and DOES check all the
// signatures.
func VerifyCommitLightTrusting(chainID string, vals *ValidatorSet, commit *Commit, trustLevel cmtmath.Fraction) error {
func VerifyCommitLightTrustingAllSignatures(
chainID string,
vals *ValidatorSet,
commit *Commit,
trustLevel cmtmath.Fraction,
) error {
return verifyCommitLightTrustingInternal(chainID, vals, commit, trustLevel, true)
}

func verifyCommitLightTrustingInternal(
chainID string,
vals *ValidatorSet,
commit *Commit,
trustLevel cmtmath.Fraction,
countAllSignatures bool,
sergio-mena marked this conversation as resolved.
Show resolved Hide resolved
) error {
// sanity checks
if vals == nil {
return errors.New("nil validator set")
Expand Down Expand Up @@ -122,12 +184,12 @@ func VerifyCommitLightTrusting(chainID string, vals *ValidatorSet, commit *Commi
// up by address rather than index.
if shouldBatchVerify(vals, commit) {
return verifyCommitBatch(chainID, vals, commit,
votingPowerNeeded, ignore, count, false, false)
votingPowerNeeded, ignore, count, countAllSignatures, false)
}

// attempt with single verification
return verifyCommitSingle(chainID, vals, commit, votingPowerNeeded,
ignore, count, false, false)
ignore, count, countAllSignatures, false)
}

// ValidateHash returns an error if the hash is not empty, but its
Expand Down
Loading
Loading