Flush unrecognized network messages from the read buffer #529
Conversation
src/network/message.rs
Outdated
| @@ -70,7 +70,7 @@ impl Encodable for CommandString { | |||
| let mut rawbytes = [0u8; 12]; | |||
| let strbytes = self.0.as_bytes(); | |||
| if strbytes.len() > 12 { | |||
| return Err(encode::Error::UnrecognizedNetworkCommand(self.0.clone().into_owned())); | |||
| return Err(encode::Error::UnrecognizedNetworkCommand(self.0.clone().into_owned(), strbytes.len())); | |||
There was a problem hiding this comment.
I think we should give this a separate error variant.
There was a problem hiding this comment.
How about NetworkCommandTooLong? or just a more generic InvalidNetworkCommand?
There was a problem hiding this comment.
I think TooLong is the better one. That's been the only invalidity condition for many years and I can't imagine it changing :).
Currently whenever an unrecognized network message is received, it is never flushed from the read buffer, meaning that unless the stream is closed and recreated it will keep returning the same error every time `read_next()` is called. This commit adds the length of the message to `UnrecognizedNetworkCommand`, so that the `StreamReader` can flush those bytes before returning the error to the caller.
afilini
force-pushed
the
fix/flush-unrecognized-messages
branch
from
28034b3
to
e33b914
Compare
apoelstra
added
the
API break
|
It seems that some of the error variant changes are no longer required after https://github.com/rust-bitcoin/rust-bitcoin/pull/496/files. Let's wait for #496 and #494 and rebase this on top of that. But yeah I ended up not using the Stream thingy in bitcoin-p2p because it forces you to use a buffer per stream instead of one global buffer for all peers. |
| @@ -68,6 +68,10 @@ impl<R: Read> StreamReader<R> { | |||
| return Err(encode::Error::Io(io::Error::from(io::ErrorKind::UnexpectedEof))); | |||
| } | |||
| }, | |||
| Err(encode::Error::UnrecognizedNetworkCommand(message, len)) => { | |||
| self.unparsed.drain(..len); | |||
There was a problem hiding this comment.
Just a note for future readers: This can't panic because for the UnrecognizedNetworkCommand error to be returned the payload length has to already have been verified in CheckedData::consensus_decode.
Currently whenever an unrecognized network message is received, it is never flushed from the read buffer, meaning that unless the stream is closed and recreated it will keep returning the same error every time
read_next()is called.This commit adds the length of the message to
UnrecognizedNetworkCommand, so that theStreamReadercan flush those bytes before returning the error to the caller.@RCasatta and I encountered this issue while connecting to a Bitcoin Core 0.21-rc2 node using the latest release of
rust-bitcoin. The specific error we were getting aboutsendaddrv2being unrecognized should already be fixed here inmaster, but still this is probably an issue worth fixing for messages that could be added in the future to the P2P protocol.