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

feat(iroh-net)!: remove fs based peers storage #2510

Merged
merged 10 commits into from
Jul 22, 2024

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Jul 16, 2024

Description

Instead introduces

  • endpoint::Builder::known_nodes to provide existing NodeAddrs
  • you can use endpoint::Endpoint::connection_infos to retrieve the current list of known nodes that can be persisted

Notes

  • prune_inactive is not called anymore when storing, only on the heartbeat, and so there is a chance of storing uninteresting addresses
  • the logic of filtering nodes has changed a little bit, to simplify the code paths

Breaking Changes

  • endpoint::Builder::peers_path is removed
  • no automatic storage of known peers in iroh-net

TODO

  • Recreate automatic storage and loading of peers inside of iroh

iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

In principle I like this.

The important part is getting back the saving functionality that used to be in iroh-net.
Ideally we bring back the same "save on tick" functionality where the peer map is saved.

Perhaps we can even save it in redb instead? That could be nicer for something that may be a little more corruption resistant.

iroh-base/src/node_addr.rs Outdated Show resolved Hide resolved
@@ -771,6 +767,11 @@ impl Endpoint {
self.msock.conn_type_stream(node_id)
}

/// Get the known node addresses which should be persisted.
pub fn node_addresses_for_storage(&self) -> Vec<NodeAddr> {
self.msock.node_addresses_for_storage()
Copy link
Contributor

Choose a reason for hiding this comment

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

This information can already be constructed from the Endpoint::connection_infos response. Maybe it makes more sense to add a From<ConnectionInfo> for NodeAddr impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, done

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the same. The previous code saved only the verified paths of used nodes and all paths of unverified nodes. Now, as far as i can tell, we will store again paths not active/used for nodes with known verified paths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@divagant-martian should be fixed, the filtering is happening now at the node layer

@flub
Copy link
Contributor

flub commented Jul 16, 2024

The important part is getting back the saving functionality that used to be in iroh-net. Ideally we bring back the same "save on tick" functionality where the peer map is saved.

Perhaps we can even save it in redb instead? That could be nicer for something that may be a little more corruption resistant.

This sounds like Endpoint::connection_infos (or something else) should return a stream, yielding a new item every time the internal node map is updated.

@dignifiedquire
Copy link
Contributor Author

I implemented a timer based version of storage again, which I think works well enough for now

@dignifiedquire dignifiedquire marked this pull request as ready for review July 16, 2024 12:27
iroh/src/node.rs Outdated Show resolved Hide resolved
@flub
Copy link
Contributor

flub commented Jul 16, 2024

lgtm i think

Instead introduces

- `endpoint::Builder::known_nodes` to provide existing `NodeAddr`s
- `endpoint::Endpoint::node_addresses_for_storage` to retrieve the current list of known nodes that can be persisted
Copy link
Contributor

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

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

👍 lgtm

@@ -1478,6 +1465,8 @@ pub struct DirectAddrInfo {
pub last_control: Option<(Duration, ControlMsg)>,
/// How long ago was the last payload message for this node.
pub last_payload: Option<Duration>,
/// When was this connection last alive, if ever.
pub last_alive: Option<Duration>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be redundant, tldr this is the max between the two above. But it's harmless anyway

@matheus23 matheus23 self-requested a review July 22, 2024 15:21
@dignifiedquire dignifiedquire added this pull request to the merge queue Jul 22, 2024
Merged via the queue into main with commit 0a8cb8a Jul 22, 2024
25 of 26 checks passed
@dignifiedquire dignifiedquire deleted the feat-move-node-storage branch November 28, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants