diff --git a/consensus/quorum/quorom_test.go b/consensus/quorum/quorom_test.go index 59ea163b90..ddfdb01e87 100644 --- a/consensus/quorum/quorom_test.go +++ b/consensus/quorum/quorom_test.go @@ -52,6 +52,38 @@ func TestPolicyStrings(t *testing.T) { } } +func TestUniformVerifierQuorumByMask(t *testing.T) { + verifier := &uniformVerifier{pubKeyCnt: 4} + publics := make([]bls.PublicKeyWrapper, verifier.pubKeyCnt) + for i := range publics { + publics[i].Object = bls.RandPrivateKey().GetPublicKey() + publics[i].Bytes.FromLibBLSPublicKey(publics[i].Object) + } + + tests := []struct { + name string + bitmap []byte + quorum bool + }{ + {name: "no signers", bitmap: []byte{0x00}, quorum: false}, + {name: "below threshold", bitmap: []byte{0x03}, quorum: false}, + {name: "at threshold", bitmap: []byte{0x07}, quorum: true}, + {name: "padding bits", bitmap: []byte{0xf0}, quorum: false}, + {name: "nil mask", bitmap: nil, quorum: false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var mask *bls.Mask + if test.bitmap != nil { + mask = bls.NewMask(publics) + require.NoError(t, mask.SetMask(test.bitmap)) + } + assert.Equal(t, test.quorum, verifier.IsQuorumAchievedByMask(mask)) + }) + } +} + func TestAddingQuoromParticipants(t *testing.T) { decider := NewDecider(SuperMajorityVote, shard.BeaconChainShardID) diff --git a/consensus/quorum/verifier.go b/consensus/quorum/verifier.go index aefc97c873..a47020101a 100644 --- a/consensus/quorum/verifier.go +++ b/consensus/quorum/verifier.go @@ -70,15 +70,18 @@ func newUniformVerifier(committee *shard.Committee) (*uniformVerifier, error) { }, nil } -// IsQuorumAchievedByMask returns whether the quorum is achieved with the provided mask, -// which is whether more than (2/3+1) nodes is included in mask. +// IsQuorumAchievedByMask returns whether the quorum is achieved with the provided mask. func (uv *uniformVerifier) IsQuorumAchievedByMask(mask *bls_cosi.Mask) bool { - got := int64(len(mask.Publics)) - exp := uv.thresholdKeyCount() - // Theoretically speaking, greater or equal will do the work. But current logic is more strict - // without equal, thus conform to current logic implemented. - // (engineImpl.VerifySeal, uniformVoteWeight.IsQuorumAchievedByMask) - return got > exp + if mask == nil { + return false + } + var signerCount int64 + for i := range mask.Publics { + if enabled, err := mask.IndexEnabled(i); err == nil && enabled { + signerCount++ + } + } + return signerCount >= uv.thresholdKeyCount() } func (uv *uniformVerifier) thresholdKeyCount() int64 { diff --git a/core/blockchain_impl.go b/core/blockchain_impl.go index f0e7e237ff..4757d3e5cc 100644 --- a/core/blockchain_impl.go +++ b/core/blockchain_impl.go @@ -2708,7 +2708,14 @@ func (bc *BlockChainImpl) CXMerkleProof(toShardID uint32, block *block.Header) ( func (bc *BlockChainImpl) WriteCXReceiptsProofSpent(db rawdb.DatabaseWriter, cxps []*types.CXReceiptsProof) error { for _, cxp := range cxps { - if cxp.Header != nil && bc.Config().IsCXMerkleProofReplayFixEpoch(cxp.Header.Epoch()) { + // Key the spent-marker off the signed Header, not the unauthenticated + // MerkleProof.ShardID/BlockNum: those fields are only bound to the + // Header by ValidateCXReceiptsProof from IsCXMerkleProofReplayFixEpoch + // onward, so a proof claiming an earlier epoch can carry a mutated + // MerkleProof while keeping a genuine Header/signature. Deriving the + // key from MerkleProof would let such a mutated copy of an already + //-applied receipt look unspent and be replayed for a fresh credit. + if cxp.Header != nil { if err := rawdb.WriteCXReceiptsProofSpentWithKey( db, cxp.Header.ShardID(), cxp.Header.Number().Uint64(), ); err != nil { @@ -2726,7 +2733,10 @@ func (bc *BlockChainImpl) WriteCXReceiptsProofSpent(db rawdb.DatabaseWriter, cxp func (bc *BlockChainImpl) IsSpent(cxp *types.CXReceiptsProof) bool { shardID := cxp.MerkleProof.ShardID blockNum := cxp.MerkleProof.BlockNum.Uint64() - if cxp.Header != nil && bc.Config().IsCXMerkleProofReplayFixEpoch(cxp.Header.Epoch()) { + // See WriteCXReceiptsProofSpent: always resolve the spent-marker key from + // the signed Header so the check can't be bypassed by mutating the + // unauthenticated MerkleProof fields on a genuine, previously-applied proof. + if cxp.Header != nil { shardID = cxp.Header.ShardID() blockNum = cxp.Header.Number().Uint64() } diff --git a/core/blockchain_impl_test.go b/core/blockchain_impl_test.go index ce3113f26d..1e23ee3bd1 100644 --- a/core/blockchain_impl_test.go +++ b/core/blockchain_impl_test.go @@ -11,6 +11,51 @@ import ( staking "github.com/harmony-one/harmony/staking/types" ) +// TestIsSpentIgnoresMutatedMerkleProofIdentity guards against replaying a +// genuine, already-applied CXReceiptsProof by mutating the unauthenticated +// MerkleProof.ShardID/BlockNum while keeping the same signed Header: the +// spent-marker must be keyed off the Header, which cannot be altered without +// invalidating the commit signature, not off MerkleProof fields that +// ValidateCXReceiptsProof only binds to the Header from +// IsCXMerkleProofReplayFixEpoch onward. +func TestIsSpentIgnoresMutatedMerkleProofIdentity(t *testing.T) { + key, _ := crypto.GenerateKey() + chain, _, header, database := getTestEnvironment(*key) + + header = header.With().ShardID(1).Number(big.NewInt(42)).Header() + + original := &types.CXReceiptsProof{ + Header: header, + MerkleProof: &types.CXMerkleProof{ + ShardID: 1, + BlockNum: big.NewInt(42), + }, + } + + batch := database.NewBatch() + if err := chain.WriteCXReceiptsProofSpent(batch, []*types.CXReceiptsProof{original}); err != nil { + t.Fatalf("WriteCXReceiptsProofSpent failed: %v", err) + } + if err := batch.Write(); err != nil { + t.Fatalf("batch.Write failed: %v", err) + } + + if !chain.IsSpent(original) { + t.Fatal("expected original proof to be marked spent") + } + + replay := &types.CXReceiptsProof{ + Header: header, // same genuine, signed header + MerkleProof: &types.CXMerkleProof{ + ShardID: 99, // mutated, unauthenticated + BlockNum: big.NewInt(9999), // mutated, unauthenticated + }, + } + if !chain.IsSpent(replay) { + t.Fatal("expected replay with mutated MerkleProof identity to be detected as already spent") + } +} + func TestPrepareStakingMetadata(t *testing.T) { key, _ := crypto.GenerateKey() chain, db, header, _ := getTestEnvironment(*key) diff --git a/internal/chain/engine_test.go b/internal/chain/engine_test.go index 170cce75fd..25a8a0113f 100644 --- a/internal/chain/engine_test.go +++ b/internal/chain/engine_test.go @@ -10,6 +10,7 @@ import ( "github.com/harmony-one/harmony/block" blockfactory "github.com/harmony-one/harmony/block/factory" "github.com/harmony-one/harmony/consensus/engine" + "github.com/harmony-one/harmony/consensus/quorum" consensus_sig "github.com/harmony-one/harmony/consensus/signature" "github.com/harmony-one/harmony/crypto/bls" bls_core "github.com/harmony-one/harmony/crypto/bls/core" @@ -498,6 +499,42 @@ func TestVerifiedSigCacheKeyIncludesShardID(t *testing.T) { } } +func TestVerifySignatureRejectsEmptyPreStakingQuorum(t *testing.T) { + chain := makeFakeBlockChain() + eng := NewEngine() + state := makeDefaultCommittee() + committee, err := state.FindCommitteeByID(shard.BeaconChainShardID) + if err != nil { + t.Fatal(err) + } + pubKeys, err := committee.BLSPublicKeys() + if err != nil { + t.Fatal(err) + } + verifier, err := quorum.NewVerifier(committee, big.NewInt(0), false) + if err != nil { + t.Fatal(err) + } + eng.epochCtxCache.Add(epochCtxKey{shardID: shard.BeaconChainShardID, epoch: 0}, epochCtx{ + qrVerifier: verifier, + pubKeys: pubKeys, + }) + + err = eng.verifySignature(chain, payloadArgs{ + blockHash: common.Hash{1}, + shardID: shard.BeaconChainShardID, + epoch: big.NewInt(0), + number: 2, + viewID: 1, + }, sigArgs{ + sig: bls.SerializedSignature{}, + bitmap: make([]byte, (len(pubKeys)+7)/8), + }) + if err == nil || err.Error() != "not enough signature collected" { + t.Fatalf("expected empty quorum rejection, got %v", err) + } +} + // setupTimestampValidationChain returns a chain wired with the timestamp // validation fork active, the parent header committed at parentTime, and a // helper to build child headers parented to it.