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
New values for DHT Provider Record Republish and Expiration (22h/48h, RFM17) #451
New values for DHT Provider Record Republish and Expiration (22h/48h, RFM17) #451
Changes from 1 commit
b5fd7cc
9f7f275
7f64613
094094d
2318d76
9464d50
3ba82bb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Perhaps this has already been considered, but given this is an implicit protocol change it might help implementers to know what they should use as the republish interval. 24hrs matches the current expiration time so reproviding every 24hrs while the network is (slowly) upgrading might not be great. Perhaps it's fine in networks like the IPFS Public DHT since some nodes will upgrade quickly (e.g. Hydras, people autodeploying the latest kubo Docker containers, etc.) but just wanted to flag this.
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.
Right, so you suggest not having the new republish interval the same as the old expiration interval as this will become confusing and might have side effects as well? I guess a valid workaround is to have the republish interval set to something a bit smaller (say, 20hrs?) for the transition period? Any better approaches?
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.
I've set this to 22hrs. @mxinden @aschmahmann let me know if that works for you.
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.
This should be harmonized with the PUT_VALUE expiration time as well, 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.
@yiannisbot while we're changing the times any reason not to do this too? Seems like it'd be reasonable since expiration is based on the same properties. It'd also make things easier to reason about.
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.
You mean in the PR? Yes, the intention is to change both the republish and the expiration interval. Otherwise it would make no sense (or well, it would be confusing). Or do you mean something else?
cc: @cortze who is working to submit the relevant PR.
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.
I agree with changing them together. However, unless I am missing something in the code, they will have to be two separate PRs:
kubo
's defaultReprovider.Interval
herego-libp2p-kad-dht
'sProvideValidity
hereThere 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.
Linking here the PR libp2p/go-libp2p-kad-dht#793 to increase the expiration time of the PRs to
48h
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.
No, I mean to change the expiration time for
PUT_VALUE
records in addition toADD_PROVIDER
records.i.e. for the IPFS Public DHT the expiration time for
PUT_VALUE
(i.e. IPNS and the deprecated public key records) is 36hrshttps://github.com/libp2p/go-libp2p-kad-dht/blob/dae5a9a5bd9c7cc8cfb5073c711bc308efad0ada/internal/config/config.go#L117. It seems like this could be 48hrs as well rather than 36.
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.
@cortze should we change that too then? It makes sense. Do you want to create the PR and set it to 48hrs?
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.
Just created it! here is the link to the PR -> go-libp2p-kad-dht#794