Grow noise buffers dynamically. #1436
Conversation
Currently we allocate a buffer of 176 KiB for each noise state, i.e. each connection. For connections which see only small data frames this is wasteful. At the same time we limit the max. write buffer size to 16 KiB to keep the total buffer size relatively small, which results in smaller encrypted messages and also makes it less likely to ever encounter the max. noise package size of 64 KiB in practice when communicating with other nodes using the same implementation. This PR repaces the static buffer allocation with a dynamic one. We only reserve a small space for the authentication tag plus some extra reserve and are able to buffer larger data frames before encrypting.
| @@ -443,7 +414,7 @@ fn read_frame_len<R: AsyncRead + Unpin>( | |||
| cx: &mut Context<'_>, | |||
| buf: &mut [u8; 2], | |||
| off: &mut usize, | |||
| ) -> Poll<Result<Option<u16>, std::io::Error>> { | |||
| ) -> Poll<io::Result<Option<u16>>> { | |||
There was a problem hiding this comment.
Commenting on this PR as well, because I comment every time someone does that: I'm not a fan of doing random codestyle changes, especially for code that isn't otherwise touched by the PR.
| const MAX_WRITE_BUF_LEN: usize = 16384; | ||
| const TOTAL_BUFFER_LEN: usize = 2 * MAX_NOISE_PKG_LEN + 3 * MAX_WRITE_BUF_LEN; | ||
| /// Extra space given to the encryption buffer to hold key material. | ||
| const EXTRA_ENCRYPT_SPACE: usize = 1024; |
There was a problem hiding this comment.
That constant looks a bit like dark magic to me, but the test would cover any possible mistake here anyway.
| &buffer.read[.. len], | ||
| buffer.read_crypto | ||
| ){ | ||
| this.decrypt_buffer.resize(len, 0u8); |
There was a problem hiding this comment.
At this point len is the size of the encrypted message in this.read_buffer, correct? Do you resize this.decrypt_buffer to len to ensure it is large enough, given that the decrypted payload will always be smaller than the encrypted payload?
protocols/noise/src/io.rs
Outdated
| loop { | ||
| trace!("write state: {:?}", this.write_state); | ||
| match this.write_state { | ||
| WriteState::Init => { | ||
| this.write_state = WriteState::BufferData { off: 0 } | ||
| } | ||
| WriteState::BufferData { ref mut off } => { | ||
| let n = std::cmp::min(MAX_WRITE_BUF_LEN - *off, buf.len()); | ||
| buffer.write[*off .. *off + n].copy_from_slice(&buf[.. n]); | ||
| let n = min(MAX_WRITE_BUF_LEN, this.write_buffer.len().saturating_add(buf.len())); |
There was a problem hiding this comment.
Do I understand correctly, that we never set this.write_buffer back to 0. Let's say one always calls poll_write with a very small buf (e.g. length of 1). In addition let's say one always calls poll_flush right afterwards each time.
As far as I can tell this.write_buffer would eventually be as large as MAX_WRITE_BUF_LEN due to this.write_buffer.len().saturating_add(buf.len()) followed by this.write_buffer.resize(n, 0u8); even though one only ever needed this.write_buffer to be of length 1.
Would resizing this.write_buffer with the current offset (off) instead of this.write_buffer.len() solve this?
| let n = min(MAX_WRITE_BUF_LEN, this.write_buffer.len().saturating_add(buf.len())); | |
| let n = min(MAX_WRITE_BUF_LEN, off.saturating_add(buf.len())); |
As suggested by @mxinden, this prevents increasing the write buffer up to MAX_WRITE_BUF_LEN.
| @@ -40,7 +41,8 @@ fn core_upgrade_compat() { | |||
| #[test] | |||
| fn xx() { | |||
| let _ = env_logger::try_init(); | |||
| fn prop(message: Vec<u8>) -> bool { | |||
| fn prop(mut messages: Vec<Message>) -> bool { | |||
| messages.truncate(5); | |||
There was a problem hiding this comment.
I am guessing that you are truncateing messages here to prevent huge messages vector, right?
Why not use TestResult::discard when messages is greater than 5. Would increase the entropy of the test as it would also run with 4, 3, 2 and 1 message.
See for comparison: https://github.com/libp2p/rust-libp2p/blob/master/protocols/gossipsub/tests/smoke.rs#L171
There was a problem hiding this comment.
I am guessing that you are
truncateingmessageshere to prevent hugemessagesvector, right?
Yes.
Why not use
TestResult::discardwhenmessagesis greater than 5.
It is not so much the length that matters here, but just that multiple messages are transmitted. If generation is skewed towards the upper bound 5 it is alright. Truncating the input means less input is generated only to be discarded. That being said I am not opposed to change this to use TestResult::discard.
Would increase the entropy of the test as it would also run with 4, 3, 2 and 1 message.
The test runs just fine with vectors of length 0 to 4.
There was a problem hiding this comment.
I am fine either way. In case you think this would be beneficial, add it, otherwise leave it as is with static 5.
Currently we allocate a buffer of 176 KiB for each noise state, i.e. each connection. For connections which see only small data frames this is wasteful. At the same time we limit the max. write buffer size to 16 KiB to keep the total buffer size relatively small, which results in smaller encrypted messages and also makes it less likely to ever encounter the max. noise package size of 64 KiB in practice when communicating with other nodes using the same implementation.
This PR replaces the static buffer allocation with a dynamic one. We only reserve a small space for the authentication tag plus some extra reserve and are able to buffer larger data frames before encrypting.
In addition the noise smoke tests have been changed to send a sequence of messages over the connection and not just one to better test the state handling. The messages themselves are also often larger now to test that the max. noise package size is not exceeded and write buffer handling works properly.