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

feat(kad): update republish interval and expiration time defaults #3230

Merged
merged 97 commits into from
Mar 29, 2024
Merged

feat(kad): update republish interval and expiration time defaults #3230

merged 97 commits into from
Mar 29, 2024

Conversation

lidel
Copy link
Member

@lidel lidel commented Dec 12, 2022 •
edited by guillaumemichel
Loading

Description

This patch applies changes from libp2p/specs#451. In particular, the new defaults are:

  • Record Expiration: 48h
  • Record Republish Interval: 22h

Closes #3229.

Notes

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@lidel
feat(dht): updated republish and expiration
1ef309e 
Applies changes from libp2p/specs#451

New defaults are:
Record Expiration: 48h
Record Republish Interval: 22h

Closes #3229
@lidel lidel requested a review from mxinden December 12, 2022 18:07
@thomaseizinger thomaseizinger changed the title feat(dht): updated republish and expiration feat(dht): update republish interval and expiration time defaults Dec 13, 2022
mxinden
mxinden reviewed Dec 19, 2022
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

@lidel thank you for tackling this. Very much appreciate you making sure implementations are consistent.

Can you add a changelog entry? Otherwise this is good to go from my end.

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved
@mxinden mxinden changed the title feat(dht): update republish interval and expiration time defaults feat(dht)!: update republish interval and expiration time defaults Dec 19, 2022
@mxinden
Copy link
Member

mxinden commented Dec 19, 2022

@thomaseizinger I added ! to the title, given that this is a breaking change. Let me know in case I misunderstand the conventions of conventional commits.

@thomaseizinger
Copy link
Contributor

@thomaseizinger I added ! to the title, given that this is a breaking change. Let me know in case I misunderstand the conventions of conventional commits.

I guess this is semantically breaking yeah.

@mxinden
Copy link
Member

mxinden commented Jan 19, 2023

@lidel friendly ping. Can you add a changelog entry? Let us know if you want us to take over.

@thomaseizinger thomaseizinger added the need/author-input Needs input from the original author label Jan 19, 2023
@melekes melekes mentioned this pull request Feb 8, 2023
Closed
@github-actions github-actions bot added the Stale label Mar 21, 2023
@github-actions github-actions bot closed this Mar 28, 2023
@thomaseizinger thomaseizinger deleted the feat/update-dht-defaults branch April 26, 2023 16:38
@guillaumemichel guillaumemichel restored the feat/update-dht-defaults branch January 23, 2024 18:53
@guillaumemichel
Copy link
Contributor

