kad: Expose all peer records of `GET_VALUE` query #96

lexnv · 2024-04-29T10:08:38Z

This PR extends the GetRecordSucess variant of kad queries to provide all the query information.
Previously, only the last stored query record was returned to the user.

In the libp2p crate, an event is generated for every found record. However, litep2p responds only when the query finishes.
Records are extended to offer similar information back to the user while sending a minimum number of events.

A new RecordsType is introduced to avoid confusion about records extracted from the local store and records found on the network
We save the record backed by the most number of peers in our local store
A panic is removed from the kad/query since we now operate with a multitude of records and potentially none
Expired records are now ignored

cc @paritytech/networking

Signed-off-by: Alexandru Vasile <[email protected]>

alexggh · 2024-04-30T12:42:01Z

src/protocol/libp2p/kademlia/mod.rs

@@ -52,6 +52,8 @@ pub use handle::{KademliaEvent, KademliaHandle, Quorum, RoutingTableUpdateMode};
 pub use query::QueryId;
 pub use record::{Key as RecordKey, PeerRecord, Record};

+use self::handle::RecordsType;


pub, so we can use this type to deconstruct it, or maybe we can try to keep the libp2p semantics for PeerRecord and we don't need this extra type:

/// A record either received by the given peer or retrieved from the local /// record store. #[derive(Debug, Clone, PartialEq, Eq)] pub struct PeerRecord { /// The peer from whom the record was received. `None` if the record was /// retrieved from local storage. pub peer: Option<PeerId>, pub record: Record, }

Signed-off-by: Alexandru Vasile <[email protected]>

Signed-off-by: Alexandru Gheorghe <[email protected]>

This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: #4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: #3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: #4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>

This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: paritytech#4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: paritytech#3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: paritytech#4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>

dmitry-markin

LGTM!

dmitry-markin · 2024-05-09T13:52:01Z

src/protocol/libp2p/kademlia/mod.rs

+                // Considering this gives a view of all peers and their records, some peers may have
+                // outdated records. Store only the record which is backed by most
+                // peers.
+                let rec = records
+                    .iter()
+                    .map(|peer_record| &peer_record.record)
+                    .fold(HashMap::new(), |mut acc, rec| {
+                        *acc.entry(rec).or_insert(0) += 1;
+                        acc
+                    })
+                    .into_iter()
+                    .max_by_key(|(_, v)| *v)
+                    .map(|(k, _)| k);


Would be nice to have tests for correctly updating the local store and also discarding the expired records. At least as a follow-up planned / TODO issue if it's too much work to implement straight away.

This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: paritytech#4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: paritytech#3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: paritytech#4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv · 2024-05-16T11:07:44Z

Thanks @dmitry-markin for the review 🙏

I've added to tests to validate the kademlia behavior in this case:

one test to see that the record backed by most peers is added to the local storage
one test to ensure that expired records don't count towards the records frequency

Signed-off-by: Alexandru Vasile <[email protected]>

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 --------- Signed-off-by: Alexandru Vasile <[email protected]>

## [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]>

This PR updates the litep2p crate to the latest version. This fixes the build for developers that want to perform `cargo update` on all their dependencies: paritytech#4343, by porting the latest changes. The peer records were introduced to litep2p to be able to distinguish and update peers with outdated records. It is going to be properly used in substrate via: paritytech#3786, however that is pending the commit to merge on litep2p master: paritytech/litep2p#96. Closes: paritytech#4343 --------- Signed-off-by: Alexandru Vasile <[email protected]>

lexnv added 5 commits April 29, 2024 12:27

kad: Extend records event with multiple peers and different records

4d089ab

Signed-off-by: Alexandru Vasile <[email protected]>

kad: Return HashMap<record, vec<peers>>

4257819

Signed-off-by: Alexandru Vasile <[email protected]>

kad/query: Do not propagate expired records

3f210b1

Signed-off-by: Alexandru Vasile <[email protected]>

kad/tests: Check all peers return the same record

a1b3f10

Signed-off-by: Alexandru Vasile <[email protected]>

kad: Return vec of records instead

1143636

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv changed the title ~~kad: Expose all peer records of FIND_VALUE query~~ kad: Expose all peer records of GET_VALUE query Apr 29, 2024

lexnv self-assigned this Apr 29, 2024

lexnv mentioned this pull request Apr 29, 2024

kad: Refactor GetRecord query and add tests #97

Merged

alexggh reviewed Apr 30, 2024

View reviewed changes

kad: Export RecordsType as pub

b6fb4c0

Signed-off-by: Alexandru Vasile <[email protected]>

alexggh added a commit to paritytech/polkadot-sdk that referenced this pull request Apr 30, 2024

Integrate with paritytech/litep2p#96

fd5dc46

Signed-off-by: Alexandru Gheorghe <[email protected]>

alexggh mentioned this pull request May 1, 2024

authorithy-discovery: Make changing of peer-id while active a bit more robust paritytech/polkadot-sdk#3786

Merged

4 tasks

lexnv mentioned this pull request May 1, 2024

cargo: Update experimental litep2p to latest version paritytech/polkadot-sdk#4344

Merged

dmitry-markin approved these changes May 9, 2024

View reviewed changes

lexnv added 3 commits May 16, 2024 13:58

kad/tests: Check get records update local storage by record frequency

7857710

Signed-off-by: Alexandru Vasile <[email protected]>

kad: Ignore expired records

55476e2

Signed-off-by: Alexandru Vasile <[email protected]>

kad/tests: Check get records update with expired records

11a3e45

Signed-off-by: Alexandru Vasile <[email protected]>

kad/tests: Rename _make_kad to make_kad

d5b8d38

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv merged commit 423a35d into master May 16, 2024
8 checks passed

lexnv deleted the lenxv/expose-peer-records branch May 16, 2024 11:16

lexnv mentioned this pull request May 24, 2024

chore: Release v0.5.0 #118

Merged

lexnv mentioned this pull request Oct 16, 2024

[litep2p-milestone]: Improve litep2p robustness #140

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kad: Expose all peer records of `GET_VALUE` query #96

kad: Expose all peer records of `GET_VALUE` query #96

lexnv commented Apr 29, 2024 •

edited

Loading

alexggh Apr 30, 2024

dmitry-markin left a comment

dmitry-markin May 9, 2024 •

edited

Loading

lexnv commented May 16, 2024

kad: Expose all peer records of GET_VALUE query #96

kad: Expose all peer records of GET_VALUE query #96

Conversation

lexnv commented Apr 29, 2024 • edited Loading

alexggh Apr 30, 2024

Choose a reason for hiding this comment

dmitry-markin left a comment

Choose a reason for hiding this comment

dmitry-markin May 9, 2024 • edited Loading

Choose a reason for hiding this comment

lexnv commented May 16, 2024

kad: Expose all peer records of `GET_VALUE` query #96

kad: Expose all peer records of `GET_VALUE` query #96

lexnv commented Apr 29, 2024 •

edited

Loading

dmitry-markin May 9, 2024 •

edited

Loading