Merge pull request #71 from getamis/feature/log-comments-and-errors-refactoring

consensus/pbft: log, comments and other slight refactoring

Author: alanchchen

Makefile

@@ -11,11 +11,6 @@
 GOBIN = build/bin
 GO ?= latest
 
-pbft:
-	build/env.sh go run build/ci.go install ./consensus/pbft/example/simplenode
-	@echo "Done building."
-	@echo "Run \"$(GOBIN)/simplenode\" to launch simplenode."
-
 geth:
 	build/env.sh go run build/ci.go install ./cmd/geth
 	@echo "Done building."

consensus/pbft/backend.go

@@ -21,50 +21,51 @@ import (
 	"github.com/ethereum/go-ethereum/event"
 )
 
-// Backend provides callbacks for PBFT consensus core
+// Backend provides application specific functions for PBFT core
 type Backend interface {
 	// Address returns self address
 	Address() common.Address
 
-	// Validators returns validator set
+	// Validators returns the validator set
 	Validators() ValidatorSet
 
-	// EventMux is defined to handle request event and pbft message event
+	// EventMux returns the event mux in backend
 	EventMux() *event.TypeMux
 
-	// Send is to send pbft message to specific peer
+	// Send sends a message to specific peer
 	Send(payload []byte, target common.Address) error
 
-	// Broadcast is to send pbft message to all peers
+	// Broadcast sends a message to all peers
 	Broadcast(payload []byte) error
 
-	// Commit is to deliver a final result to write into blockchain
+	// Commit delivers a approved proposal to backend.
+	// The delivered proposal will be put into blockchain.
 	Commit(Proposal) error
 
 	// NextRound is called when we want to trigger next Seal()
 	NextRound() error
 
-	// Verify is to verify the proposal request
+	// Verify verifies the proposal.
 	Verify(Proposal) error
 
-	// Sign is to sign the data
+	// Sign signs input data with the backend's private key
 	Sign([]byte) ([]byte, error)
 
-	// CheckSignature is to verify the signature is signed from given peer
+	// CheckSignature verifies the signature by checking if it's signed by given peer
 	CheckSignature(data []byte, addr common.Address, sig []byte) error
 
 	// CheckValidatorSignature verifies if the data is signed by one of the validators
+	// If the verification succeeds, the signer's address is returned, otherwise
+	// an empty address and an error are returned.
 	CheckValidatorSignature(data []byte, sig []byte) (common.Address, error)
 
-	// FIXME: Hash, Encode, Decode and SetHandler are workaround functions for developing
-	Hash(b interface{}) common.Hash
-
 	Persistence
 }
 
+// Persistence provides persistence data storage for PBFT core
 type Persistence interface {
-	// Save an object into db
+	// Save an object into database
 	Save(key string, val interface{}) error
-	// Restore an object to val from db
+	// Restore an object to val from database
 	Restore(key string, val interface{}) error
 }

consensus/pbft/backends/simple/api.go

