fix(dht/encryption): greatly reduce heap allocations for encrypted messaging #4753
Conversation
sdbondi
added
the
P-controversial
sdbondi
force-pushed
the
dht-message-padding-encrypt-optimise
branch
from
9e23de3 to
94be522
Compare
sdbondi
force-pushed
the
dht-message-padding-encrypt-optimise
branch
from
94be522 to
c7a2c2b
Compare
| node_identity: &NodeIdentity, | ||
| body: Vec<u8>, | ||
| body: &T, |
There was a problem hiding this comment.
You might be able to save a lot of &[...].to_vec() calls with this type of thing:
fn foo<R: Debug + ?Sized, T: AsRef<R>>(t: T) {
println!("{:?}", t.as_ref());
}
fn main() {
let v = vec![1];
foo::<Vec<i32>, _>(&v);
foo::<[i32], _>(&[1,2,3]);
}There was a problem hiding this comment.
We want this to work for any impl prost::Message, [u8] nor &[u8] implement this but Vec does which is why, for tests only, we get the wrong looking &[..].to_vec(). Some tests need to pass in a DhtEnvelope type which is the case where &T where T: prost::Message means the test doesn't have to serialize first (and we can use the non-test function prepare_message instead of reimplementing that for tests).
| @@ -213,12 +213,12 @@ mod test { | |||
| #[test] | |||
| fn deterministic_hash() { | |||
| const TEST_MSG: &[u8] = b"test123"; | |||
| const EXPECTED_HASH: &str = "d6333668f259f677703fbe4e89152ee41c7c01f6dec502befc63120246523ffe"; | |||
| const EXPECTED_HASH: &str = "1c2bb1bcff443af4441b789bd1d6984bb8d7bed2c9f85e8cf4f45615fdd9e47d"; | |||
There was a problem hiding this comment.
😕 Why does the hash change here? It looks like this should be a drop-in replacement?
There was a problem hiding this comment.
Yeah looks like a breaking change :) This has more to do with changing the test helper to take a prost::Message instead of raw bytes. Now the Vec is serialized into protobuf before being hashed, which matches thebehaviour of the real code.
sdbondi
force-pushed
the
dht-message-padding-encrypt-optimise
branch
from
7617351 to
d8aca37
Compare
|
Really great ! Was unaware of this issue, and seeing how to fix it with |
* development: v0.38.5 feat: different default grpc ports for different networks (tari-project#4755) fix(core): broken doctests (tari-project#4763) ci: fix coverage job ci: run coverage on prs (tari-project#4738) fix(comms): fixes edge case where online status event does not get published (tari-project#4756) fix(dht/encryption): greatly reduce heap allocations for encrypted messaging (tari-project#4753) docs: explain the emission curve parameters (tari-project#4750) fix(comms/peer_manager): add migration to remove onionv2 addresses (tari-project#4748) fix(ci): add cargo cache, reduce Ubuntu dependencies and action on pull_request (tari-project#4757) feat(tariscript): adds ToRistrettoPoint op-code (tari-project#4749) fix: cli wallet cucumber (tari-project#4739) fix(clients): fix tari nodejs client proto paths (tari-project#4743) chore: disallow onion v2 (tari-project#4745) feat: change priority in mempool to take into account age (tari-project#4737) feat: trigger mempool sync on lag (tari-project#4730) fix(core): use compact inputs for block propagation (tari-project#4714) ci: deny dbg macro (tari-project#4740)
Description
Motivation and Context
Encrypted message handling should be as efficient as possible. The previous implementation performed allocations of the full padded message size twice for encryption and twice for decryption. Increasing memory usage, and negating the performance benefits of using an encryption keystream.
This PR allocates a single buffer for the message to be de/encrypted and de/encrypts the contents in-place using the BytesMut type from the
bytescrate.How Has This Been Tested?
This change is backwards compatible, tested on current esme network and updated existing tests as required.
Discovery: OK
Memorynet: OK
PingPong: OK
InteractiveTransactions: OK
SafTransactions: OK