Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(peer-store): introduce libp2p-peer-store #5724
base: master
Are you sure you want to change the base?
feat(peer-store): introduce libp2p-peer-store #5724
Changes from 5 commits
1affefe
8d4930a
9cb59c3
fbc1344
2bcfa7f
6df6ee5
99a6bfd
5c7ba32
b6dcd59
cbb1906
6270259
cc91ab5
f9b040e
a9597da
0e6b280
fe14c42
92ac2fb
913ed1f
b8a7114
5f628fb
121e91b
47048ec
9da96ff
9945fd0
cb294c6
a9dc9aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make more sense to remove this for
FromSwarm
events since a connection could be denied later on (ie connection limits, banned peer, etc.), so that way the store isnt exactly cluttered.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Will favor the swarm event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we instead define Store::handle_* methods that are called here and in the other
NetworkBehaviour::handle_*
so that it allows us to further manage our peers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are those specifically? The store no longer record connected peers.
Check failure on line 14 in misc/peer-store/src/lib.rs
GitHub Actions / clippy (beta)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are tracking peer connections too, should we also keep tabs on the
ConnectionId
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but how to store it? I think there can be multiple connections from a single peer, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
update
gives the impression that a peer just has one address, and that this address gets updated here.Wdyt of instead of calling it instead
on_new_address
or something like that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because for now I only see one address pop up at a time. I was planning to use a boxed slice but you know there would be a heap allocation, which isn't necessary for only one element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we can't do a batch update so there will be a iterator anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, maybe I wasn't clear. I was just nitpicking on the name of the function, not the
address: &Multiaddr
parameter :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, I'm not so sure about the naming, because the address isn't necessarily a new address. If the address is not new, it is updated due to LRU rules, like
touch
, so I can't quite make the decision.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdyt of
on_address_discovered
then?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discovered also kind of suggests the address is new? I'm not really convinced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where using a
HashSet<PeerId>
to track connected peers is unsuitable for a specific use case? If now, how about moving this into theBehavior
, so that theStore
only concerns the "address-book" part of this behavior?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of
Store
trait is to allow on-disk storage, now I think about it, this info will be changing in real time so it should be kept in memory anyway. Will move it into the behaviour itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have
MemoryStore<T=()>
so that we are able to store data for peers (like scoring etc):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MemoryStore
is more of a reference implementation, I don't think it is necessary to include a generic parameter for customization since we are maintaining its internals.