@@ -1,3 +1,19 @@
+// Copyright 2017 AMIS Technologies
+// This file is part of the go-ethereum library.
+//
+// The go-ethereum library is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Lesser General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// The go-ethereum library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
+
 package simple
 
 import (
@@ -19,8 +35,8 @@ func (api *API) Snapshot() {
 	state, snapshot := api.backend.core.Snapshot()
 	p := api.backend.valSet.GetProposer().Address()
 	log.Info("Snapshot", "sequence", snapshot.Sequence, "Round", snapshot.Round,
-		"state", state, "proposer", p.Hex(),
-		"hash", snapshot.Preprepare.Proposal.Hash().Hex(),
+		"state", state, "proposer", p,
+		"hash", snapshot.Preprepare.Proposal.Hash(),
 		"prepares", snapshot.Prepares, "commits", snapshot.Commits,
 		"checkpoint", snapshot.Checkpoints)
 }
@@ -29,8 +45,8 @@ func (api *API) Snapshot() {
 func (api *API) Backlog() {
 	backlog := api.backend.core.Backlog()
 	logs := make([]string, 0, len(backlog))
-	for address, q := range backlog {
-		logs = append(logs, fmt.Sprintf("{%v, %v}", address.Address().Hex(), q.Size()))
+	for validator, q := range backlog {
+		logs = append(logs, fmt.Sprintf("{%v, %v}", validator, q.Size()))
 	}
 	log.Info("Backlog", "logs", fmt.Sprintf("[%v]", strings.Join(logs, ", ")))
 }

consensus/pbft/backends/simple/backend.go

@@ -27,13 +27,12 @@ import (
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/crypto"
-	"github.com/ethereum/go-ethereum/crypto/sha3"
 	"github.com/ethereum/go-ethereum/ethdb"
 	"github.com/ethereum/go-ethereum/event"
 	"github.com/ethereum/go-ethereum/log"
-	"github.com/ethereum/go-ethereum/rlp"
 )
 
+// New creates an Ethereum backend for PBFT core engine.
 func New(config *pbft.Config, eventMux *event.TypeMux, privateKey *ecdsa.PrivateKey, db ethdb.Database) consensus.PBFT {
 	backend := &simpleBackend{
 		config:       config,
@@ -50,6 +49,7 @@ func New(config *pbft.Config, eventMux *event.TypeMux, privateKey *ecdsa.Private
 }
 
 // ----------------------------------------------------------------------------
+
 type simpleBackend struct {
 	config       *pbft.Config
 	peerSet      *peerSet
@@ -116,15 +116,15 @@ func (sb *simpleBackend) Broadcast(payload []byte) error {
 
 // Commit implements pbft.Backend.Commit
 func (sb *simpleBackend) Commit(proposal pbft.Proposal) error {
-	sb.logger.Info("Committed", "address", sb.Address().Hex(), "hash", proposal.Hash(), "number", proposal.Number().Uint64())
-	// step1: update validator set from extra data of block
-	// step2: insert chain
+	// Check if the proposal is a valid block
 	block := &types.Block{}
 	block, ok := proposal.(*types.Block)
 	if !ok {
-		sb.logger.Error("Failed to commit proposal since RequestContext cannot cast to *types.Block")
-		return errCastingRequest
+		sb.logger.Error("Invalid proposal, %v", proposal)
+		return errInvalidProposal
 	}
+
+	sb.logger.Info("Committed", "address", sb.Address(), "hash", proposal.Hash(), "number", proposal.Number().Uint64())
 	// - if the proposed and committed blocks are the same, send the proposed hash
 	//   to commit channel, which is being watched inside the engine.Seal() function.
 	// - otherwise, we try to insert the block.
@@ -143,33 +143,24 @@ func (sb *simpleBackend) Commit(proposal pbft.Proposal) error {
 // NextRound will broadcast ChainHeadEvent to trigger next seal()
 func (sb *simpleBackend) NextRound() error {
 	header := sb.chain.CurrentHeader()
-	sb.logger.Debug("NextRound", "address", sb.Address().Hex(), "current_hash", header.Hash(), "current_number", header.Number)
+	sb.logger.Debug("NextRound", "address", sb.Address(), "current_hash", header.Hash(), "current_number", header.Number)
 	go sb.eventMux.Post(core.ChainHeadEvent{})
 	return nil
 }
 
-// Hash implements pbft.Backend.Hash
-func (sb *simpleBackend) Hash(x interface{}) (h common.Hash) {
-	hw := sha3.NewKeccak256()
-	rlp.Encode(hw, x)
-	hw.Sum(h[:0])
-	return h
-}
-
 // EventMux implements pbft.Backend.EventMux
 func (sb *simpleBackend) EventMux() *event.TypeMux {
-	// not implemented
 	return sb.pbftEventMux
 }
 
 // Verify implements pbft.Backend.Verify
 func (sb *simpleBackend) Verify(proposal pbft.Proposal) error {
-	// decode the proposal to block
+	// Check if the proposal is a valid block
 	block := &types.Block{}
 	block, ok := proposal.(*types.Block)
 	if !ok {
-		sb.logger.Error("Failed to commit proposal since RequestContext cannot cast to *types.Block")
-		return errCastingRequest
+		sb.logger.Error("Invalid proposal, %v", proposal)
+		return errInvalidProposal
 	}
 	// verify the header of proposed block
 	return sb.VerifyHeader(sb.chain, block.Header(), false)
@@ -185,12 +176,12 @@ func (sb *simpleBackend) Sign(data []byte) ([]byte, error) {
 func (sb *simpleBackend) CheckSignature(data []byte, address common.Address, sig []byte) error {
 	signer, err := sb.getSignatureAddress(data, sig)
 	if err != nil {
-		log.Error("CheckSignature", "error", err)
+		log.Error("Failed to get signer address", "err", err)
 		return err
 	}
-	//Compare derived addresses
+	// Compare derived addresses
 	if signer != address {
-		return pbft.ErrInvalidSignature
+		return errInvalidSignature
 	}
 	return nil
 }
@@ -200,7 +191,7 @@ func (sb *simpleBackend) CheckValidatorSignature(data []byte, sig []byte) (commo
 	// 1. Get signature address
 	signer, err := sb.getSignatureAddress(data, sig)
 	if err != nil {
-		log.Error("CheckValidatorSignature", "error", err)
+		log.Error("Failed to get signer address", "err", err)
 		return common.Address{}, err
 	}
 
@@ -209,14 +200,14 @@ func (sb *simpleBackend) CheckValidatorSignature(data []byte, sig []byte) (commo
 		return val.Address(), nil
 	}
 
-	return common.Address{}, pbft.ErrNoMatchingValidator
+	return common.Address{}, pbft.ErrUnauthorizedAddress
 }
 
 // get the signer address from the signature
 func (sb *simpleBackend) getSignatureAddress(data []byte, sig []byte) (common.Address, error) {
-	//1. Keccak data
+	// 1. Keccak data
 	hashData := crypto.Keccak256([]byte(data))
-	//2. Recover public key
+	// 2. Recover public key
 	pubkey, err := crypto.SigToPub(hashData, sig)
 	if err != nil {
 		return common.Address{}, err

consensus/pbft/backends/simple/backend_test.go

@@ -59,7 +59,7 @@ func TestCheckSignature(t *testing.T) {
 	}
 	a = getInvalidAddress()
 	err = b.CheckSignature(data, a, sig)
-	if err != pbft.ErrInvalidSignature {
+	if err != errInvalidSignature {
 		t.Error("Should fail with ErrInvalidSignature")
 	}
 }
@@ -98,10 +98,10 @@ func TestCheckValidatorSignature(t *testing.T) {
 		t.Errorf("Unable to sign data")
 	}
 
-	// CheckValidatorSignature should return ErrNoMatchingValidator
+	// CheckValidatorSignature should return ErrUnauthorizedAddress
 	addr, err := b.CheckValidatorSignature(data, sig)
-	if err != pbft.ErrNoMatchingValidator {
-		t.Errorf("Expected error pbft.ErrNoMatchingValidator, but got: %v", err)
+	if err != pbft.ErrUnauthorizedAddress {
+		t.Errorf("Expected error pbft.ErrUnauthorizedAddress, but got: %v", err)
 	}
 	emptyAddr := common.Address{}
 	if addr != emptyAddr {
@@ -151,7 +151,7 @@ func (slice Keys) Len() int {
 }
 
 func (slice Keys) Less(i, j int) bool {
-	return strings.Compare(crypto.PubkeyToAddress(slice[i].PublicKey).Hex(), crypto.PubkeyToAddress(slice[j].PublicKey).Hex()) < 0
+	return strings.Compare(crypto.PubkeyToAddress(slice[i].PublicKey).String(), crypto.PubkeyToAddress(slice[j].PublicKey).String()) < 0
 }
 
 func (slice Keys) Swap(i, j int) {

consensus/pbft/backends/simple/engine.go

@@ -42,13 +42,16 @@ const (
 )
 
 var (
+	// errInvalidProposal is returned when a prposal is malformed.
+	errInvalidProposal = errors.New("invalid proposal")
+	// errInvalidSignature is returned when given signature is not signed by given
+	// address.
+	errInvalidSignature = errors.New("invalid signature")
 	// errUnknownBlock is returned when the list of signers is requested for a block
 	// that is not part of the local blockchain.
 	errUnknownBlock = errors.New("unknown block")
 	// errUnauthorized is returned if a header is signed by a non authorized entity.
 	errUnauthorized = errors.New("unauthorized")
-	// errNotInValidatorSet is returned if I'm not in validator set but try to start engine
-	errNotInValidatorSet = errors.New("not in validator set")
 	// errInvalidDifficulty is returned if the difficulty of a block is not 1
 	errInvalidDifficulty = errors.New("invalid difficulty")
 	// errInvalidPeer is returned when a message from invalid peer comes
@@ -63,8 +66,6 @@ var (
 	errInvalidUncleHash = errors.New("non empty uncle hash")
 	// errInconsistentValidatorSet is returned if the validator set is inconsistent
 	errInconsistentValidatorSet = errors.New("non empty uncle hash")
-	// errCastingRequest is returned if request cannot cast specific type
-	errCastingRequest = errors.New("failed to cast the request")
 	// errInvalidTimestamp is returned if the timestamp of a block is lower than the previous block's timestamp + the minimum block period.
 	errInvalidTimestamp = errors.New("invalid timestamp")
 )
@@ -365,10 +366,6 @@ func (sb *simpleBackend) AddPeer(peerID string, publicKey *ecdsa.PublicKey) erro
 	if _, val := sb.valSet.GetByAddress(peer.Address()); val != nil {
 		// add to peer set
 		sb.peerSet.Add(peer)
-		// post connection event to pbft core
-		go sb.pbftEventMux.Post(pbft.ConnectionEvent{
-			Address: val.Address(),
-		})
 	}
 	return nil
 }
@@ -388,7 +385,7 @@ func (sb *simpleBackend) HandleMsg(peerID string, data []byte) error {
 	}
 
 	if _, val := sb.valSet.GetByAddress(peer.Address()); val == nil {
-		sb.logger.Error("Not in validator set", "peerAddr", peer.Address().Hex())
+		sb.logger.Error("Not in validator set", "peerAddr", peer.Address())
 		return errInvalidPeer
 	}
 
@@ -402,7 +399,7 @@ func (sb *simpleBackend) HandleMsg(peerID string, data []byte) error {
 func (sb *simpleBackend) NewChainHead(block *types.Block) {
 	p, err := sb.Author(block.Header())
 	if err != nil {
-		sb.logger.Error("Failed to get block proposer", "error", err)
+		sb.logger.Error("Failed to get block proposer", "err", err)
 		return
 	}
 	go sb.pbftEventMux.Post(pbft.FinalCommittedEvent{
@@ -417,7 +414,7 @@ func (sb *simpleBackend) Start(chain consensus.ChainReader, inserter func(block
 		return err
 	}
 	if _, v := sb.valSet.GetByAddress(sb.address); v == nil {
-		return errNotInValidatorSet
+		return pbft.ErrUnauthorizedAddress
 	}
 	sb.chain = chain
 	sb.inserter = inserter

consensus/pbft/backends/simple/engine_test.go

@@ -229,8 +229,8 @@ func TestSealCommitted(t *testing.T) {
 	if err != nil {
 		t.Errorf("error should be nil, but got: %v", err)
 	}
-	if finalBlock.Hash().Hex() != expectedBlock.Hash().Hex() {
-		t.Errorf("block should be equal, got: %v, expected: %v", finalBlock.Hash().Hex(), expectedBlock.Hash().Hex())
+	if finalBlock.Hash() != expectedBlock.Hash() {
+		t.Errorf("block should be equal, got: %v, expected: %v", finalBlock.Hash(), expectedBlock.Hash())
 	}
 }
 

consensus/pbft/backends/simple/persistence.go

@@ -18,6 +18,7 @@ package simple
 
 import (
 	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/consensus/pbft"
 	"github.com/ethereum/go-ethereum/rlp"
 )
 
@@ -30,11 +31,11 @@ func (sb *simpleBackend) Save(key string, val interface{}) error {
 	if err != nil {
 		return err
 	}
-	return sb.db.Put(toDatabaseKey(sb.Hash, key), blob)
+	return sb.db.Put(toDatabaseKey(pbft.RLPHash, key), blob)
 }
 
 func (sb *simpleBackend) Restore(key string, val interface{}) error {
-	blob, err := sb.db.Get(toDatabaseKey(sb.Hash, key))
+	blob, err := sb.db.Get(toDatabaseKey(pbft.RLPHash, key))
 	if err != nil {
 		return err
 	}