From 70fe2444ab1c3aab77e7de977e3a191c70d1fe27 Mon Sep 17 00:00:00 2001
From: Ethan Frey <ethanfrey@users.noreply.github.com>
Date: Thu, 3 Aug 2017 19:00:03 +0200
Subject: [PATCH] Deduplicate validator changes in EndBlock and test this

---
 app/app.go          |  26 ++++++++-
 app/app_val_test.go | 137 ++++++++++++++++++++++++++++++++++++++++++++
 app/store.go        |  10 ++++
 3 files changed, 170 insertions(+), 3 deletions(-)
 create mode 100644 app/app_val_test.go

diff --git a/app/app.go b/app/app.go
index f0852b49bd..117b155e88 100644
--- a/app/app.go
+++ b/app/app.go
@@ -1,6 +1,7 @@
 package app
 
 import (
+	"bytes"
 	"fmt"
 	"strings"
 
@@ -110,9 +111,7 @@ func (app *Basecoin) DeliverTx(txBytes []byte) abci.Result {
 	if err != nil {
 		return errors.Result(err)
 	}
-	if len(res.Diff) > 0 {
-		app.pending = append(app.pending, res.Diff...)
-	}
+	app.addValChange(res.Diff)
 	return res.ToABCI()
 }
 
@@ -181,6 +180,27 @@ func (app *Basecoin) EndBlock(height uint64) (res abci.ResponseEndBlock) {
 	return
 }
 
+func (app *Basecoin) addValChange(diffs []*abci.Validator) {
+	for _, d := range diffs {
+		idx := findVal(d, app.pending)
+		if idx >= 0 {
+			app.pending[idx] = d
+		} else {
+			app.pending = append(app.pending, d)
+		}
+	}
+}
+
+// return index of list with validator of same PubKey, or -1 if no match
+func findVal(val *abci.Validator, list []*abci.Validator) int {
+	for i, v := range list {
+		if bytes.Equal(val.PubKey, v.PubKey) {
+			return i
+		}
+	}
+	return -1
+}
+
 //TODO move split key to tmlibs?
 
 // Splits the string at the first '/'.
diff --git a/app/app_val_test.go b/app/app_val_test.go
new file mode 100644
index 0000000000..cf2cdbfe0e
--- /dev/null
+++ b/app/app_val_test.go
@@ -0,0 +1,137 @@
+package app
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	abci "github.com/tendermint/abci/types"
+	wire "github.com/tendermint/go-wire"
+	cmn "github.com/tendermint/tmlibs/common"
+	"github.com/tendermint/tmlibs/log"
+
+	"github.com/tendermint/basecoin"
+	"github.com/tendermint/basecoin/errors"
+	"github.com/tendermint/basecoin/state"
+)
+
+//--------------------------------
+// Setup tx and handler for validation test cases
+
+const (
+	ValName       = "val"
+	TypeValChange = ValName + "/change"
+	ByteValChange = 0xfe
+)
+
+func init() {
+	basecoin.TxMapper.RegisterImplementation(ValChangeTx{}, TypeValChange, ByteValChange)
+}
+
+type ValSetHandler struct {
+	basecoin.NopCheck
+	basecoin.NopInitState
+	basecoin.NopInitValidate
+}
+
+var _ basecoin.Handler = ValSetHandler{}
+
+func (ValSetHandler) Name() string {
+	return ValName
+}
+
+func (ValSetHandler) DeliverTx(ctx basecoin.Context, store state.SimpleDB,
+	tx basecoin.Tx) (res basecoin.DeliverResult, err error) {
+	change, ok := tx.Unwrap().(ValChangeTx)
+	if !ok {
+		return res, errors.ErrUnknownTxType(tx)
+	}
+	res.Diff = change.Diff
+	return
+}
+
+type ValChangeTx struct {
+	Diff []*abci.Validator
+}
+
+func (v ValChangeTx) Wrap() basecoin.Tx {
+	return basecoin.Tx{v}
+}
+
+func (v ValChangeTx) ValidateBasic() error { return nil }
+
+//-----------------------------------
+// Test cases start here
+
+func power() uint64 {
+	// % can return negative numbers, so this ensures result is positive
+	return uint64(cmn.RandInt()%50 + 60)
+}
+
+func makeVal() *abci.Validator {
+	return &abci.Validator{
+		PubKey: cmn.RandBytes(10),
+		Power:  power(),
+	}
+}
+
+// newPower returns a copy of the validator with a different power
+func newPower(val *abci.Validator) *abci.Validator {
+	res := *val
+	res.Power = power()
+	if res.Power == val.Power {
+		panic("no no")
+	}
+	return &res
+}
+
+func TestEndBlock(t *testing.T) {
+	assert, require := assert.New(t), require.New(t)
+
+	logger := log.NewNopLogger()
+	store := MockStore()
+	handler := ValSetHandler{}
+	app := NewBasecoin(handler, store, logger)
+
+	val1 := makeVal()
+	val2 := makeVal()
+	val3 := makeVal()
+	val1a := newPower(val1)
+	val2a := newPower(val2)
+
+	cases := [...]struct {
+		changes  [][]*abci.Validator
+		expected []*abci.Validator
+	}{
+		// Nothing in, nothing out, no crash
+		0: {},
+		// One in, one out, no problem
+		1: {
+			changes:  [][]*abci.Validator{{val1}},
+			expected: []*abci.Validator{val1},
+		},
+		// Combine a few ones
+		2: {
+			changes:  [][]*abci.Validator{{val1}, {val2, val3}},
+			expected: []*abci.Validator{val1, val2, val3},
+		},
+		// Make sure changes all to one validators are squished into one diff
+		3: {
+			changes:  [][]*abci.Validator{{val1}, {val2, val1a}, {val2a, val3}},
+			expected: []*abci.Validator{val1a, val2a, val3},
+		},
+	}
+
+	for i, tc := range cases {
+		app.BeginBlock(nil, nil)
+		for _, c := range tc.changes {
+			tx := ValChangeTx{c}.Wrap()
+			txBytes := wire.BinaryBytes(tx)
+			res := app.DeliverTx(txBytes)
+			require.True(res.IsOK(), "%#v", res)
+		}
+		diff := app.EndBlock(app.height)
+		// TODO: don't care about order here...
+		assert.Equal(tc.expected, diff.Diffs, "%d", i)
+	}
+}
diff --git a/app/store.go b/app/store.go
index adf5128066..329b63516b 100644
--- a/app/store.go
+++ b/app/store.go
@@ -36,6 +36,16 @@ type ChainState struct {
 	Height uint64
 }
 
+// MockStore returns an in-memory store only intended for testing
+func MockStore() *Store {
+	res, err := NewStore("", 0, log.NewNopLogger())
+	if err != nil {
+		// should never happen, abort test if it does
+		panic(err)
+	}
+	return res
+}
+
 // NewStore initializes an in-memory IAVLTree, or attempts to load a persistant
 // tree from disk
 func NewStore(dbName string, cacheSize int, logger log.Logger) (*Store, error) {