Address nkcr's comments

blockchain/skipchain/block.go

@@ -253,7 +253,7 @@ func (f blockFactory) FromVerifiable(src proto.Message) (blockchain.Block, error
 		return nil, xerrors.Errorf("couldn't get the chain factory: %v", err)
 	}
 
-	// Integrity of the chain is verifier during decoding.
+	// Integrity of the chain is verified during decoding.
 	chain, err := chainFactory.FromProto(in.GetChain())
 	if err != nil {
 		return nil, xerrors.Errorf("couldn't decode the chain: %v", err)

consensus/cosipbft/forward_link.go

@@ -87,15 +87,15 @@ func (fl forwardLink) Pack(enc encoding.ProtoMarshaler) (proto.Message, error) {
 }
 
 func (fl forwardLink) packChangeSet(enc encoding.ProtoMarshaler) (*ChangeSet, error) {
+	add, err := fl.packPlayers(fl.changeset.Add, enc)
+	if err != nil {
+		return nil, xerrors.Errorf("couldn't pack players: %v", err)
+	}
+
 	pb := &ChangeSet{
 		Leader: fl.changeset.Leader,
 		Remove: fl.changeset.Remove,
-	}
-
-	var err error
-	pb.Add, err = fl.packPlayers(fl.changeset.Add, enc)
-	if err != nil {
-		return nil, xerrors.Errorf("couldn't pack players: %v", err)
+		Add:    add,
 	}
 
 	return pb, nil
@@ -156,6 +156,8 @@ func (fl forwardLink) computeHash(h hash.Hash, enc encoding.ProtoMarshaler) ([]b
 		}
 	}
 
+	// This buffer will store all the encoded integers where each one is using 4
+	// bytes: leader index + removal indices.
 	buffer := make([]byte, 4+(4*len(fl.changeset.Remove)))
 	binary.LittleEndian.PutUint32(buffer, fl.changeset.Leader)
 
@@ -246,17 +248,17 @@ func (f *unsecureChainFactory) decodeForwardLink(pb proto.Message) (forwardLink,
 		return fl, xerrors.Errorf("unknown message type: %T", msg)
 	}
 
-	fl = forwardLink{
-		from: src.GetFrom(),
-		to:   src.GetTo(),
-	}
-
-	var err error
-	fl.changeset, err = f.decodeChangeSet(src.GetChangeSet())
+	changeset, err := f.decodeChangeSet(src.GetChangeSet())
 	if err != nil {
 		return fl, xerrors.Errorf("couldn't decode changeset: %v", err)
 	}
 
+	fl = forwardLink{
+		from:      src.GetFrom(),
+		to:        src.GetTo(),
+		changeset: changeset,
+	}
+
 	if src.GetPrepare() != nil {
 		sig, err := f.signatureFactory.FromProto(src.GetPrepare())
 		if err != nil {
@@ -286,15 +288,15 @@ func (f *unsecureChainFactory) decodeForwardLink(pb proto.Message) (forwardLink,
 }
 
 func (f *unsecureChainFactory) decodeChangeSet(in *ChangeSet) (viewchange.ChangeSet, error) {
+	add, err := f.decodeAdd(in.GetAdd())
+	if err != nil {
+		return viewchange.ChangeSet{}, xerrors.Errorf("couldn't decode add: %v", err)
+	}
+
 	changeset := viewchange.ChangeSet{
 		Leader: in.GetLeader(),
 		Remove: in.GetRemove(),
-	}
-
-	var err error
-	changeset.Add, err = f.decodeAdd(in.GetAdd())
-	if err != nil {
-		return changeset, xerrors.Errorf("couldn't decode add: %v", err)
+		Add:    add,
 	}
 
 	return changeset, nil
@@ -373,6 +375,8 @@ func newChainFactory(cosi cosi.CollectiveSigning, m mino.Mino,
 func (f *chainFactory) FromProto(pb proto.Message) (consensus.Chain, error) {
 	chain, err := f.unsecureChainFactory.FromProto(pb)
 	if err != nil {
+		// No wrapping to avoid redundancy in the error message. It will point
+		// anyway to a meaningful line.
 		return nil, err
 	}
 

consensus/cosipbft/mod.go

@@ -130,9 +130,9 @@ func (a pbftActor) Propose(p consensus.Proposal) error {
 	leader, ok := a.viewchange.Wait(p, authority)
 	if !ok {
 		fabric.Logger.Trace().Msg("proposal skipped by view change")
-		// 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.
+		// Not authorized to propose a block as the leader is moving forward so
+		// we drop the proposal. The upper layer is responsible for trying again
+		// until the leader includes the data.
 		return nil
 	}
 
@@ -186,7 +186,9 @@ func (a pbftActor) Propose(p consensus.Proposal) error {
 	return nil
 }
 
-func (a pbftActor) newPrepareRequest(prop consensus.Proposal, cs viewchange.ChangeSet) (Prepare, error) {
+func (a pbftActor) newPrepareRequest(prop consensus.Proposal,
+	cs viewchange.ChangeSet) (Prepare, error) {
+
 	req := Prepare{proposal: prop}
 
 	forwardLink := forwardLink{
@@ -195,12 +197,13 @@ func (a pbftActor) newPrepareRequest(prop consensus.Proposal, cs viewchange.Chan
 		changeset: cs,
 	}
 
-	var err error
-	req.digest, err = forwardLink.computeHash(a.hashFactory.New(), a.encoder)
+	digest, err := forwardLink.computeHash(a.hashFactory.New(), a.encoder)
 	if err != nil {
 		return req, xerrors.Errorf("couldn't compute hash: %v", err)
 	}
 
+	req.digest = digest
+
 	return req, nil
 }
 

consensus/viewchange/constant/mod.go

@@ -21,8 +21,8 @@ func NewViewChange(addr mino.Address) ViewChange {
 	}
 }
 
-// Wait implements viewchange.ViewChange. It true if the first player of the
-// authority is the current participant.
+// Wait implements viewchange.ViewChange. It returns true if the first player of
+// the authority is the current participant.
 func (vc ViewChange) Wait(p consensus.Proposal, a crypto.CollectiveAuthority) (uint32, bool) {
 	leader := a.AddressIterator().GetNext()
 

consensus/viewchange/mod.go

@@ -7,7 +7,7 @@ import (
 )
 
 // ViewChange provides primitives to verify if a participant is allowed to
-// propose a block as the leader. Some consensus needs a single node to propose
+// propose a block as the leader. Some consensus need a single node to propose
 // and the others as backups when it is failing. The index returned announces
 // who is allowed to be the leader.
 type ViewChange interface {
@@ -45,11 +45,12 @@ type EvolvableAuthority interface {
 // Governance is an interface to get information about the collective authority
 // of a proposal.
 type Governance interface {
-	// GetAuthority must return the authority that governs the block. It will be
-	// used to sign the forward link to the next proposal.
+	// GetAuthority must return the authority that governs the proposal at the
+	// given index. It will be used to sign the forward link to the next
+	// proposal.
 	GetAuthority(index uint64) (EvolvableAuthority, error)
 
 	// GetChangeSet must return the changes to the authority that will be
-	// applied for the next proposal.
+	// applied for the proposal following the given index.
 	GetChangeSet(index uint64) ChangeSet
 }

cosi/mod.go

@@ -43,8 +43,8 @@ type CollectiveSigning interface {
 	// GetSignatureFactory returns the signature factory.
 	GetSignatureFactory() crypto.SignatureFactory
 
-	// GetVerifier returns a verifier that can verify the signature created from
-	// a collective signing.
+	// GetVerifierFactory returns a factory that can create a verifier to check
+	// the validity of a signature.
 	GetVerifierFactory() crypto.VerifierFactory
 
 	// Listen starts the collective signing so that it will answer to requests.

docs/introduction.md

@@ -29,6 +29,9 @@ definitions of modular pieces that build a blockchain.
   algorithm that can be used to verify the integrity of some data. One example
   is the inventory page integrity to prove which instances are stored.
 
+- **governance** - Governance is a black box that provides to act on the
+  participants of a consensus.
+
 - **instance** - An instance is the smallest unit of storage in a ledger. It is
   identified by a unique key and stores a generic piece of data.
 

ledger/byzcoin/mod_test.go

@@ -53,7 +53,7 @@ func TestLedger_Basic(t *testing.T) {
 
 	for _, actor := range actors {
 		err := <-actor.HasStarted()
-		require.Nil(t, err)
+		require.NoError(t, err)
 	}
 
 	ctx, cancel := context.WithCancel(context.Background())

ledger/byzcoin/roster.go

@@ -17,15 +17,13 @@ type iterator struct {
 }
 
 func (i *iterator) HasNext() bool {
-	return i.index+1 < i.roster.Len()
+	return i.index < i.roster.Len()
 }
 
 func (i *iterator) GetNext() int {
-	if i.HasNext() {
-		i.index++
-		return i.index
-	}
-	return -1
+	res := i.index
+	i.index++
+	return res
 }
 
 // addressIterator is an iterator for a list of addresses.
@@ -37,9 +35,8 @@ type addressIterator struct {
 
 // GetNext implements mino.AddressIterator. It returns the next address.
 func (i *addressIterator) GetNext() mino.Address {
-	index := i.iterator.GetNext()
-	if index >= 0 {
-		return i.roster.addrs[index]
+	if i.iterator.HasNext() {
+		return i.roster.addrs[i.iterator.GetNext()]
 	}
 	return nil
 }
@@ -53,9 +50,8 @@ type publicKeyIterator struct {
 
 // GetNext implements crypto.PublicKeyIterator. It returns the next public key.
 func (i *publicKeyIterator) GetNext() crypto.PublicKey {
-	index := i.iterator.GetNext()
-	if index >= 0 {
-		return i.roster.pubkeys[index]
+	if i.iterator.HasNext() {
+		return i.roster.pubkeys[i.iterator.GetNext()]
 	}
 	return nil
 }
@@ -116,13 +112,13 @@ func (r roster) GetPublicKey(target mino.Address) (crypto.PublicKey, int) {
 // AddressIterator implements mino.Players. It returns an iterator of the
 // addresses of the roster in a deterministic order.
 func (r roster) AddressIterator() mino.AddressIterator {
-	return &addressIterator{iterator: &iterator{roster: &r, index: -1}}
+	return &addressIterator{iterator: &iterator{roster: &r}}
 }
 
 // PublicKeyIterator implements crypto.CollectiveAuthority. It returns an
 // iterator of the public keys of the roster in a deterministic order.
 func (r roster) PublicKeyIterator() crypto.PublicKeyIterator {
-	return &publicKeyIterator{iterator: &iterator{roster: &r, index: -1}}
+	return &publicKeyIterator{iterator: &iterator{roster: &r}}
 }
 
 // Pack implements encoding.Packable. It returns the protobuf message for the
@@ -170,11 +166,9 @@ func (f rosterFactory) New(authority crypto.CollectiveAuthority) roster {
 
 	addrIter := authority.AddressIterator()
 	pubkeyIter := authority.PublicKeyIterator()
-	index := 0
-	for addrIter.HasNext() && pubkeyIter.HasNext() {
-		roster.addrs[index] = addrIter.GetNext()
-		roster.pubkeys[index] = pubkeyIter.GetNext()
-		index++
+	for i := 0; addrIter.HasNext() && pubkeyIter.HasNext(); i++ {
+		roster.addrs[i] = addrIter.GetNext()
+		roster.pubkeys[i] = pubkeyIter.GetNext()
 	}
 
 	return roster
@@ -194,19 +188,23 @@ func (f rosterFactory) FromProto(in proto.Message) (viewchange.EvolvableAuthorit
 			len(pb.Addresses), len(pb.PublicKeys))
 	}
 
-	roster := roster{
-		addrs:   make([]mino.Address, len(pb.Addresses)),
-		pubkeys: make([]crypto.PublicKey, len(pb.PublicKeys)),
-	}
+	addrs := make([]mino.Address, len(pb.Addresses))
+	pubkeys := make([]crypto.PublicKey, len(pb.PublicKeys))
 
-	var err error
 	for i, addrpb := range pb.GetAddresses() {
-		roster.addrs[i] = f.addressFactory.FromText(addrpb)
+		addrs[i] = f.addressFactory.FromText(addrpb)
 
-		roster.pubkeys[i], err = f.pubkeyFactory.FromProto(pb.GetPublicKeys()[i])
+		pubkey, err := f.pubkeyFactory.FromProto(pb.GetPublicKeys()[i])
 		if err != nil {
 			return nil, xerrors.Errorf("couldn't decode public key: %v", err)
 		}
+
+		pubkeys[i] = pubkey
+	}
+
+	roster := roster{
+		addrs:   addrs,
+		pubkeys: pubkeys,
 	}
 
 	return roster, nil

ledger/byzcoin/roster_test.go

@@ -12,18 +12,17 @@ import (
 func TestIterator_HasNext(t *testing.T) {
 	iter := &iterator{
 		roster: &roster{addrs: make([]mino.Address, 3)},
-		index:  -1,
 	}
 
 	require.True(t, iter.HasNext())
 
-	iter.index = 0
-	require.True(t, iter.HasNext())
-
 	iter.index = 1
 	require.True(t, iter.HasNext())
 
 	iter.index = 2
+	require.True(t, iter.HasNext())
+
+	iter.index = 3
 	require.False(t, iter.HasNext())
 
 	iter.index = 10
@@ -33,23 +32,21 @@ func TestIterator_HasNext(t *testing.T) {
 func TestIterator_GetNext(t *testing.T) {
 	iter := &iterator{
 		roster: &roster{addrs: make([]mino.Address, 3)},
-		index:  -1,
 	}
 
 	for i := 0; i < 3; i++ {
 		c := iter.GetNext()
 		require.NotNil(t, c)
 	}
 
-	require.Equal(t, -1, iter.GetNext())
+	require.Equal(t, 3, iter.GetNext())
 }
 
 func TestAddressIterator_GetNext(t *testing.T) {
 	roster := rosterFactory{}.New(fake.NewAuthority(3, fake.NewSigner))
 	iter := &addressIterator{
 		iterator: &iterator{
 			roster: &roster,
-			index:  -1,
 		},
 	}
 
@@ -66,7 +63,6 @@ func TestPublicKeyIterator_GetNext(t *testing.T) {
 	iter := &publicKeyIterator{
 		iterator: &iterator{
 			roster: &roster,
-			index:  -1,
 		},
 	}