wire: Do not read discarded input

[jrick]

When ReadMessage/ReadMessageN error due to the message having an invalid network magic or invalid encoding of the command string, error immediately instead of reading the remaining byte count reported in the message header. There is no good reason to do so, and the explanation given by the discardInput comment is bogus.

Unlike message serialization for generic purposes (via the BtcEncode method or the Message interface), ReadMessage{,N} is intended to only be used for network connections which have already negotiated a protocol version. Upon
any obvious error, the socket should just be closed (as dcrd's peer and dcrwallet's p2p package both do) without reading any more input.

The previous behavior of discarding this input to read the next wire protocol message is not documented by ReadMessage{,N}. Changing this is considered to be a bug fix rather than a major API break.

[davecgh]

This is all a remnant of legacy BTC things that have never applied to Decred, so I have no objections to removing it.

However, I wonder if we should update the comments on ReadMessage and ReadMessageN to call out the semantics in the event of error, because this is technically a change to the semantics. Since they're undocumented as it stands, and we also happen to know that all known consumers won't be affected by it, I think it's fine, but is a change nonetheless.

In particular, prior to the change, it is possible to ignore unknown messages (and even slightly malformed messages) because it discards the payload of that unknown message leaving it ready to read the next message. After this change, that is no longer true since it will not consume the payload leaving the input stream in a state that is no longer usable.

In practice, I think that change in semantics is perfectly acceptable because we have protocol versions that are negotiated and sending unknown messages are a violation of the protocol which means the offending peer should be immediately disconnected, which is in fact what both dcrd and dcrwallet already correctly do.

Also, I will note that the commit message is incorrect regarding the comment being bogus, so please update the commit message to remove that.

and the explanation given by the discardInput comment is bogus.

The function is standalone and could have theoretically be called from anywhere. If the calling code were to make a mistake and not properly check for a reasonable maximum size and fail prior to invoking the function and the function were instead to naively allocate one big slice of whatever size the function was provided, it would in fact allow rogue nodes to cause massive memory allocation.

I'm aware that no actual instances of calling the function without first checking the size exist since the wire code is generally paranoid, but that is what the comment is referring to and it isn't bogus.

[jrick]

I will push back that the bogus description is valid.

discardInput is a helper function and we know the callers. It is clearly a helper function, and we can evaluate all places it is used. In those places, it is indeed bogus, as the read message length has already been validated and is known to not exceed the protocol maximum.

Even if we assume the comment is valid, it is not saving memory resources in any smart way by doing so, as it could simply write the read bytes, copying them to io.Discard instead of to actual memory allocations.

[davecgh]

I pushed back a bit on that bit of the commit message because I prefer to have accurate commit messages for anyone reviewing history and it just doesn't seem like an accurate assessment to me, but given your position, I suppose it depends on how you want to interpret it. I still think it was fine, but given it's being removed now anyway, it doesn't matter and isn't any reason to hold up the PR.