Merge pull request #1529 from nspcc-dev/fix/nextconsensus

consensus: update NextConsensus only when committee is recalculated

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/gorilla/websocket v1.4.2
 	github.com/hashicorp/golang-lru v0.5.4
 	github.com/mr-tron/base58 v1.1.2
-	github.com/nspcc-dev/dbft v0.0.0-20200925163137-8f3b9ab3b720
+	github.com/nspcc-dev/dbft v0.0.0-20201109143252-cd27d76617ed
 	github.com/nspcc-dev/rfc6979 v0.2.0
 	github.com/pierrec/lz4 v2.5.2+incompatible
 	github.com/prometheus/client_golang v1.2.1

go.sum

@@ -167,6 +167,8 @@ github.com/nspcc-dev/dbft v0.0.0-20200219114139-199d286ed6c1 h1:yEx9WznS+rjE0jl0
 github.com/nspcc-dev/dbft v0.0.0-20200219114139-199d286ed6c1/go.mod h1:O0qtn62prQSqizzoagHmuuKoz8QMkU3SzBoKdEvm3aQ=
 github.com/nspcc-dev/dbft v0.0.0-20200925163137-8f3b9ab3b720 h1:e/lFQUIPnWErf2yiHjp2HyPhf0nyo3lp4hMm8IlQX1U=
 github.com/nspcc-dev/dbft v0.0.0-20200925163137-8f3b9ab3b720/go.mod h1:I5D0W3tu3epdt2RMCTxS//HDr4S+OHRqajouQTOAHI8=
+github.com/nspcc-dev/dbft v0.0.0-20201109143252-cd27d76617ed h1:T4qjutPMqjnYDQFyrbCew3lGeJt6MIbNyNn7gRx0o/g=
+github.com/nspcc-dev/dbft v0.0.0-20201109143252-cd27d76617ed/go.mod h1:I5D0W3tu3epdt2RMCTxS//HDr4S+OHRqajouQTOAHI8=
 github.com/nspcc-dev/neo-go v0.73.1-pre.0.20200303142215-f5a1b928ce09/go.mod h1:pPYwPZ2ks+uMnlRLUyXOpLieaDQSEaf4NM3zHVbRjmg=
 github.com/nspcc-dev/neofs-crypto v0.2.0 h1:ftN+59WqxSWz/RCgXYOfhmltOOqU+udsNQSvN6wkFck=
 github.com/nspcc-dev/neofs-crypto v0.2.0/go.mod h1:F/96fUzPM3wR+UGsPi3faVNmFlA9KAEAUQR7dMxZmNA=

pkg/consensus/consensus.go

