refactor validator (iotexproject#1970)

Author: CoderZhi

action/action.go

@@ -82,6 +82,11 @@ func Verify(sealed SealedEnvelope) error {
 	if sealed.SrcPubkey() == nil {
 		return errors.New("empty public key")
 	}
+	// Reject action with insufficient gas limit
+	intrinsicGas, err := sealed.IntrinsicGas()
+	if intrinsicGas > sealed.GasLimit() || err != nil {
+		return errors.Wrap(ErrInsufficientBalanceForGas, "insufficient gas")
+	}
 
 	hash := sealed.Envelope.Hash()
 	if sealed.SrcPubkey().Verify(hash[:], sealed.Signature()) {

action/action_test.go

@@ -8,35 +8,72 @@ package action
 
 import (
 	"encoding/hex"
-	"fmt"
 	"math/big"
+	"strings"
 	"testing"
 
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 
 	"github.com/iotexproject/iotex-core/test/identityset"
 )
 
-func TestActionProto(t *testing.T) {
+func TestActionProtoAndVerify(t *testing.T) {
 	require := require.New(t)
 	data, err := hex.DecodeString("")
 	require.NoError(err)
 	v, err := NewExecution("", 0, big.NewInt(10), uint64(10), big.NewInt(10), data)
 	require.NoError(err)
-	fmt.Println(v)
+	t.Run("no error", func(t *testing.T) {
+		bd := &EnvelopeBuilder{}
+		elp := bd.SetGasPrice(big.NewInt(10)).
+			SetGasLimit(uint64(100000)).
+			SetAction(v).Build()
 
-	bd := &EnvelopeBuilder{}
-	elp := bd.SetGasPrice(big.NewInt(10)).
-		SetGasLimit(uint64(100000)).
-		SetAction(v).Build()
+		selp, err := Sign(elp, identityset.PrivateKey(28))
+		require.NoError(err)
 
-	selp, err := Sign(elp, identityset.PrivateKey(28))
-	require.NoError(err)
+		require.NoError(Verify(selp))
+
+		nselp := &SealedEnvelope{}
+		require.NoError(nselp.LoadProto(selp.Proto()))
+
+		require.Equal(selp.Hash(), nselp.Hash())
+	})
+	t.Run("empty public key", func(t *testing.T) {
+		bd := &EnvelopeBuilder{}
+		elp := bd.SetGasPrice(big.NewInt(10)).
+			SetGasLimit(uint64(100000)).
+			SetAction(v).Build()
+
+		selp, err := Sign(elp, identityset.PrivateKey(28))
+		require.NoError(err)
+
+		selp.srcPubkey = nil
+
+		require.EqualError(Verify(selp), "empty public key")
+	})
+	t.Run("gas limit too low", func(t *testing.T) {
+		bd := &EnvelopeBuilder{}
+		elp := bd.SetGasPrice(big.NewInt(10)).
+			SetGasLimit(uint64(1000)).
+			SetAction(v).Build()
+
+		selp, err := Sign(elp, identityset.PrivateKey(28))
+		require.NoError(err)
 
-	require.NoError(Verify(selp))
+		require.Equal(ErrInsufficientBalanceForGas, errors.Cause(Verify(selp)))
+	})
+	t.Run("invalid signature", func(t *testing.T) {
+		bd := &EnvelopeBuilder{}
+		elp := bd.SetGasPrice(big.NewInt(10)).
+			SetGasLimit(uint64(100000)).
+			SetAction(v).Build()
 
-	nselp := &SealedEnvelope{}
-	require.NoError(nselp.LoadProto(selp.Proto()))
+		selp, err := Sign(elp, identityset.PrivateKey(28))
+		require.NoError(err)
+		selp.signature = []byte("invalid signature")
 
-	require.Equal(selp.Hash(), nselp.Hash())
+		require.True(strings.Contains(Verify(selp).Error(), "failed to verify action hash"))
+	})
 }

action/execution_test.go

@@ -27,7 +27,7 @@ func TestExecutionSignVerify(t *testing.T) {
 
 	bd := &EnvelopeBuilder{}
 	elp := bd.SetNonce(0).
-		SetGasLimit(uint64(10)).
+		SetGasLimit(uint64(100000)).
 		SetGasPrice(big.NewInt(10)).
 		SetAction(ex).Build()
 

action/protocol/execution/protocol_test.go

@@ -37,6 +37,7 @@ import (
 	"github.com/iotexproject/iotex-core/action/protocol/rewarding"
 	"github.com/iotexproject/iotex-core/action/protocol/rolldpos"
 	"github.com/iotexproject/iotex-core/blockchain"
+	"github.com/iotexproject/iotex-core/blockchain/block"
 	"github.com/iotexproject/iotex-core/blockchain/blockdao"
 	"github.com/iotexproject/iotex-core/blockindex"
 	"github.com/iotexproject/iotex-core/config"
@@ -320,12 +321,15 @@ func (sct *SmartContractTest) prepareBlockchain(
 		dao,
 		sf,
 		blockchain.RegistryOption(registry),
+		blockchain.BlockValidatorOption(block.NewValidator(
+			sf,
+			protocol.NewGenericValidator(sf, accountutil.AccountState),
+		)),
 	)
 	reward := rewarding.NewProtocol(nil)
 	r.NoError(reward.Register(registry))
 
 	r.NotNil(bc)
-	bc.Validator().AddActionEnvelopeValidators(protocol.NewGenericValidator(sf, accountutil.AccountState))
 	execution := NewProtocol(dao.GetBlockHash)
 	r.NoError(execution.Register(registry))
 	r.NoError(bc.Start(ctx))
@@ -498,10 +502,13 @@ func TestProtocol_Handle(t *testing.T) {
 			dao,
 			sf,
 			blockchain.RegistryOption(registry),
+			blockchain.BlockValidatorOption(block.NewValidator(
+				sf,
+				protocol.NewGenericValidator(sf, accountutil.AccountState),
+			)),
 		)
 		exeProtocol := NewProtocol(dao.GetBlockHash)
 		require.NoError(exeProtocol.Register(registry))
-		bc.Validator().AddActionEnvelopeValidators(protocol.NewGenericValidator(sf, accountutil.AccountState))
 		require.NoError(bc.Start(ctx))
 		require.NotNil(bc)
 		defer func() {

action/protocol/generic_validator.go

@@ -8,8 +8,8 @@ package protocol
 
 import (
 	"context"
-	"sync"
 
+	"github.com/iotexproject/iotex-address/address"
 	"github.com/pkg/errors"
 
 	"github.com/iotexproject/iotex-core/action"
@@ -21,7 +21,6 @@ type (
 	AccountState func(StateReader, string) (*state.Account, error)
 	// GenericValidator is the validator for generic action verification
 	GenericValidator struct {
-		mu           sync.RWMutex
 		accountState AccountState
 		sr           StateReader
 	}
@@ -36,26 +35,41 @@ func NewGenericValidator(sr StateReader, accountState AccountState) *GenericVali
 }
 
 // Validate validates a generic action
-func (v *GenericValidator) Validate(ctx context.Context, act action.SealedEnvelope) error {
-	actionCtx := MustGetActionCtx(ctx)
-	// Reject action with insufficient gas limit
-	intrinsicGas, err := act.IntrinsicGas()
-	if intrinsicGas > act.GasLimit() || err != nil {
-		return errors.Wrap(action.ErrInsufficientBalanceForGas, "insufficient gas")
-	}
+func (v *GenericValidator) Validate(ctx context.Context, selp action.SealedEnvelope) error {
 	// Verify action using action sender's public key
-	if err := action.Verify(act); err != nil {
+	if err := action.Verify(selp); err != nil {
 		return errors.Wrap(err, "failed to verify action signature")
 	}
+	caller, err := address.FromBytes(selp.SrcPubkey().Hash())
+	if err != nil {
+		return err
+	}
 	// Reject action if nonce is too low
-	confirmedState, err := v.accountState(v.sr, actionCtx.Caller.String())
+	confirmedState, err := v.accountState(v.sr, caller.String())
 	if err != nil {
-		return errors.Wrapf(err, "invalid state of account %s", actionCtx.Caller.String())
+		return errors.Wrapf(err, "invalid state of account %s", caller.String())
 	}
 
 	pendingNonce := confirmedState.Nonce + 1
-	if act.Nonce() > 0 && pendingNonce > act.Nonce() {
+	if selp.Nonce() > 0 && pendingNonce > selp.Nonce() {
 		return errors.Wrap(action.ErrNonce, "nonce is too low")
 	}
+	intrinsicGas, err := selp.IntrinsicGas()
+	if err != nil {
+		return err
+	}
+	ctx = WithActionCtx(ctx, ActionCtx{
+		Caller:       caller,
+		ActionHash:   selp.Hash(),
+		GasPrice:     selp.GasPrice(),
+		IntrinsicGas: intrinsicGas,
+		Nonce:        selp.Nonce(),
+	})
+	bcCtx := MustGetBlockchainCtx(ctx)
+	for _, validator := range bcCtx.Registry.All() {
+		if err := validator.Validate(ctx, selp.Action()); err != nil {
+			return err
+		}
+	}
 	return nil
 }

action/protocol/generic_validator_test.go

@@ -14,18 +14,25 @@ import (
 	"testing"
 	"time"
 
+	"github.com/golang/mock/gomock"
+	"github.com/iotexproject/iotex-address/address"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 
-	"github.com/iotexproject/iotex-address/address"
 	"github.com/iotexproject/iotex-core/action"
 	"github.com/iotexproject/iotex-core/config"
 	"github.com/iotexproject/iotex-core/state"
 	"github.com/iotexproject/iotex-core/test/identityset"
 )
 
-func TestActionProto(t *testing.T) {
+func TestActionProtoAndGenericValidator(t *testing.T) {
 	require := require.New(t)
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	reg := NewRegistry()
+	mp := NewMockProtocol(ctrl)
+	require.NoError(reg.Register("1", mp))
 	caller, err := address.FromString("io1mflp9m6hcgm2qcghchsdqj3z3eccrnekx9p0ms")
 	require.NoError(err)
 	producer, err := address.FromString("io1emxf8zzqckhgjde6dqd97ts0y3q496gm3fdrl6")
@@ -49,18 +56,21 @@ func TestActionProto(t *testing.T) {
 				Hash:      config.Default.Genesis.Hash(),
 				Timestamp: time.Unix(config.Default.Genesis.Timestamp, 0),
 			},
+			Registry: reg,
 		})
 
 	valid := NewGenericValidator(nil, func(sr StateReader, addr string) (*state.Account, error) {
-		if strings.EqualFold("io1emxf8zzqckhgjde6dqd97ts0y3q496gm3fdrl6", addr) {
+		pk := identityset.PrivateKey(27).PublicKey()
+		eAddr, _ := address.FromBytes(pk.Hash())
+		if strings.EqualFold(eAddr.String(), addr) {
 			return nil, errors.New("MockChainManager nonce error")
 		}
 		return &state.Account{Nonce: 2}, nil
 	})
 	data, err := hex.DecodeString("")
 	require.NoError(err)
-	// Case I: Normal
-	{
+	t.Run("normal", func(t *testing.T) {
+		mp.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil).Times(1)
 		v, err := action.NewExecution("", 0, big.NewInt(10), uint64(10), big.NewInt(10), data)
 		require.NoError(err)
 		bd := &action.EnvelopeBuilder{}
@@ -72,9 +82,8 @@ func TestActionProto(t *testing.T) {
 		nselp := action.SealedEnvelope{}
 		require.NoError(nselp.LoadProto(selp.Proto()))
 		require.NoError(valid.Validate(ctx, nselp))
-	}
-	// Case II: GasLimit lower
-	{
+	})
+	t.Run("Gas limit low", func(t *testing.T) {
 		v, err := action.NewExecution("", 0, big.NewInt(10), uint64(10), big.NewInt(10), data)
 		require.NoError(err)
 		bd := &action.EnvelopeBuilder{}
@@ -88,31 +97,23 @@ func TestActionProto(t *testing.T) {
 		err = valid.Validate(ctx, nselp)
 		require.Error(err)
 		require.True(strings.Contains(err.Error(), "insufficient gas"))
-	}
-	// Case III: Call cm Nonce err
-	{
-		caller, err := address.FromString("io1emxf8zzqckhgjde6dqd97ts0y3q496gm3fdrl6")
-		require.NoError(err)
-		ctx := WithActionCtx(ctx,
-			ActionCtx{
-				Caller: caller,
-			})
+	})
+	t.Run("state error", func(t *testing.T) {
 		v, err := action.NewExecution("", 0, big.NewInt(10), uint64(10), big.NewInt(10), data)
 		require.NoError(err)
 		bd := &action.EnvelopeBuilder{}
 		elp := bd.SetGasPrice(big.NewInt(10)).
 			SetGasLimit(uint64(100000)).
 			SetAction(v).Build()
-		selp, err := action.Sign(elp, identityset.PrivateKey(28))
+		selp, err := action.Sign(elp, identityset.PrivateKey(27))
 		require.NoError(err)
 		nselp := action.SealedEnvelope{}
 		require.NoError(nselp.LoadProto(selp.Proto()))
 		err = valid.Validate(ctx, nselp)
 		require.Error(err)
 		require.True(strings.Contains(err.Error(), "invalid state of account"))
-	}
-	// Case IV: Call Nonce err
-	{
+	})
+	t.Run("nonce too low", func(t *testing.T) {
 		v, err := action.NewExecution("", 1, big.NewInt(10), uint64(10), big.NewInt(10), data)
 		require.NoError(err)
 		bd := &action.EnvelopeBuilder{}
@@ -127,5 +128,40 @@ func TestActionProto(t *testing.T) {
 		err = valid.Validate(ctx, nselp)
 		require.Error(err)
 		require.True(strings.Contains(err.Error(), "nonce is too low"))
-	}
+	})
+	t.Run("wrong recipient", func(t *testing.T) {
+		v, err := action.NewTransfer(1, big.NewInt(1), "io1qyqsyqcyq5narhapakcsrhksfajfcpl24us3xp38zwvsep", []byte{}, uint64(100000), big.NewInt(10))
+		require.NoError(err)
+		bd := &action.EnvelopeBuilder{}
+		elp := bd.SetAction(v).SetGasLimit(100000).
+			SetGasPrice(big.NewInt(10)).
+			SetNonce(1).Build()
+		selp, err := action.Sign(elp, identityset.PrivateKey(27))
+		require.NoError(err)
+		require.Error(valid.Validate(ctx, selp))
+	})
+	t.Run("wrong signature", func(t *testing.T) {
+		unsignedTsf, err := action.NewTransfer(uint64(1), big.NewInt(1), caller.String(), []byte{}, uint64(100000), big.NewInt(0))
+		require.NoError(err)
+
+		bd := &action.EnvelopeBuilder{}
+		elp := bd.SetNonce(1).
+			SetAction(unsignedTsf).
+			SetGasLimit(100000).Build()
+		selp := action.FakeSeal(elp, identityset.PrivateKey(27).PublicKey())
+		require.True(strings.Contains(valid.Validate(ctx, selp).Error(), "failed to verify action signature"))
+	})
+	t.Run("protocol validation failed", func(t *testing.T) {
+		me := errors.New("mock error")
+		mp.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(me).Times(1)
+		v, err := action.NewExecution("", 0, big.NewInt(10), uint64(10), big.NewInt(10), data)
+		require.NoError(err)
+		bd := &action.EnvelopeBuilder{}
+		elp := bd.SetGasPrice(big.NewInt(10)).
+			SetGasLimit(uint64(100000)).
+			SetAction(v).Build()
+		selp, err := action.Sign(elp, identityset.PrivateKey(28))
+		require.NoError(err)
+		require.Equal(me, errors.Cause(valid.Validate(ctx, selp)))
+	})
 }

action/protocol/protocol.go

@@ -53,11 +53,6 @@ type ActionValidator interface {
 	Validate(context.Context, action.Action) error
 }
 
-// ActionEnvelopeValidator is the interface of validating an SealedEnvelope action
-type ActionEnvelopeValidator interface {
-	Validate(context.Context, action.SealedEnvelope) error
-}
-
 // ActionHandler is the interface for the action handlers. For each incoming action, the assembled actions will be
 // called one by one to process it. ActionHandler implementation is supposed to parse the sub-type of the action to
 // decide if it wants to handle this action or not.

action/sealedenvelopevalidator.go

@@ -0,0 +1,17 @@
+// Copyright (c) 2019 IoTeX Foundation
+// This is an alpha (internal) release and is not suitable for production. This source code is provided 'as is' and no
+// warranties are given as to title or non-infringement, merchantability or fitness for purpose and, to the extent
+// permitted by law, all liability for your use of the code is disclaimed. This source code is governed by Apache
+// License 2.0 that can be found in the LICENSE file.
+
+package action
+
+import (
+	"context"
+)
+
+// SealedEnvelopeValidator is the interface of validating an SealedEnvelope action
+type SealedEnvelopeValidator interface {
+	// Validate returns an error if any validation failed
+	Validate(context.Context, SealedEnvelope) error
+}