From 5fda1e64cf4130bd02443245491def91d49f7fde Mon Sep 17 00:00:00 2001 From: jimboj Date: Mon, 19 Sep 2022 16:34:42 -0600 Subject: [PATCH] feedback --- dot/core/messages.go | 16 +++--- dot/core/messages_test.go | 4 +- dot/core/service_integration_test.go | 1 + dot/rpc/modules/author_integration_test.go | 1 - lib/runtime/invalid_transaction.go | 7 ++- lib/runtime/invalid_transaction_test.go | 46 --------------- lib/runtime/transaction_validity.go | 34 +++++------ lib/runtime/transaction_validity_test.go | 65 ++++++++++++++++++---- lib/runtime/unknown_transaction.go | 4 +- lib/runtime/unknown_transaction_test.go | 46 --------------- lib/runtime/wasmer/exports.go | 8 +-- 11 files changed, 91 insertions(+), 141 deletions(-) delete mode 100644 lib/runtime/invalid_transaction_test.go delete mode 100644 lib/runtime/unknown_transaction_test.go diff --git a/dot/core/messages.go b/dot/core/messages.go index 1b244f72b6..0b7a7feada 100644 --- a/dot/core/messages.go +++ b/dot/core/messages.go @@ -70,34 +70,32 @@ func (s *Service) HandleTransactionMessage(peerID peer.ID, msg *network.Transact return false, err } - allTxsAreValid := true + allTxnsAreValid := true for _, tx := range txs { - isValidTxn := true + txnIsValid := true validity, err := s.validateTransaction(head, rt, tx) if err != nil { + txnIsValid = false + allTxnsAreValid = false switch err.(type) { case runtime.InvalidTransaction: - isValidTxn = false - allTxsAreValid = false s.net.ReportPeer(peerset.ReputationChange{ Value: peerset.BadTransactionValue, Reason: peerset.BadTransactionReason, }, peerID) case runtime.UnknownTransaction: - isValidTxn = false - allTxsAreValid = false default: - return false, fmt.Errorf("failed validating transaction for peerID %s: %w", peerID, err) + return false, fmt.Errorf("validating transaction from peerID %s: %w", peerID, err) } } - if isValidTxn && validity.Propagate { + if txnIsValid && validity.Propagate { // find tx(s) that should propagate toPropagate = append(toPropagate, tx) } } - if allTxsAreValid { + if allTxnsAreValid { s.net.ReportPeer(peerset.ReputationChange{ Value: peerset.GoodTransactionValue, Reason: peerset.GoodTransactionReason, diff --git a/dot/core/messages_test.go b/dot/core/messages_test.go index 9fcadbf25c..38bb2fd5e8 100644 --- a/dot/core/messages_test.go +++ b/dot/core/messages_test.go @@ -230,7 +230,7 @@ func TestServiceHandleTransactionMessage(t *testing.T) { }, }, expErr: errDummyErr, - expErrMsg: "failed validating transaction for peerID D1KeRhQ: cannot get trie state from storage" + + expErrMsg: "validating transaction from peerID D1KeRhQ: cannot get trie state from storage" + " for root 0x0000000000000000000000000000000000000000000000000000000000000000: dummy error for testing", }, { @@ -262,7 +262,7 @@ func TestServiceHandleTransactionMessage(t *testing.T) { setContextStorage: &mockSetContextStorage{trieState: &storage.TrieState{}}, validateTxn: &mockValidateTxn{ input: types.Extrinsic(append([]byte{byte(types.TxnExternal)}, testExtrinsic[0]...)), - err: invalidTransaction, + err: *invalidTransaction, }, }, args: args{ diff --git a/dot/core/service_integration_test.go b/dot/core/service_integration_test.go index 1b556de149..e7f30a171a 100644 --- a/dot/core/service_integration_test.go +++ b/dot/core/service_integration_test.go @@ -554,6 +554,7 @@ func TestService_HandleSubmittedExtrinsic(t *testing.T) { require.NoError(t, err) extBytes := createExtrinsic(t, rt, genHeader.Hash(), 0) + err = s.HandleSubmittedExtrinsic(extBytes) require.NoError(t, err) } diff --git a/dot/rpc/modules/author_integration_test.go b/dot/rpc/modules/author_integration_test.go index c85d8d2f49..443ac2fd2e 100644 --- a/dot/rpc/modules/author_integration_test.go +++ b/dot/rpc/modules/author_integration_test.go @@ -218,7 +218,6 @@ func TestAuthorModule_SubmitExtrinsic_AlreadyInPool(t *testing.T) { // creating an extrisinc to the System.remark call using a sample argument extHex := runtime.NewTestExtrinsic(t, integrationTestController.runtime, genesisHash, genesisHash, 0, "System.remark", []byte{}) - fmt.Println(extHex) extBytes := common.MustHexToBytes(extHex) integrationTestController.storageState = &state.StorageState{} diff --git a/lib/runtime/invalid_transaction.go b/lib/runtime/invalid_transaction.go index 62179b8c4f..8ec373d1e2 100644 --- a/lib/runtime/invalid_transaction.go +++ b/lib/runtime/invalid_transaction.go @@ -48,13 +48,14 @@ func (i InvalidTransaction) Error() string { } // NewInvalidTransaction is constructor for InvalidTransaction -func NewInvalidTransaction() InvalidTransaction { +func NewInvalidTransaction() *InvalidTransaction { vdt, err := scale.NewVaryingDataType(Call{}, Payment{}, Future{}, Stale{}, BadProof{}, AncientBirthBlock{}, ExhaustsResources{}, InvalidCustom(0), BadMandatory{}, MandatoryDispatch{}) if err != nil { panic(err) } - return InvalidTransaction(vdt) + invalidTxnVdr := InvalidTransaction(vdt) + return &invalidTxnVdr } // Call The call of the transaction is not expected @@ -140,7 +141,7 @@ type InvalidCustom uint8 // Index returns the VDT index func (InvalidCustom) Index() uint { return 7 } -// Error returns the error message associated with the Call +// Error returns the error message associated with the InvalidCustom func (i InvalidCustom) Error() string { return newUnknownError(i).Error() } diff --git a/lib/runtime/invalid_transaction_test.go b/lib/runtime/invalid_transaction_test.go deleted file mode 100644 index ce54626172..0000000000 --- a/lib/runtime/invalid_transaction_test.go +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright 2022 ChainSafe Systems (ON) -// SPDX-License-Identifier: LGPL-3.0-only - -package runtime - -import ( - "testing" - - "github.com/ChainSafe/gossamer/lib/transaction" - - "github.com/stretchr/testify/require" -) - -func Test_InvalidTransaction_Errors(t *testing.T) { - t.Parallel() - testCases := []struct { - name string - encodedData []byte - expErr bool - expErrMsg string - expValidity *transaction.Validity - }{ - { - name: "ancient birth block", - encodedData: []byte{1, 0, 5}, - expErrMsg: "ancient birth block", - expErr: true, - }, - } - - for _, c := range testCases { - c := c - t.Run(c.name, func(t *testing.T) { - t.Parallel() - validity, err := UnmarshalTransactionValidity(c.encodedData) - if !c.expErr { - require.NoError(t, err) - } else { - require.Error(t, err) - require.EqualError(t, err, c.expErrMsg) - } - - require.Equal(t, c.expValidity, validity) - }) - } -} diff --git a/lib/runtime/transaction_validity.go b/lib/runtime/transaction_validity.go index 7ecfa0cd8e..bd15aa4309 100644 --- a/lib/runtime/transaction_validity.go +++ b/lib/runtime/transaction_validity.go @@ -50,7 +50,7 @@ func (tve TransactionValidityError) Error() string { // NewTransactionValidityError is constructor for TransactionValidityError func NewTransactionValidityError() *TransactionValidityError { - vdt, err := scale.NewVaryingDataType(NewInvalidTransaction(), NewUnknownTransaction()) + vdt, err := scale.NewVaryingDataType(*NewInvalidTransaction(), NewUnknownTransaction()) if err != nil { panic(err) } @@ -66,28 +66,28 @@ func UnmarshalTransactionValidity(res []byte) (*transaction.Validity, error) { txnValidityResult := scale.NewResult(validTxn, *txnValidityErrResult) err := scale.Unmarshal(res, &txnValidityResult) if err != nil { - return nil, fmt.Errorf("scale decoding transaction validity result: %w", err) + return nil, fmt.Errorf("scale decoding transaction validity result: %s", err) } txnValidityRes, err := txnValidityResult.Unwrap() if err != nil { scaleWrappedErr, ok := err.(scale.WrappedErr) - if ok { - txnValidityErr, ok := scaleWrappedErr.Err.(TransactionValidityError) - if !ok { - return nil, fmt.Errorf("%w: %T", errors.New("invalid type cast"), scaleWrappedErr.Err) - } + if !ok { + return nil, fmt.Errorf("unwrapping transaction validity result: %s", err) + } + txnValidityErr, ok := scaleWrappedErr.Err.(TransactionValidityError) + if !ok { + return nil, fmt.Errorf("%w: %T", errors.New("invalid type cast"), scaleWrappedErr.Err) + } - switch val := txnValidityErr.Value().(type) { - // TODO use custom result issue #2780 - case InvalidTransaction: - return nil, val - case UnknownTransaction: - return nil, val - default: - return nil, fmt.Errorf("unsupported transaction validity error: %T", txnValidityErr.Value()) - } + switch val := txnValidityErr.Value().(type) { + // TODO use custom result issue #2780 + case InvalidTransaction: + return nil, val + case UnknownTransaction: + return nil, val + default: + return nil, fmt.Errorf("unsupported transaction validity error: %T", txnValidityErr.Value()) } - return nil, fmt.Errorf("%w: %T", errors.New("invalid error value"), err) } validity, ok := txnValidityRes.(transaction.Validity) if !ok { diff --git a/lib/runtime/transaction_validity_test.go b/lib/runtime/transaction_validity_test.go index 5355d21474..10acf227d2 100644 --- a/lib/runtime/transaction_validity_test.go +++ b/lib/runtime/transaction_validity_test.go @@ -4,25 +4,70 @@ package runtime import ( - "errors" "testing" + "github.com/ChainSafe/gossamer/lib/transaction" "github.com/ChainSafe/gossamer/pkg/scale" "github.com/stretchr/testify/require" ) -func Test_ErrorsAs_Function(t *testing.T) { - transactionValidityErr := NewTransactionValidityError() - unknownTransaction := NewUnknownTransaction() - err := unknownTransaction.Set(NoUnsignedValidator{}) +func Test_UnmarshalTransactionValidity(t *testing.T) { + t.Parallel() + + txtValidity := transaction.Validity{Priority: 1} + txnValidityResult := scale.NewResult(transaction.Validity{}, NewTransactionValidityError()) + err := txnValidityResult.Set(scale.OK, txtValidity) require.NoError(t, err) - err = transactionValidityErr.Set(unknownTransaction) + encResult, err := scale.Marshal(txnValidityResult) require.NoError(t, err) - - var txnValErr *TransactionValidityError - isTxnValErr := errors.As(transactionValidityErr, &txnValErr) - require.True(t, isTxnValErr) + testCases := []struct { + name string + encodedData []byte + expErr bool + expErrMsg string + expValidity *transaction.Validity + }{ + { + name: "ancient birth block", + encodedData: []byte{1, 0, 5}, + expErrMsg: "ancient birth block", + expErr: true, + }, + { + name: "lookup failed", + encodedData: []byte{1, 1, 0}, + expErrMsg: "lookup failed", + expErr: true, + }, + { + name: "unmarshal error", + encodedData: []byte{1}, + expErrMsg: "scale decoding transaction validity result: EOF", + expErr: true, + }, + { + name: "valid case", + encodedData: encResult, + expValidity: &txtValidity, + }, + } + + for _, c := range testCases { + c := c + t.Run(c.name, func(t *testing.T) { + t.Parallel() + validity, err := UnmarshalTransactionValidity(c.encodedData) + if !c.expErr { + require.NoError(t, err) + } else { + require.Error(t, err) + require.EqualError(t, err, c.expErrMsg) + } + + require.Equal(t, c.expValidity, validity) + }) + } } func Test_InvalidTransactionValidity(t *testing.T) { diff --git a/lib/runtime/unknown_transaction.go b/lib/runtime/unknown_transaction.go index a813c673fd..29c17a831a 100644 --- a/lib/runtime/unknown_transaction.go +++ b/lib/runtime/unknown_transaction.go @@ -9,7 +9,7 @@ import ( "github.com/ChainSafe/gossamer/pkg/scale" ) -// UnknownTransaction is child VDT of TransactionValidityError +// UnknownTransaction is the child VDT of TransactionValidityError type UnknownTransaction scale.VaryingDataType // Index returns the VDT index @@ -28,7 +28,7 @@ func (u *UnknownTransaction) Set(val scale.VaryingDataTypeValue) (err error) { return nil } -// Value will return value from underying VaryingDataType +// Value will return value from the underying VaryingDataType func (u *UnknownTransaction) Value() (val scale.VaryingDataTypeValue) { vdt := scale.VaryingDataType(*u) return vdt.Value() diff --git a/lib/runtime/unknown_transaction_test.go b/lib/runtime/unknown_transaction_test.go deleted file mode 100644 index 2f04afb973..0000000000 --- a/lib/runtime/unknown_transaction_test.go +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright 2022 ChainSafe Systems (ON) -// SPDX-License-Identifier: LGPL-3.0-only - -package runtime - -import ( - "testing" - - "github.com/ChainSafe/gossamer/lib/transaction" - - "github.com/stretchr/testify/require" -) - -func Test_UnknownTransaction_Errors(t *testing.T) { - t.Parallel() - testCases := []struct { - name string - encodedData []byte - expErr bool - expErrMsg string - expValidity *transaction.Validity - }{ - { - name: "lookup failed", - encodedData: []byte{1, 1, 0}, - expErrMsg: "lookup failed", - expErr: true, - }, - } - - for _, c := range testCases { - c := c - t.Run(c.name, func(t *testing.T) { - t.Parallel() - validity, err := UnmarshalTransactionValidity(c.encodedData) - if !c.expErr { - require.NoError(t, err) - } else { - require.Error(t, err) - require.EqualError(t, err, c.expErrMsg) - } - - require.Equal(t, c.expValidity, validity) - }) - } -} diff --git a/lib/runtime/wasmer/exports.go b/lib/runtime/wasmer/exports.go index 4bb4718108..c8c3734706 100644 --- a/lib/runtime/wasmer/exports.go +++ b/lib/runtime/wasmer/exports.go @@ -13,11 +13,9 @@ import ( ) // ValidateTransaction runs the extrinsic through the runtime function -// TaggedTransactionQueue_validate_transaction and returns *Validity. Two types of errors -// are returned here: 1) *txnvalidity.transactionValidityError is a VDT containing -// VDTs of the types of transaction validity errors. The whole VDT is returned so -// the caller can handle as it seems fit, as this will vary per use case. 2) normal error -// returned if something fails in the process i.e. unmarshalling error +// TaggedTransactionQueue_validate_transaction and returns **transaction.Validity. The error can +// be a VDT of either transaction.InvalidTransaction or transaction.UnknownTransaction, or can represent +// a normal error i.e. unmarshalling error func (in *Instance) ValidateTransaction(e types.Extrinsic) ( *transaction.Validity, error) { ret, err := in.Exec(runtime.TaggedTransactionQueueValidateTransaction, e)