@@ -14,6 +14,7 @@ import (
 	coreb "github.com/nspcc-dev/neo-go/pkg/core/block"
 	"github.com/nspcc-dev/neo-go/pkg/core/blockchainer"
 	"github.com/nspcc-dev/neo-go/pkg/core/mempool"
+	"github.com/nspcc-dev/neo-go/pkg/core/native"
 	"github.com/nspcc-dev/neo-go/pkg/core/transaction"
 	"github.com/nspcc-dev/neo-go/pkg/crypto/hash"
 	"github.com/nspcc-dev/neo-go/pkg/crypto/keys"
@@ -584,7 +585,13 @@ func (s *service) newBlockFromContext(ctx *dbft.Context) block.Block {
 	block.Block.Timestamp = ctx.Timestamp / nsInMs
 	block.Block.Index = ctx.BlockIndex
 
-	validators, err := s.Chain.GetValidators()
+	var validators keys.PublicKeys
+	var err error
+	if native.ShouldUpdateCommittee(ctx.BlockIndex, s.Chain) {
+		validators, err = s.Chain.GetValidators()
+	} else {
+		validators, err = s.Chain.GetNextBlockValidators()
+	}
 	if err != nil {
 		return nil
 	}

pkg/consensus/consensus_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/nspcc-dev/dbft/block"
 	"github.com/nspcc-dev/dbft/payload"
+	"github.com/nspcc-dev/dbft/timer"
 	"github.com/nspcc-dev/neo-go/pkg/config"
 	"github.com/nspcc-dev/neo-go/pkg/config/netmode"
 	"github.com/nspcc-dev/neo-go/pkg/core"
@@ -13,6 +14,7 @@ import (
 	"github.com/nspcc-dev/neo-go/pkg/core/native"
 	"github.com/nspcc-dev/neo-go/pkg/core/storage"
 	"github.com/nspcc-dev/neo-go/pkg/core/transaction"
+	"github.com/nspcc-dev/neo-go/pkg/crypto/hash"
 	"github.com/nspcc-dev/neo-go/pkg/crypto/keys"
 	"github.com/nspcc-dev/neo-go/pkg/internal/testchain"
 	"github.com/nspcc-dev/neo-go/pkg/io"
@@ -20,6 +22,7 @@ import (
 	"github.com/nspcc-dev/neo-go/pkg/util"
 	"github.com/nspcc-dev/neo-go/pkg/vm/emit"
 	"github.com/nspcc-dev/neo-go/pkg/vm/opcode"
+	"github.com/nspcc-dev/neo-go/pkg/wallet"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zaptest"
 )
@@ -39,6 +42,122 @@ func TestNewService(t *testing.T) {
 	srv.Chain.Close()
 }
 
+func initServiceNextConsensus(t *testing.T, newAcc *wallet.Account, offset uint32) (*service, *wallet.Account) {
+	acc, err := wallet.NewAccountFromWIF(testchain.WIF(testchain.IDToOrder(0)))
+	require.NoError(t, err)
+	priv := acc.PrivateKey()
+	require.NoError(t, acc.ConvertMultisig(1, keys.PublicKeys{priv.PublicKey()}))
+
+	bc := newSingleTestChain(t)
+	newPriv := newAcc.PrivateKey()
+
+	// Transfer funds to new validator.
+	w := io.NewBufBinWriter()
+	emit.AppCallWithOperationAndArgs(w.BinWriter, bc.GoverningTokenHash(), "transfer",
+		acc.Contract.ScriptHash().BytesBE(), newPriv.GetScriptHash().BytesBE(), int64(native.NEOTotalSupply))
+	emit.Opcodes(w.BinWriter, opcode.ASSERT)
+	emit.AppCallWithOperationAndArgs(w.BinWriter, bc.UtilityTokenHash(), "transfer",
+		acc.Contract.ScriptHash().BytesBE(), newPriv.GetScriptHash().BytesBE(), int64(1_000_000_000))
+	emit.Opcodes(w.BinWriter, opcode.ASSERT)
+	require.NoError(t, w.Err)
+
+	tx := transaction.New(netmode.UnitTestNet, w.Bytes(), 20_000_000)
+	tx.ValidUntilBlock = bc.BlockHeight() + 1
+	tx.NetworkFee = 10_000_000
+	tx.Signers = []transaction.Signer{{Scopes: transaction.Global, Account: acc.Contract.ScriptHash()}}
+	require.NoError(t, acc.SignTx(tx))
+	require.NoError(t, bc.PoolTx(tx))
+
+	srv := newTestServiceWithChain(t, bc)
+	srv.dbft.Start()
+
+	// Register new candidate.
+	w.Reset()
+	emit.AppCallWithOperationAndArgs(w.BinWriter, bc.GoverningTokenHash(), "registerCandidate", newPriv.PublicKey().Bytes())
+	require.NoError(t, w.Err)
+
+	tx = transaction.New(netmode.UnitTestNet, w.Bytes(), 20_000_000)
+	tx.ValidUntilBlock = bc.BlockHeight() + 1
+	tx.NetworkFee = 20_000_000
+	tx.Signers = []transaction.Signer{{Scopes: transaction.Global, Account: newPriv.GetScriptHash()}}
+	require.NoError(t, newAcc.SignTx(tx))
+
+	require.NoError(t, bc.PoolTx(tx))
+	srv.dbft.OnTimeout(timer.HV{Height: srv.dbft.Context.BlockIndex})
+
+	for i := srv.dbft.BlockIndex; !native.ShouldUpdateCommittee(i+offset, bc); i++ {
+		srv.dbft.OnTimeout(timer.HV{Height: srv.dbft.Context.BlockIndex})
+	}
+
+	// Vote for new candidate.
+	w.Reset()
+	emit.AppCallWithOperationAndArgs(w.BinWriter, bc.GoverningTokenHash(), "vote",
+		newPriv.GetScriptHash(), newPriv.PublicKey().Bytes())
+	emit.Opcodes(w.BinWriter, opcode.ASSERT)
+	require.NoError(t, w.Err)
+
+	tx = transaction.New(netmode.UnitTestNet, w.Bytes(), 20_000_000)
+	tx.ValidUntilBlock = bc.BlockHeight() + 1
+	tx.NetworkFee = 20_000_000
+	tx.Signers = []transaction.Signer{{Scopes: transaction.Global, Account: newPriv.GetScriptHash()}}
+	require.NoError(t, newAcc.SignTx(tx))
+
+	require.NoError(t, bc.PoolTx(tx))
+	srv.dbft.OnTimeout(timer.HV{Height: srv.dbft.BlockIndex})
+
+	return srv, acc
+}
+
+func TestService_NextConsensus(t *testing.T) {
+	newAcc, err := wallet.NewAccount()
+	require.NoError(t, err)
+	script, err := smartcontract.CreateMajorityMultiSigRedeemScript(keys.PublicKeys{newAcc.PrivateKey().PublicKey()})
+	require.NoError(t, err)
+
+	checkNextConsensus := func(t *testing.T, bc *core.Blockchain, height uint32, h util.Uint160) {
+		hdrHash := bc.GetHeaderHash(int(height))
+		hdr, err := bc.GetHeader(hdrHash)
+		require.NoError(t, err)
+		require.Equal(t, h, hdr.NextConsensus)
+	}
+
+	t.Run("vote 1 block before update", func(t *testing.T) {
+		srv, acc := initServiceNextConsensus(t, newAcc, 1)
+		bc := srv.Chain.(*core.Blockchain)
+		defer bc.Close()
+
+		height := bc.BlockHeight()
+		checkNextConsensus(t, bc, height, acc.Contract.ScriptHash())
+		// Reset     <- we are here, update NextConsensus
+		// OnPersist <- update committee
+		// Block     <-
+
+		srv.dbft.OnTimeout(timer.HV{Height: srv.dbft.BlockIndex})
+		checkNextConsensus(t, bc, height+1, hash.Hash160(script))
+	})
+
+	t.Run("vote 2 blocks before update", func(t *testing.T) {
+		srv, acc := initServiceNextConsensus(t, newAcc, 2)
+		bc := srv.Chain.(*core.Blockchain)
+		defer bc.Close()
+
+		height := bc.BlockHeight()
+		checkNextConsensus(t, bc, height, acc.Contract.ScriptHash())
+		// Reset     <- we are here
+		// OnPersist <- nothing to do
+		// Block     <-
+		//
+		// Reset     <- update next consensus
+		// OnPersist <- update committee
+		// Block     <-
+		srv.dbft.OnTimeout(timer.HV{Height: srv.dbft.BlockIndex})
+		checkNextConsensus(t, bc, height+1, acc.Contract.ScriptHash())
+
+		srv.dbft.OnTimeout(timer.HV{Height: srv.dbft.BlockIndex})
+		checkNextConsensus(t, bc, height+2, hash.Hash160(script))
+	})
+}
+
 func TestService_GetVerified(t *testing.T) {
 	srv := newTestService(t)
 	srv.dbft.Start()
@@ -289,11 +408,16 @@ func shouldNotReceive(t *testing.T, ch chan Payload) {
 }
 
 func newTestService(t *testing.T) *service {
+	return newTestServiceWithChain(t, newTestChain(t))
+}
+
+func newTestServiceWithChain(t *testing.T, bc *core.Blockchain) *service {
 	srv, err := NewService(Config{
 		Logger:       zaptest.NewLogger(t),
 		Broadcast:    func(*Payload) {},
-		Chain:     newTestChain(t),
+		Chain:        bc,
 		RequestTx:    func(...util.Uint256) {},
+		TimePerBlock: time.Duration(bc.GetConfig().SecondsPerBlock) * time.Second,
 		Wallet: &config.Wallet{
 			Path:     "./testdata/wallet1.json",
 			Password: "one",
@@ -309,6 +433,18 @@ func getTestValidator(i int) (*privateKey, *publicKey) {
 	return &privateKey{PrivateKey: key}, &publicKey{PublicKey: key.PublicKey()}
 }
 
+func newSingleTestChain(t *testing.T) *core.Blockchain {
+	configPath := "../../config/protocol.unit_testnet.single.yml"
+	cfg, err := config.LoadFile(configPath)
+	require.NoError(t, err, "could not load config")
+
+	chain, err := core.NewBlockchain(storage.NewMemoryStore(), cfg.ProtocolConfiguration, zaptest.NewLogger(t))
+	require.NoError(t, err, "could not create chain")
+
+	go chain.Run()
+	return chain
+}
+
 func newTestChain(t *testing.T) *core.Blockchain {
 	unitTestNetCfg, err := config.Load("../../config", netmode.UnitTestNet)
 	require.NoError(t, err)

pkg/consensus/testdata/wallet1.json

@@ -42,6 +42,23 @@
       },
       "lock": false,
       "isdefault": false
+    },
+    {
+      "address": "NVNvVRW5Q5naSx2k2iZm7xRgtRNGuZppAK",
+      "key": "6PYN7LvaWqBNw7Xb7a52LSbPnP91kyuzYi3HncGvQwQoYAY2W8DncTgpux",
+      "label": "",
+      "contract": {
+        "script": "EQwhArNiK/QBe9/jF8WK7V9MdT8ga324lgRvp9d0u8S/f43CEQtBE43vrw==",
+        "parameters": [
+          {
+            "name": "parameter0",
+            "type": "Signature"
+          }
+        ],
+        "deployed": false
+      },
+      "lock": false,
+      "isdefault": false
     }
   ],
   "scrypt": {

pkg/core/native/native_neo.go

@@ -266,7 +266,8 @@ func (n *NEO) updateCommittee(ic *interop.Context) error {
 	return ic.DAO.PutStorageItem(n.ContractID, prefixCommittee, si)
 }
 
-func shouldUpdateCommittee(h uint32, bc blockchainer.Blockchainer) bool {
+// ShouldUpdateCommittee returns true if committee is updated at block h.
+func ShouldUpdateCommittee(h uint32, bc blockchainer.Blockchainer) bool {
 	cfg := bc.GetConfig()
 	r := cfg.ValidatorsCount + len(cfg.StandbyCommittee)
 	return h%uint32(r) == 0
@@ -274,7 +275,7 @@ func shouldUpdateCommittee(h uint32, bc blockchainer.Blockchainer) bool {
 
 // OnPersist implements Contract interface.
 func (n *NEO) OnPersist(ic *interop.Context) error {
-	if shouldUpdateCommittee(ic.Block.Index, ic.Chain) {
+	if ShouldUpdateCommittee(ic.Block.Index, ic.Chain) {
 		if err := n.updateCommittee(ic); err != nil {
 			return err
 		}