My first contribution here (:

Just added the changelog entry, let me know if anything else is required.

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 23, 2024

This pull request has merge conflicts. Could you please resolve them @lidel? 🙏

@guillaumemichel guillaumemichel force-pushed the feat/update-dht-defaults branch from 0b92650 to 1ef309e Compare January 23, 2024 19:32
@github-actions github-actions bot removed the Stale label Jan 24, 2024
@thomaseizinger thomaseizinger added this to the v0.54.0 milestone Jan 24, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 19, 2024

This pull request has merge conflicts. Could you please resolve them @lidel? 🙏

dhuseby and others added 3 commits March 28, 2024 16:22
@dhuseby @guillaumemichel
chore: update config.yml for new issues
f05b752 
Rewording the the choices to clearly direct technical questions to GH dicussions and community discussion to discuss.libp2p.io.

Pull-Request: #5116.
@thomaseizinger @guillaumemichel
chore(webrtc): release version 0.7.1-alpha
b3bdee8 
Resolves: #5117.

Pull-Request: #5118.
@StemCll @guillaumemichel
feat(swarm): allow behaviours to share addresses of peers
a153df9 
Resolves: #4302.

Pull-Request: #4371.
dependabot bot and others added 24 commits March 28, 2024 16:31
@dependabot @guillaumemichel
deps: bump reqwest from 0.11.25 to 0.11.26
8afa8ee 
Pull-Request: #5239.
@jxs @guillaumemichel
chore: address clippy lints and bump ci clippy stable
0aef95c 
superseeds #5248, sorry @drHuangMHT only saw yours now, can you review this one?
https://github.com/libp2p/rust-libp2p/actions/runs/8377129630/job/22938498078?pr=5253#step:6:843 is a bug, see [here](rust-lang/rust-clippy#12377)
Thanks!

Pull-Request: #5253.
@cuishuang @guillaumemichel
docs: fix typos in some comments
b8b157a 
The same as #5196

There are some problem when merging #5196, resubmit this mr.

Pull-Request: #5255.
@sisou @guillaumemichel
fix(websocket-websys): change close code in drop implementation to 1000
21ffd2f 
In browsers, only the code 1000 and codes between 3000-4999 are allowed to be set by userland code: https://websockets.spec.whatwg.org/#dom-websocket-close

I found this while debugging use-after-free errors in our WASM application. Turns out, when connections are timing out, libp2p in rust drops them, but does not seem to close them explicitly (we are using libp2p-swarm). This led to these WebSockets still emitting events, the handlers of which were already dropped on the rust side, though.

A long investigation led me to have a look into the `Result` that gets returned from `close_with_code_and_reason`, and it turns out it's an error! Specifically:
```
InvalidAccessError: Failed to execute 'close' on 'WebSocket': The code must be either 1000, or between 3000 and 4999. 1001 is neither.
```

This PR only fixes the failing closing of the WebSocket, not the remaining use-after-free problem.

Pull-Request: #5229.
@jxs @guillaumemichel
chore: re-enable mergify for dependabot & trivial (#5264)
d6ed135 
## Description
mergify automatic approval for dependabot PR's was disabled on
#5208 following
malfunctioning.
This PR re-enables mergify's automatic merge of Dependabot PR's and the
`trivial` label with the syntax [suggested on mergify's
doc](https://docs.mergify.com/integrations/dependabot/#2-pr-approval)
@dependabot @guillaumemichel
deps: bump bs58 from 0.5.0 to 0.5.1
6799b3f 
Pull-Request: #5257.
@dependabot @guillaumemichel
deps: bump reqwest from 0.11.26 to 0.11.27
c229387 
Pull-Request: #5258.
@dependabot @guillaumemichel
deps: bump regex from 1.10.3 to 1.10.4
a2a5d4b 
Pull-Request: #5261.
@dependabot @guillaumemichel
deps: bump bytes from 1.5.0 to 1.6.0
49b1765 
Pull-Request: #5262.
@dependabot @guillaumemichel
deps: bump smallvec from 1.13.1 to 1.13.2
31d1aa6 
Pull-Request: #5263.
@dependabot @guillaumemichel
deps: bump the axum group with 1 update
3af69be 
Pull-Request: #5256.
@dependabot @guillaumemichel
deps: bump proc-macro2 from 1.0.78 to 1.0.79
ec2d329 
Pull-Request: #5234.
@dependabot @guillaumemichel
deps: bump the opentelemetry group with 4 updates
d7bcc0b 
Pull-Request: #5259.
@dependabot @guillaumemichel
deps: bump thiserror from 1.0.57 to 1.0.58
ff93fa7 
Pull-Request: #5237.
@dependabot @guillaumemichel
deps: bump async-trait from 0.1.77 to 0.1.78
143fac4 
Pull-Request: #5235.
@dependabot @guillaumemichel
deps: bump syn from 2.0.52 to 2.0.55
505a194 
Pull-Request: #5265.
@dependabot @guillaumemichel
deps: bump anyhow from 1.0.80 to 1.0.81
5bccabe 
Pull-Request: #5236.
@dependabot @guillaumemichel
deps: bump trybuild from 1.0.89 to 1.0.90
625ec7c 
Pull-Request: #5233.
@dependabot @guillaumemichel
deps: bump pin-project from 1.1.4 to 1.1.5
29c4200 
Pull-Request: #5216.
@junekhan @guillaumemichel
fix(kad): compute jobs_query_capacity accurately
e5346ec 
Fixes: #4191.

Pull-Request: #5148.
@guillaumemichel
feat(kad): FIND_NODE for own peer id returns closest peers
f792b58 
Fixes #5269.

Pull-Request: #5270.
@guillaumemichel
added changelog entry
76fc6f8
@guillaumemichel
rebase
90824fa
@guillaumemichel
Merge branch 'master' into feat/update-dht-defaults
190ed48
@jxs jxs added the send-it label Mar 29, 2024
@mergify mergify bot merged commit 9805339 into master Mar 29, 2024
71 of 72 checks passed
@mergify mergify bot deleted the feat/update-dht-defaults branch March 29, 2024 11:34
@jxs jxs changed the title feat(dht)!: update republish interval and expiration time defaults feat(kad): update republish interval and expiration time defaults Mar 29, 2024
@CHr15F0x CHr15F0x mentioned this pull request Aug 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guillaumemichel guillaumemichel guillaumemichel left review comments

@mxinden mxinden mxinden left review comments

@thomaseizinger thomaseizinger thomaseizinger approved these changes

@melekes melekes melekes left review comments

@jxs jxs jxs approved these changes

Assignees
No one assigned
Labels
need/author-input Needs input from the original author send-it
Projects
None yet
Milestone
v0.54.0
Development

Successfully merging this pull request may close these issues.

kad: Update republish and expiration interval to new spec