View change improvement and unit tests

blockchain/skipchain/block_test.go

@@ -580,7 +580,7 @@ type fakeMino struct {
 }
 
 func (m fakeMino) GetAddress() mino.Address {
-	return fakeAddress{}
+	return fakeAddress{id: []byte{0xaa}}
 }
 
 func (m fakeMino) GetAddressFactory() mino.AddressFactory {

blockchain/skipchain/conode.go

@@ -128,26 +128,6 @@ func newConodes(ca cosi.CollectiveAuthority) Conodes {
 	return conodes
 }
 
-// Rotate takes the new leader and moves it to the beginning of the array while
-// moving the old one to the end.
-func (cc Conodes) Rotate(addr mino.Address) Conodes {
-	index := 0
-	for i, conode := range cc {
-		if conode.GetAddress().Equal(addr) {
-			index = i
-		}
-	}
-
-	if index == 0 {
-		return cc
-	}
-
-	newConodes := append(Conodes{cc[index]}, cc[1:index]...)
-	newConodes = append(newConodes, cc[index+1:]...)
-	newConodes = append(newConodes, cc[0])
-	return newConodes
-}
-
 // Take implements mino.Players. It returns a subset of the conodes.
 func (cc Conodes) Take(filters ...mino.FilterUpdater) mino.Players {
 	f := mino.ApplyFilters(filters)

blockchain/skipchain/mod.go

@@ -7,11 +7,13 @@ package skipchain
 
 import (
 	"context"
+	fmt "fmt"
 
 	"github.com/golang/protobuf/proto"
+	"github.com/rs/zerolog"
 	"go.dedis.ch/fabric"
 	"go.dedis.ch/fabric/blockchain"
-	"go.dedis.ch/fabric/blockchain/skipchain/viewchange"
+	"go.dedis.ch/fabric/blockchain/viewchange"
 	"go.dedis.ch/fabric/consensus"
 	"go.dedis.ch/fabric/consensus/cosipbft"
 	"go.dedis.ch/fabric/cosi"
@@ -26,7 +28,9 @@ import (
 // between the blocks.
 //
 // - implements blockchain.Blockchain
+// - implements fmt.Stringer
 type Skipchain struct {
+	logger     zerolog.Logger
 	mino       mino.Mino
 	cosi       cosi.CollectiveSigning
 	db         Database
@@ -41,6 +45,7 @@ func NewSkipchain(m mino.Mino, cosi cosi.CollectiveSigning) *Skipchain {
 	db := NewInMemoryDatabase()
 
 	return &Skipchain{
+		logger:    fabric.Logger,
 		mino:      m,
 		cosi:      cosi,
 		db:        db,
@@ -135,6 +140,12 @@ func (s *Skipchain) Watch(ctx context.Context) <-chan blockchain.Block {
 	return ch
 }
 
+// String implements fmt.Stringer. It returns a simple representation of the
+// skipchain instance to easily identify it.
+func (s *Skipchain) String() string {
+	return fmt.Sprintf("skipchain@%v", s.mino.GetAddress())
+}
+
 // skipchainActor provides the primitives of a blockchain actor.
 //
 // - implements blockchain.Actor
@@ -228,14 +239,13 @@ func (a skipchainActor) Store(data proto.Message, players mino.Players) error {
 	// If the leader has failed and this node has to take over, we use the
 	// inherant property of CoSiPBFT to prove that 2f participants want the view
 	// change.
-	err = a.viewchange.Wait(block)
+	rotation, err := a.viewchange.Wait(block)
 	if err == nil {
 		// If the node is not the current leader and a rotation is necessary, it
 		// will be done.
-		block.Conodes = block.Conodes.Rotate(a.mino.GetAddress())
+		block.Conodes = rotation.(Conodes)
 	} else {
-		fabric.Logger.Debug().Msgf("%v refusing view change: %v",
-			a.mino.GetAddress(), err)
+		a.logger.Debug().Msgf("%v refusing view change: %v", a, err)
 		// Not authorized to propose a block as the leader is moving
 		// forward so we drop the proposal. The upper layer is responsible to
 		// try again until the leader includes the data.

blockchain/skipchain/mod_test.go

@@ -1,11 +1,13 @@
 package skipchain
 
 import (
+	"bytes"
 	"context"
 	"testing"
 
 	proto "github.com/golang/protobuf/proto"
 	"github.com/golang/protobuf/ptypes/empty"
+	"github.com/rs/zerolog"
 	"github.com/stretchr/testify/require"
 	"go.dedis.ch/fabric/blockchain"
 	"go.dedis.ch/fabric/consensus"
@@ -184,19 +186,30 @@ func TestActor_InitChain(t *testing.T) {
 }
 
 func TestActor_Store(t *testing.T) {
+	buffer := new(bytes.Buffer)
+	cons := &fakeConsensusActor{}
 	actor := skipchainActor{
 		Skipchain: &Skipchain{
+			logger:     zerolog.New(buffer),
 			viewchange: fakeViewChange{},
 			mino:       fakeMino{},
 			db:         &fakeDatabase{},
 		},
-		consensus: fakeConsensusActor{},
+		consensus: cons,
 	}
 
-	conodes := Conodes{{addr: fakeAddress{}}}
+	conodes := Conodes{
+		{addr: fakeAddress{id: []byte{0xbb}}},
+		{addr: fakeAddress{id: []byte{0xaa}}},
+		{addr: fakeAddress{id: []byte{0xcc}}},
+	}
 
 	err := actor.Store(&empty.Empty{}, conodes)
 	require.NoError(t, err)
+	// Make sure the conodes rotate if the view change allows it.
+	require.NotNil(t, cons.prop)
+	prop := cons.prop.(SkipBlock)
+	require.Equal(t, prop.Conodes[0].GetAddress(), conodes[1].GetAddress())
 
 	err = actor.Store(&empty.Empty{}, fakePlayers{})
 	require.EqualError(t, err, "players must implement cosi.CollectiveAuthority")
@@ -206,7 +219,14 @@ func TestActor_Store(t *testing.T) {
 	require.EqualError(t, err, "couldn't read the latest block: oops")
 
 	actor.Skipchain.db = &fakeDatabase{}
-	actor.consensus = fakeConsensusActor{err: xerrors.New("oops")}
+	actor.Skipchain.viewchange = fakeViewChange{err: xerrors.New("oops")}
+	err = actor.Store(&empty.Empty{}, conodes)
+	// A view change is ignored.
+	require.NoError(t, err)
+	require.Contains(t, buffer.String(), "skipchain@aa refusing view change: oops")
+
+	actor.Skipchain.viewchange = fakeViewChange{}
+	actor.consensus = &fakeConsensusActor{err: xerrors.New("oops")}
 	err = actor.Store(&empty.Empty{}, conodes)
 	require.EqualError(t, err, "couldn't propose the block: oops")
 }
@@ -280,10 +300,12 @@ type fakePlayers struct {
 
 type fakeConsensusActor struct {
 	consensus.Actor
-	err error
+	err  error
+	prop consensus.Proposal
 }
 
-func (a fakeConsensusActor) Propose(consensus.Proposal, mino.Players) error {
+func (a *fakeConsensusActor) Propose(prop consensus.Proposal, pp mino.Players) error {
+	a.prop = prop
 	return a.err
 }
 
@@ -299,12 +321,17 @@ func (rand fakeRandGenerator) Read(buffer []byte) (int, error) {
 	return len(buffer), rand.err
 }
 
-type fakeViewChange struct{}
+type fakeViewChange struct {
+	err error
+}
 
-func (vc fakeViewChange) Wait(blockchain.Block) error {
-	return nil
+func (vc fakeViewChange) Wait(block blockchain.Block) (mino.Players, error) {
+	// Simulate a rotating view change.
+	players := block.GetPlayers().
+		Take(mino.RangeFilter(0, block.GetPlayers().Len()), mino.RotateFilter(1))
+	return players, vc.err
 }
 
 func (vc fakeViewChange) Verify(blockchain.Block) error {
-	return nil
+	return vc.err
 }

blockchain/skipchain/validator_test.go

@@ -49,6 +49,11 @@ func TestBlockValidator_Validate(t *testing.T) {
 			fmt.Sprintf("mismatch genesis hash '%v' != '%v'", Digest{}, block.GenesisID))
 
 		v.Skipchain.db = &fakeDatabase{genesisID: block.GenesisID}
+		v.Skipchain.viewchange = fakeViewChange{err: xerrors.New("oops")}
+		_, err = v.Validate(fakeAddress{}, packed)
+		require.EqualError(t, err, "viewchange refused the block: oops")
+
+		v.Skipchain.viewchange = fakeViewChange{}
 		v.validator = fakeValidator{err: xerrors.New("oops")}
 		_, err = v.Validate(fakeAddress{}, packed)
 		require.EqualError(t, err, "couldn't validate the payload: oops")

blockchain/skipchain/viewchange/constant.go → blockchain/viewchange/constant.go

@@ -7,7 +7,8 @@ import (
 )
 
 // ConstantViewChange is a naive implementation of the view change that will
-// simply keep the same leader all the time.
+// simply keep the same leader all the time and only allow a leader to propose a
+// block.
 //
 // - implements viewchange.ViewChange
 type ConstantViewChange struct {
@@ -24,24 +25,25 @@ func NewConstant(addr mino.Address, bc blockchain.Blockchain) ConstantViewChange
 }
 
 // Wait implements viewchange.ViewChange. It returns an error if the address
-// does not match the leader of the previous block.
-func (vc ConstantViewChange) Wait(block blockchain.Block) error {
+// does not match the leader of the previous block. The implementation of the
+// returned players is preserved.
+func (vc ConstantViewChange) Wait(block blockchain.Block) (mino.Players, error) {
 	latest, err := vc.bc.GetBlock()
 	if err != nil {
-		return xerrors.Errorf("couldn't read latest block: %v", err)
+		return nil, xerrors.Errorf("couldn't read latest block: %v", err)
 	}
 
 	if latest.GetPlayers().Len() == 0 {
-		return xerrors.New("players is empty")
+		return nil, xerrors.New("players is empty")
 	}
 
 	leader := latest.GetPlayers().AddressIterator().GetNext()
 
 	if !leader.Equal(vc.addr) {
-		return xerrors.Errorf("mismatching leader: %v != %v", leader, vc.addr)
+		return nil, xerrors.Errorf("mismatching leader: %v != %v", leader, vc.addr)
 	}
 
-	return nil
+	return block.GetPlayers(), nil
 }
 
 func getLeader(block blockchain.Block) mino.Address {

blockchain/viewchange/constant_test.go

@@ -0,0 +1,106 @@
+package viewchange
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.dedis.ch/fabric/blockchain"
+	"go.dedis.ch/fabric/mino"
+	"golang.org/x/xerrors"
+)
+
+func TestConstantViewChange_Wait(t *testing.T) {
+	vc := NewConstant(fakeAddress{id: "unknown"}, fakeBlockchain{len: 1, id: "unknown"})
+
+	players, err := vc.Wait(fakeBlock{})
+	require.NoError(t, err)
+	require.NotNil(t, players)
+
+	vc.bc = fakeBlockchain{err: xerrors.New("oops")}
+	_, err = vc.Wait(fakeBlock{})
+	require.EqualError(t, err, "couldn't read latest block: oops")
+
+	vc.bc = fakeBlockchain{len: 0}
+	_, err = vc.Wait(fakeBlock{})
+	require.EqualError(t, err, "players is empty")
+
+	vc.bc = fakeBlockchain{len: 1, id: "unknown"}
+	vc.addr = fakeAddress{id: "deadbeef"}
+	_, err = vc.Wait(fakeBlock{})
+	require.EqualError(t, err, "mismatching leader: unknown != deadbeef")
+}
+
+func TestConstantViewChange_Verify(t *testing.T) {
+	vc := NewConstant(fakeAddress{}, fakeBlockchain{})
+
+	err := vc.Verify(fakeBlock{})
+	require.NoError(t, err)
+
+	vc.bc = fakeBlockchain{err: xerrors.New("oops")}
+	err = vc.Verify(fakeBlock{})
+	require.EqualError(t, err, "couldn't read latest block: oops")
+
+	vc.bc = fakeBlockchain{id: "B"}
+	err = vc.Verify(fakeBlock{id: "A"})
+	require.EqualError(t, err, "mismatching leader: A != B")
+}
+
+//------------------------------------------------------------------------------
+// Utility functions
+
+type fakeAddress struct {
+	mino.Address
+	id string
+}
+
+func (a fakeAddress) Equal(other mino.Address) bool {
+	return a.id == other.(fakeAddress).id
+}
+
+func (a fakeAddress) String() string {
+	return a.id
+}
+
+type fakeIterator struct {
+	mino.AddressIterator
+	id string
+}
+
+func (i fakeIterator) GetNext() mino.Address {
+	return fakeAddress{id: i.id}
+}
+
+type fakePlayers struct {
+	mino.Players
+	len int
+	id  string
+}
+
+func (p fakePlayers) Len() int {
+	return p.len
+}
+
+func (p fakePlayers) AddressIterator() mino.AddressIterator {
+	return fakeIterator{id: p.id}
+}
+
+type fakeBlock struct {
+	blockchain.Block
+	len int
+	id  string
+}
+
+func (b fakeBlock) GetPlayers() mino.Players {
+	return fakePlayers{len: b.len, id: b.id}
+}
+
+type fakeBlockchain struct {
+	blockchain.Blockchain
+	err error
+	len int
+	id  string
+}
+
+func (bc fakeBlockchain) GetBlock() (blockchain.Block, error) {
+	return fakeBlock{len: bc.len, id: bc.id}, bc.err
+}

blockchain/viewchange/mod.go

@@ -0,0 +1,23 @@
+package viewchange
+
+import (
+	"go.dedis.ch/fabric/blockchain"
+	"go.dedis.ch/fabric/mino"
+)
+
+// ViewChange provides primitives to verify if a participant is allowed to
+// propose a block as the leader. It is also responsible for verifying the
+// integrity of the players of the chain.
+type ViewChange interface {
+	// Wait returns a non-nil error when the node is allowed to make the
+	// proposal. It will also return the authorized list of players that must be
+	// used so that the Verify function returns nil.
+	//
+	// Note: the implementation of the returned mino.Players interface must be
+	// preserved.
+	Wait(block blockchain.Block) (mino.Players, error)
+
+	// Verify makes sure that the players for the given are authorized and in
+	// the right order if necessary.
+	Verify(block blockchain.Block) error
+}