Merge pull request #112 from getamis/feature/refactor-committed-seals…

…-to-two-dimensional-array

conesnsue/istanbul: refactor committed seals to two dimensional array

consensus/istanbul/backend.go

@@ -39,7 +39,7 @@ type Backend interface {
 
 	// Commit delivers an approved proposal to backend.
 	// The delivered proposal will be put into blockchain.
-	Commit(proposal Proposal, seals []byte) error
+	Commit(proposal Proposal, seals [][]byte) error
 
 	// NextRound is called when we want to trigger next Seal()
 	NextRound() error

consensus/istanbul/backend/backend.go

@@ -126,7 +126,7 @@ func (sb *backend) Broadcast(valSet istanbul.ValidatorSet, payload []byte) error
 }
 
 // Commit implements istanbul.Backend.Commit
-func (sb *backend) Commit(proposal istanbul.Proposal, seals []byte) error {
+func (sb *backend) Commit(proposal istanbul.Proposal, seals [][]byte) error {
 	// Check if the proposal is a valid block
 	block := &types.Block{}
 	block, ok := proposal.(*types.Block)

consensus/istanbul/backend/backend_test.go

@@ -115,16 +115,17 @@ func TestCheckValidatorSignature(t *testing.T) {
 func TestCommit(t *testing.T) {
 	backend, _, _ := newBackend()
 
+	commitCh := make(chan *types.Block)
 	// Case: it's a proposer, so the backend.commit will receive channel result from backend.Commit function
 	testCases := []struct {
 		expectedErr       error
-		expectedSignature []byte
+		expectedSignature [][]byte
 		expectedBlock     func() *types.Block
 	}{
 		{
 			// normal case
 			nil,
-			append([]byte{1}, bytes.Repeat([]byte{0x00}, types.IstanbulExtraSeal-1)...),
+			[][]byte{append([]byte{1}, bytes.Repeat([]byte{0x00}, types.IstanbulExtraSeal-1)...)},
 			func() *types.Block {
 				chain, engine := newBlockChain(1)
 				block := makeBlockWithoutSeal(chain, engine, chain.Genesis())
@@ -148,16 +149,10 @@ func TestCommit(t *testing.T) {
 	for _, test := range testCases {
 		expBlock := test.expectedBlock()
 		go func() {
-			for {
-				select {
-				case result := <-backend.commitCh:
-					if result.Hash() != expBlock.Hash() {
-						t.Errorf("hash mismatch: have %v, want %v", result.Hash(), expBlock.Hash())
-					}
-					return
-				case <-time.After(time.Second):
-					t.Error("unexpected timeout occurs")
-				}
+			select {
+			case result := <-backend.commitCh:
+				commitCh <- result
+				return
 			}
 		}()
 
@@ -167,6 +162,18 @@ func TestCommit(t *testing.T) {
 				t.Errorf("error mismatch: have %v, want %v", err, test.expectedErr)
 			}
 		}
+
+		if test.expectedErr == nil {
+			// to avoid race condition is occurred by goroutine
+			select {
+			case result := <-commitCh:
+				if result.Hash() != expBlock.Hash() {
+					t.Errorf("hash mismatch: have %v, want %v", result.Hash(), expBlock.Hash())
+				}
+			case <-time.After(10 * time.Second):
+				t.Fatal("timeout")
+			}
+		}
 	}
 }
 

consensus/istanbul/backend/engine.go

@@ -728,21 +728,24 @@ func writeSeal(h *types.Header, seal []byte) error {
 }
 
 // writeCommittedSeals writes the extra-data field of a block header with given committed seals.
-func writeCommittedSeals(h *types.Header, committedSeals []byte) error {
-	if len(committedSeals)%types.IstanbulExtraSeal != 0 {
+func writeCommittedSeals(h *types.Header, committedSeals [][]byte) error {
+	if len(committedSeals) == 0 {
 		return errInvalidCommittedSeals
 	}
 
+	for _, seal := range committedSeals {
+		if len(seal) != types.IstanbulExtraSeal {
+			return errInvalidCommittedSeals
+		}
+	}
+
 	istanbulExtra, err := types.ExtractIstanbulExtra(h)
 	if err != nil {
 		return err
 	}
 
-	istanbulExtra.CommittedSeal = make([][]byte, len(committedSeals)/types.IstanbulExtraSeal)
-	for i := 0; i < len(istanbulExtra.CommittedSeal); i++ {
-		istanbulExtra.CommittedSeal[i] = make([]byte, types.IstanbulExtraSeal)
-		copy(istanbulExtra.CommittedSeal[i][:], committedSeals[i*types.IstanbulExtraSeal:])
-	}
+	istanbulExtra.CommittedSeal = make([][]byte, len(committedSeals))
+	copy(istanbulExtra.CommittedSeal, committedSeals)
 
 	payload, err := rlp.EncodeToBytes(&istanbulExtra)
 	if err != nil {

consensus/istanbul/backend/engine_test.go

@@ -228,7 +228,7 @@ func TestSealCommittedOtherHash(t *testing.T) {
 			if !ok {
 				t.Errorf("unexpected event comes: %v", reflect.TypeOf(ev.Data))
 			}
-			engine.Commit(otherBlock, []byte{})
+			engine.Commit(otherBlock, [][]byte{})
 		}
 		eventSub.Unsubscribe()
 	}
@@ -565,7 +565,7 @@ func TestWriteCommittedSeals(t *testing.T) {
 	}
 
 	// normal case
-	err := writeCommittedSeals(h, expectedCommittedSeal)
+	err := writeCommittedSeals(h, [][]byte{expectedCommittedSeal})
 	if err != expectedErr {
 		t.Errorf("error mismatch: have %v, want %v", err, expectedErr)
 	}
@@ -581,7 +581,7 @@ func TestWriteCommittedSeals(t *testing.T) {
 
 	// invalid seal
 	unexpectedCommittedSeal := append(expectedCommittedSeal, make([]byte, 1)...)
-	err = writeCommittedSeals(h, unexpectedCommittedSeal)
+	err = writeCommittedSeals(h, [][]byte{unexpectedCommittedSeal})
 	if err != errInvalidCommittedSeals {
 		t.Errorf("error mismatch: have %v, want %v", err, errInvalidCommittedSeals)
 	}

consensus/istanbul/core/commit_test.go

@@ -17,6 +17,7 @@
 package core
 
 import (
+	"bytes"
 	"math/big"
 	"testing"
 
@@ -201,13 +202,10 @@ OUTER:
 
 		// check signatures large than 2F+1
 		signedCount := 0
-		signers := make([]common.Address, len(v0.committedSeals[0])/common.AddressLength)
-		for i := 0; i < len(signers); i++ {
-			copy(signers[i][:], v0.committedSeals[0][i*common.AddressLength:])
-		}
+		committedSeals := v0.committedMsgs[0].committedSeals
 		for _, validator := range r0.valSet.List() {
-			for _, signer := range signers {
-				if validator.Address() == signer {
+			for _, seal := range committedSeals {
+				if bytes.Compare(validator.Address().Bytes(), seal[:common.AddressLength]) == 0 {
 					signedCount++
 					break
 				}

consensus/istanbul/core/core.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/consensus/istanbul"
+	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/event"
 	"github.com/ethereum/go-ethereum/log"
 	metrics "github.com/ethereum/go-ethereum/metrics"
@@ -161,12 +162,13 @@ func (c *core) commit() {
 
 	proposal := c.current.Proposal()
 	if proposal != nil {
-		var signatures []byte
-		for _, v := range c.current.Commits.Values() {
-			signatures = append(signatures, v.CommittedSeal...)
+		committedSeals := make([][]byte, c.current.Commits.Size())
+		for i, v := range c.current.Commits.Values() {
+			committedSeals[i] = make([]byte, types.IstanbulExtraSeal)
+			copy(committedSeals[i][:], v.CommittedSeal[:])
 		}
 
-		if err := c.backend.Commit(proposal, signatures); err != nil {
+		if err := c.backend.Commit(proposal, committedSeals); err != nil {
 			c.sendNextRoundChange()
 			return
 		}

consensus/istanbul/core/core_test.go

@@ -69,14 +69,14 @@ func TestNewRequest(t *testing.T) {
 	}
 
 	for _, backend := range sys.backends {
-		if len(backend.commitMsgs) != 2 {
-			t.Errorf("the number of executed requests mismatch: have %v, want 2", len(backend.commitMsgs))
+		if len(backend.committedMsgs) != 2 {
+			t.Errorf("the number of executed requests mismatch: have %v, want 2", len(backend.committedMsgs))
 		}
-		if !reflect.DeepEqual(request1.Number(), backend.commitMsgs[0].Number()) {
-			t.Errorf("the number of requests mismatch: have %v, want %v", request1.Number(), backend.commitMsgs[0].Number())
+		if !reflect.DeepEqual(request1.Number(), backend.committedMsgs[0].commitProposal.Number()) {
+			t.Errorf("the number of requests mismatch: have %v, want %v", request1.Number(), backend.committedMsgs[0].commitProposal.Number())
 		}
-		if !reflect.DeepEqual(request2.Number(), backend.commitMsgs[1].Number()) {
-			t.Errorf("the number of requests mismatch: have %v, want %v", request2.Number(), backend.commitMsgs[1].Number())
+		if !reflect.DeepEqual(request2.Number(), backend.committedMsgs[1].commitProposal.Number()) {
+			t.Errorf("the number of requests mismatch: have %v, want %v", request2.Number(), backend.committedMsgs[1].commitProposal.Number())
 		}
 	}
 }

consensus/istanbul/core/testbackend_test.go

@@ -40,14 +40,18 @@ type testSystemBackend struct {
 	peers  istanbul.ValidatorSet
 	events *event.TypeMux
 
-	commitMsgs     []istanbul.Proposal
-	committedSeals [][]byte
-	sentMsgs       [][]byte // store the message when Send is called by core
+	committedMsgs []testCommittedMsgs
+	sentMsgs      [][]byte // store the message when Send is called by core
 
 	address common.Address
 	db      ethdb.Database
 }
 
+type testCommittedMsgs struct {
+	commitProposal istanbul.Proposal
+	committedSeals [][]byte
+}
+
 // ==============================================
 //
 // define the functions that needs to be provided for Istanbul.
@@ -88,10 +92,12 @@ func (self *testSystemBackend) NextRound() error {
 	return nil
 }
 
-func (self *testSystemBackend) Commit(proposal istanbul.Proposal, seals []byte) error {
+func (self *testSystemBackend) Commit(proposal istanbul.Proposal, seals [][]byte) error {
 	testLogger.Info("commit message", "address", self.Address())
-	self.commitMsgs = append(self.commitMsgs, proposal)
-	self.committedSeals = append(self.committedSeals, seals)
+	self.committedMsgs = append(self.committedMsgs, testCommittedMsgs{
+		commitProposal: proposal,
+		committedSeals: seals,
+	})
 
 	// fake new head events
 	go self.events.Post(istanbul.FinalCommittedEvent{