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

refacot(network): simplify the get_record handling code. #1054

Merged
merged 8 commits into from Dec 6, 2023

Conversation

RolandSherwin
Copy link
Member

@RolandSherwin RolandSherwin commented Dec 6, 2023

Description

  • Refactors the get_record re attempt code by moving the target check inside the kad event handling loop
  • This is achieved by moving GetRecordCfg inside the SwarmDriver.
  • Also removes early completion for chunks
    • This code exited early if the RecordHeader::RecordKind is found to be a chunk AND if Quorum::One.
    • But we can mimic the same behavior by just setting the Qourum::One in the main code.
    • Thus, we can safely remove the extra code.
  • Moves all the get record event handling code to its own file.

Summary generated by Reviewpad on 06 Dec 23 08:01 UTC

This pull request includes the following changes:

  • In the bootstrap.rs file, a typo in the comment on line 24 has been corrected, changing "minumn" to "minimum".

  • Changes related to handling errors and inconsistencies in retrieving and storing records from the network have been made. This includes an additional variant called ReturnedRecordDoesNotMatch in the GetRecordError enum, which now takes a Record parameter. The ReturnedRecordDoesNotMatch variant has also been removed from the Error enum. Additionally, the ReturnedRecordDoesNotMatch variant in the GetRecordError enum now includes a more detailed error message.

  • In the sn_networking crate's lib.rs file, the following changes have been made:

    • A new module called get_record_handler has been added.
    • The Quorum and RecordKey imports from the libp2p::kad module have been removed.
    • The RecordHeader and RecordKind imports from the sn_protocol module have been removed.
    • The impl Network block has been updated with changes related to the handling of record retrieval attempts.
  • In the code diff, there are various changes including import statement modifications, code reorganization, removal of unused type definition, and the addition of a new struct named GetRecordCfg. The Debug trait has also been implemented for the GetRecordCfg struct.

  • The file diff includes changes to the SwarmCmd enum and its implementation. The import statement for REPLICATE_RANGE has been removed. The GetNetworkRecord variant of the SwarmCmd enum now includes a cfg field of type GetRecordCfg instead of quorum and expected_holders fields. The Debug implementation for the SwarmCmd enum has been updated accordingly. The impl Debug for SwarmCmd block has been updated to use the {holder:?} syntax for formatting the holder field in the AddKeysToReplicationFetcher variant and the topic_id field in the GossipsubPublish variant. The GetNetworkRecord match arm in the impl SwarmDriver block has been modified to use the get_cfg field instead of the quorum and expected_holders fields.

Please review the specific changes in the diff for more details.

@reviewpad reviewpad bot requested a review from joshuef December 6, 2023 08:02
@reviewpad reviewpad bot added Large Large sized PR waiting-for-review labels Dec 6, 2023
Copy link

reviewpad bot commented Dec 6, 2023

Reviewpad Report

‼️ Errors

  • Unconventional commit detected: 'refactor(network): remove custom early completion for chunks
  • the code exited early if the record is a chunk and the quorum is set
    to One.
  • But the behaviour happens if we just set the quorum to One. No need to
    perform a custom check there.' (006c6ee)
  • Unconventional title detected: 'refacot(network): simplify the get_record handling code.' illegal 'o' character in commit message type: col=05

@RolandSherwin RolandSherwin force-pushed the spend_validation_fix_2 branch 5 times, most recently from 5137f35 to 1a7944e Compare December 6, 2023 09:46
Copy link
Contributor

@joshuef joshuef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, seems a ncie simplification. Lets ensure we need Quorum::All for chunks vs n::2 or ::One for validation there.

@RolandSherwin RolandSherwin added this pull request to the merge queue Dec 6, 2023
Merged via the queue into maidsafe:main with commit 74b37c1 Dec 6, 2023
28 checks passed
@RolandSherwin RolandSherwin deleted the spend_validation_fix_2 branch December 6, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Large Large sized PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants