From ce77d520e548db19d89fdfbf72d1fc4f1398baf4 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 3 Feb 2021 00:43:16 -0600 Subject: [PATCH] peer: Stop sending and logging reject messages. This modifies the peer package to stop sending and logging reject messages. This is part of deprecating the reject wire protocol message towards its eventual removal in a future version of the software. --- peer/log.go | 45 --------------------------------------------- peer/peer.go | 48 +++++++++++++++--------------------------------- 2 files changed, 15 insertions(+), 78 deletions(-) diff --git a/peer/log.go b/peer/log.go index 6c7233b7..49cada81 100644 --- a/peer/log.go +++ b/peer/log.go @@ -7,7 +7,6 @@ package peer import ( "fmt" - "strings" "time" "github.com/decred/dcrd/chaincfg/chainhash" @@ -16,12 +15,6 @@ import ( "github.com/decred/slog" ) -const ( - // maxRejectReasonLen is the maximum length of a sanitized reject reason - // that will be logged. - maxRejectReasonLen = 250 -) - // log is a logger that is initialized with no output filters. This // means the package will not perform any logging by default until the caller // requests it. @@ -112,30 +105,6 @@ func locatorSummary(locator []*chainhash.Hash, stopHash *chainhash.Hash) string return fmt.Sprintf("no locator, stop %s", stopHash) } -// sanitizeString strips any characters which are even remotely dangerous, such -// as html control characters, from the passed string. It also limits it to -// the passed maximum size, which can be 0 for unlimited. When the string is -// limited, it will also add "..." to the string to indicate it was truncated. -func sanitizeString(str string, maxLength uint) string { - const safeChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY" + - "Z01234567890 .,;_/:?@" - - // Strip any characters not in the safeChars string removed. - str = strings.Map(func(r rune) rune { - if strings.ContainsRune(safeChars, r) { - return r - } - return -1 - }, str) - - // Limit the string to the max allowed length. - if maxLength > 0 && uint(len(str)) > maxLength { - str = str[:maxLength] - str = str + "..." - } - return str -} - // messageSummary returns a human-readable string which summarizes a message. // Not all messages have or need a summary. This is used for debug logging. func messageSummary(msg wire.Message) string { @@ -196,20 +165,6 @@ func messageSummary(msg wire.Message) string { } return summary - case *wire.MsgReject: - // Ensure the variable length strings don't contain any - // characters which are even remotely dangerous such as HTML - // control characters, etc. Also limit them to sane length for - // logging. - rejCommand := sanitizeString(msg.Cmd, wire.CommandSize) - rejReason := sanitizeString(msg.Reason, maxRejectReasonLen) - summary := fmt.Sprintf("cmd %v, code %v, reason %v", rejCommand, - msg.Code, rejReason) - if rejCommand == wire.CmdBlock || rejCommand == wire.CmdTx { - summary += fmt.Sprintf(", hash %v", msg.Hash) - } - return summary - case *wire.MsgGetInitState: return fmt.Sprintf("types %v", msg.Types) diff --git a/peer/peer.go b/peer/peer.go index 5caa65be..8cf08c48 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -178,14 +178,18 @@ type MessageListeners struct { OnFeeFilter func(p *Peer, msg *wire.MsgFeeFilter) // OnVersion is invoked when a peer receives a version wire message. - // The caller may return a reject message in which case the message will - // be sent to the peer and the peer will be disconnected. + // + // Deprecated: The return value is no longer used beyond logging it and + // causing the peer to be disconnected when it is a non-nil value. OnVersion func(p *Peer, msg *wire.MsgVersion) *wire.MsgReject // OnVerAck is invoked when a peer receives a verack wire message. OnVerAck func(p *Peer, msg *wire.MsgVerAck) // OnReject is invoked when a peer receives a reject wire message. + // + // Deprecated: This will be removed in a future release. Callers should + // avoid using it. OnReject func(p *Peer, msg *wire.MsgReject) // OnSendHeaders is invoked when a peer receives a sendheaders wire @@ -1294,22 +1298,10 @@ out: // needed. rmsg, buf, err := p.readMessage() if err != nil { - // Only log the error and send reject message if the - // local peer is not forcibly disconnecting and the - // remote peer has not disconnected. + // Only log the error if the local peer is not forcibly + // disconnecting and the remote peer has not disconnected. if p.shouldHandleReadError(err) { - errMsg := fmt.Sprintf("Can't read message from %s: %v", p, err) - log.Errorf(errMsg) - - // Push a reject message for the malformed message and wait for - // the message to be sent before disconnecting. - // - // NOTE: Ideally this would include the command in the header if - // at least that much of the message was valid, but that is not - // currently exposed by wire, so just used malformed for the - // command. - p.PushRejectMsg("malformed", wire.RejectMalformed, errMsg, nil, - true) + log.Errorf("Can't read message from %s: %v", p, err) } var nErr net.Error @@ -1328,16 +1320,15 @@ out: switch msg := rmsg.(type) { case *wire.MsgVersion: // Limit to one version message per peer. - p.PushRejectMsg(msg.Command(), wire.RejectDuplicate, - "duplicate version message", nil, true) + log.Debugf("Already received 'version' from peer %s -- "+ + "disconnecting", p) break out case *wire.MsgVerAck: - // No read lock is necessary because verAckReceived is not written // to in any other goroutine. if p.verAckReceived { - log.Infof("Already received 'verack' from peer %v -- "+ + log.Infof("Already received 'verack' from peer %s -- "+ "disconnecting", p) break out } @@ -1878,27 +1869,18 @@ func (p *Peer) readRemoteVersionMsg() error { p.flagsMtx.Unlock() // Invoke the callback if specified. In the case the callback returns a - // reject message, notify and disconnect the peer accordingly. + // reject message, disconnect the peer accordingly. if p.cfg.Listeners.OnVersion != nil { rejectMsg := p.cfg.Listeners.OnVersion(p, msg) if rejectMsg != nil { - _ = p.writeMessage(rejectMsg) return errors.New(rejectMsg.Reason) } } - // Notify and disconnect clients that have a protocol version that is - // too old. + // Disconnect clients that have a protocol version that is too old. if msg.ProtocolVersion < int32(wire.InitialProcotolVersion) { - // Send a reject message indicating the protocol version is - // obsolete and wait for the message to be sent before - // disconnecting. - reason := fmt.Sprintf("protocol version must be %d or greater", + return fmt.Errorf("protocol version must be %d or greater", wire.InitialProcotolVersion) - rejectMsg := wire.NewMsgReject(msg.Command(), wire.RejectObsolete, - reason) - _ = p.writeMessage(rejectMsg) - return errors.New(reason) } return nil