Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(network): reject large records before sending out to network #662

Merged
merged 1 commit into from Aug 22, 2023

Conversation

RolandSherwin
Copy link
Member

@RolandSherwin RolandSherwin commented Aug 22, 2023

  • If a record is larger than the max kademlia packet size, it was previously rejected without any error. So send out an error on such cases.
  • Increase the max kad packet size from 1MB -> 2MB. The chunk size is 1mb, so this should prevent any potential failures in the current network.

Summary generated by Reviewpad on 22 Aug 23 16:19 UTC

This pull request includes several changes to the network module.

  • The patch fixes a bug where large records were being sent out to the network. Now, records larger than a specified size will be rejected before sending.
  • The patch includes changes to improve logging and error handling when sending records to the network.
  • The patch adds a constant MAX_PACKET_SIZE to define the largest packet size to send over the network. Records larger than this size will be rejected.
  • The patch also includes some code cleanup and error handling improvements in the DiskBackedRecordStore module.

Overall, these changes aim to improve the robustness and reliability of the network module by rejecting large records and handling errors more effectively.

@reviewpad reviewpad bot requested a review from joshuef August 22, 2023 16:19
@reviewpad reviewpad bot added Small Pull request is small waiting-for-review labels Aug 22, 2023
@@ -304,7 +309,7 @@
}

let store_cfg = DiskBackedRecordStoreConfig {
max_value_bytes: 1024 * 1024,
max_value_bytes: MAX_PACKET_SIZE, // TODO, does this need to be _less_ than MAX_PACKET_SIZE

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
sn_networking/src/lib.rs Fixed Show fixed Hide fixed
sn_networking/src/lib.rs Fixed Show fixed Hide fixed
@@ -68,6 +68,11 @@
/// The peer should be present among the CLOSE_GROUP_SIZE if we're fetching the close_group(peer)
pub const CLOSE_GROUP_SIZE: usize = 8;

/// What is the largest packet to send over the network.
/// Records larger than this will be rejected.
// TODO: revisit once utxo is in

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@RolandSherwin RolandSherwin added this pull request to the merge queue Aug 22, 2023
Merged via the queue into maidsafe:main with commit 2308b58 Aug 22, 2023
23 checks passed
@RolandSherwin RolandSherwin deleted the set_max_packet_size branch August 22, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Small Pull request is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants