From 94430ef3ca1deab4572ba4b959ba5d32e7024996 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 17 Feb 2021 14:51:54 +0300 Subject: [PATCH] network: refactor RelayTx error handling We don't need to wrap different core errors in server. Also it would be good to provede more error info to the user. --- pkg/network/relay_reason.go | 17 -------- pkg/network/relay_reason_string.go | 29 -------------- pkg/network/server.go | 63 +++++++++--------------------- pkg/rpc/server/server.go | 18 ++++----- 4 files changed, 26 insertions(+), 101 deletions(-) delete mode 100644 pkg/network/relay_reason.go delete mode 100644 pkg/network/relay_reason_string.go diff --git a/pkg/network/relay_reason.go b/pkg/network/relay_reason.go deleted file mode 100644 index 0b58759ff..000000000 --- a/pkg/network/relay_reason.go +++ /dev/null @@ -1,17 +0,0 @@ -package network - -//go:generate stringer -type=RelayReason -output=relay_reason_string.go - -// RelayReason is the type which describes the different relay outcome. -type RelayReason uint8 - -// List of valid RelayReason. -const ( - RelaySucceed RelayReason = iota - RelayAlreadyExists - RelayOutOfMemory - RelayUnableToVerify - RelayInvalid - RelayPolicyFail - RelayUnknown -) diff --git a/pkg/network/relay_reason_string.go b/pkg/network/relay_reason_string.go deleted file mode 100644 index 0c2da4d04..000000000 --- a/pkg/network/relay_reason_string.go +++ /dev/null @@ -1,29 +0,0 @@ -// Code generated by "stringer -type=RelayReason -output=relay_reason_string.go"; DO NOT EDIT. - -package network - -import "strconv" - -func _() { - // An "invalid array index" compiler error signifies that the constant values have changed. - // Re-run the stringer command to generate them again. - var x [1]struct{} - _ = x[RelaySucceed-0] - _ = x[RelayAlreadyExists-1] - _ = x[RelayOutOfMemory-2] - _ = x[RelayUnableToVerify-3] - _ = x[RelayInvalid-4] - _ = x[RelayPolicyFail-5] - _ = x[RelayUnknown-6] -} - -const _RelayReason_name = "RelaySucceedRelayAlreadyExistsRelayOutOfMemoryRelayUnableToVerifyRelayInvalidRelayPolicyFailRelayUnknown" - -var _RelayReason_index = [...]uint8{0, 12, 30, 46, 65, 77, 92, 104} - -func (i RelayReason) String() string { - if i >= RelayReason(len(_RelayReason_index)-1) { - return "RelayReason(" + strconv.FormatInt(int64(i), 10) + ")" - } - return _RelayReason_name[_RelayReason_index[i]:_RelayReason_index[i+1]] -} diff --git a/pkg/network/server.go b/pkg/network/server.go index 648d65477..89b3bc302 100644 --- a/pkg/network/server.go +++ b/pkg/network/server.go @@ -13,7 +13,6 @@ import ( "github.com/nspcc-dev/neo-go/pkg/config/netmode" "github.com/nspcc-dev/neo-go/pkg/consensus" - "github.com/nspcc-dev/neo-go/pkg/core" "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" @@ -152,9 +151,8 @@ func newServerFromConstructors(config ServerConfig, chain blockchainer.Blockchai Log: log, } n, err := notary.NewNotary(cfg, s.notaryRequestPool, func(tx *transaction.Transaction) error { - r := s.RelayTxn(tx) - if r != RelaySucceed { - return fmt.Errorf("can't relay completed notary transaction: hash %s, reason: %s", tx.Hash().StringLE(), r.String()) + if err := s.RelayTxn(tx); err != nil { + return fmt.Errorf("can't relay completed notary transaction: hash %s, error: %w", tx.Hash().StringLE(), err) } return nil }) @@ -185,11 +183,10 @@ func newServerFromConstructors(config ServerConfig, chain blockchainer.Blockchai return nil, fmt.Errorf("can't initialize Oracle module: %w", err) } orc.SetOnTransaction(func(tx *transaction.Transaction) { - r := s.RelayTxn(tx) - if r != RelaySucceed { + if err := s.RelayTxn(tx); err != nil { orc.Log.Error("can't pool oracle tx", zap.String("hash", tx.Hash().StringLE()), - zap.Uint8("reason", byte(r))) + zap.Error(err)) } }) s.oracle = orc @@ -825,7 +822,7 @@ func (s *Server) handleExtensibleCmd(e *payload.Extensible) error { func (s *Server) handleTxCmd(tx *transaction.Transaction) error { // It's OK for it to fail for various reasons like tx already existing // in the pool. - if s.verifyAndPoolTX(tx) == RelaySucceed { + if s.verifyAndPoolTX(tx) == nil { s.consensus.OnTransaction(tx) s.broadcastTX(tx, nil) } @@ -837,35 +834,25 @@ func (s *Server) handleP2PNotaryRequestCmd(r *payload.P2PNotaryRequest) error { if !s.chain.P2PSigExtensionsEnabled() { return errors.New("P2PNotaryRequestCMD was received, but P2PSignatureExtensions are disabled") } + // It's OK for it to fail for various reasons like request already existing + // in the pool. s.RelayP2PNotaryRequest(r) return nil } // RelayP2PNotaryRequest adds given request to the pool and relays. It does not check // P2PSigExtensions enabled. -func (s *Server) RelayP2PNotaryRequest(r *payload.P2PNotaryRequest) RelayReason { - ret := s.verifyAndPoolNotaryRequest(r) - if ret == RelaySucceed { +func (s *Server) RelayP2PNotaryRequest(r *payload.P2PNotaryRequest) error { + err := s.verifyAndPoolNotaryRequest(r) + if err == nil { s.broadcastP2PNotaryRequestPayload(nil, r) } - return ret + return err } // verifyAndPoolNotaryRequest verifies NotaryRequest payload and adds it to the payload mempool. -func (s *Server) verifyAndPoolNotaryRequest(r *payload.P2PNotaryRequest) RelayReason { - if err := s.chain.PoolTxWithData(r.FallbackTransaction, r, s.notaryRequestPool, s.notaryFeer, verifyNotaryRequest); err != nil { - switch { - case errors.Is(err, core.ErrAlreadyExists): - return RelayAlreadyExists - case errors.Is(err, core.ErrOOM): - return RelayOutOfMemory - case errors.Is(err, core.ErrPolicy): - return RelayPolicyFail - default: - return RelayInvalid - } - } - return RelaySucceed +func (s *Server) verifyAndPoolNotaryRequest(r *payload.P2PNotaryRequest) error { + return s.chain.PoolTxWithData(r.FallbackTransaction, r, s.notaryRequestPool, s.notaryFeer, verifyNotaryRequest) } // verifyNotaryRequest is a function for state-dependant P2PNotaryRequest payload verification which is executed before ordinary blockchain's verification. @@ -1163,30 +1150,18 @@ func (s *Server) relayBlocksLoop() { } // verifyAndPoolTX verifies the TX and adds it to the local mempool. -func (s *Server) verifyAndPoolTX(t *transaction.Transaction) RelayReason { - if err := s.chain.PoolTx(t); err != nil { - switch { - case errors.Is(err, core.ErrAlreadyExists): - return RelayAlreadyExists - case errors.Is(err, core.ErrOOM): - return RelayOutOfMemory - case errors.Is(err, core.ErrPolicy): - return RelayPolicyFail - default: - return RelayInvalid - } - } - return RelaySucceed +func (s *Server) verifyAndPoolTX(t *transaction.Transaction) error { + return s.chain.PoolTx(t) } // RelayTxn a new transaction to the local node and the connected peers. // Reference: the method OnRelay in C#: https://github.com/neo-project/neo/blob/master/neo/Network/P2P/LocalNode.cs#L159 -func (s *Server) RelayTxn(t *transaction.Transaction) RelayReason { - ret := s.verifyAndPoolTX(t) - if ret == RelaySucceed { +func (s *Server) RelayTxn(t *transaction.Transaction) error { + err := s.verifyAndPoolTX(t) + if err == nil { s.broadcastTX(t, nil) } - return ret + return err } // broadcastTX broadcasts an inventory message about new transaction. diff --git a/pkg/rpc/server/server.go b/pkg/rpc/server/server.go index 2fa38c454..cc9b896ef 100644 --- a/pkg/rpc/server/server.go +++ b/pkg/rpc/server/server.go @@ -1266,24 +1266,20 @@ func (s *Server) submitNotaryRequest(ps request.Params) (interface{}, *response. } // getRelayResult returns successful relay result or an error. -func getRelayResult(relayReason network.RelayReason, hash util.Uint256) (interface{}, *response.Error) { - switch relayReason { - case network.RelaySucceed: +func getRelayResult(err error, hash util.Uint256) (interface{}, *response.Error) { + switch { + case err == nil: return result.RelayResult{ Hash: hash, }, nil - case network.RelayAlreadyExists: + case errors.Is(err, core.ErrAlreadyExists): return nil, response.ErrAlreadyExists - case network.RelayOutOfMemory: + case errors.Is(err, core.ErrOOM): return nil, response.ErrOutOfMemory - case network.RelayUnableToVerify: - return nil, response.ErrUnableToVerify - case network.RelayInvalid: - return nil, response.ErrValidationFailed - case network.RelayPolicyFail: + case errors.Is(err, core.ErrPolicy): return nil, response.ErrPolicyFail default: - return nil, response.ErrUnknown + return nil, response.ErrValidationFailed } }