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.

Sign up for GitHub

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

Jump to bottom

kad: Refactor GetRecord query and add tests #97

Merged
merged 19 commits into from
May 16, 2024
Merged

kad: Refactor GetRecord query and add tests #97

merged 19 commits into from
May 16, 2024

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Apr 29, 2024 •
edited
Loading

This PR refactors the GetRecord query to make it more robust.

  • Avoid panics on unwraps and unimplemented logic
  • Separate config immutable variables from main query logic
  • Simplifies logic of next_action method
  • Private methods for internal logic

Builds on top of: #96

cc @paritytech/networking

lexnv added 10 commits April 29, 2024 12:27
@lexnv
kad: Extend records event with multiple peers and different records
4d089ab 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad: Return HashMap<record, vec<peers>>
4257819 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad/query: Do not propagate expired records
3f210b1 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad/tests: Check all peers return the same record
a1b3f10 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad: Return vec of records instead
1143636 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad/query: Extract immutable components to GetRecordConfig
8efdf89 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad/get_record: Avoid panics
72f7fbc 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad/get_record: Refactor query candidates
23fdf7d 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad/get_record: Clean-up next action state and avoid panics
136b59e 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv
kad/get_record: Cache kad msg to avoid encoding multiple times
d232828 
Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv self-assigned this Apr 29, 2024
@lexnv lexnv changed the title kad: Refactor GetRecord query kad: Refactor GetRecord query and add tests Apr 30, 2024
@lexnv lexnv mentioned this pull request May 6, 2024
Closed
dmitry-markin
dmitry-markin approved these changes May 10, 2024
View reviewed changes
Copy link
Collaborator

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

LGTM!

src/protocol/libp2p/kademlia/query/get_record.rs Outdated Show resolved Hide resolved
src/protocol/libp2p/kademlia/query/get_record.rs Outdated Show resolved Hide resolved
Base automatically changed from lenxv/expose-peer-records to master May 16, 2024 11:16
lexnv added 2 commits May 16, 2024 14:21
@lexnv lexnv merged commit 5548223 into master May 16, 2024
8 checks passed
@lexnv lexnv deleted the lexnv/refactor-get-records branch May 16, 2024 11:50
@lexnv lexnv mentioned this pull request May 24, 2024
Merged
lexnv added a commit that referenced this pull request May 24, 2024
@lexnv
chore: Release v0.5.0 (#118)
0364bf5 
## [0.5.0] - 2023-05-24

This is a small patch release that makes the `FindNode` command a bit
more robst:

- The `FindNode` command now retains the K (replication factor) best
results.
- The `FindNode` command has been updated to handle errors and
unexpected states without panicking.

### Changed

- kad: Refactor FindNode query, keep K best results and add tests
([#114](#114))

## [0.4.0] - 2023-05-23

This release introduces breaking changes to the litep2p crate, primarily
affecting the `kad` module. Key updates include:

- The `GetRecord` command now exposes all peer records, not just the
latest one.
- A new `RecordType` has been introduced to clearly distinguish between
locally stored records and those discovered from the network.

Significant refactoring has been done to enhance the efficiency and
accuracy of the `kad` module. The updates are as follows:

- The `GetRecord` command now exposes all peer records.
- The `GetRecord` command has been updated to handle errors and
unexpected states without panicking.

Additionally, we've improved code coverage in the `kad` module by adding
more tests.

### Added

- Add release checklist
([#115](#115))
- Re-export `multihash` & `multiaddr` types
([#79](#79))
- kad: Expose all peer records of `GET_VALUE` query
([#96](#96))

### Changed

- multistream_select: Remove unneeded changelog.md
([#116](#116))
- kad: Refactor `GetRecord` query and add tests
([#97](#97))
- kad/store: Set memory-store on an incoming record for PutRecordTo
([#88](#88))
- multistream: Dialer deny multiple /multistream/1.0.0 headers
([#61](#61))
- kad: Limit MemoryStore entries
([#78](#78))
- Refactor WebRTC code
([#51](#51))
- Revert "Bring `rustfmt.toml` in sync with polkadot-sdk (#71)"
([#74](#74))
- cargo: Update str0m from 0.4.1 to 0.5.1
([#95](#95))

### Fixed

- Fix clippy  ([#83](#83))
- crypto: Don't panic on unsupported key types
([#84](#84))

---------

Signed-off-by: Alexandru Vasile <[email protected]>
@lexnv lexnv mentioned this pull request Oct 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-markin dmitry-markin dmitry-markin approved these changes

Assignees

@lexnv lexnv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lexnv @dmitry-markin