Skip to content
This repository has been archived by the owner on Oct 23, 2022. It is now read-only.

fix restore_bootstrappers doesn't enable content discovery #406

Merged
merged 16 commits into from
Oct 7, 2020
Merged

fix restore_bootstrappers doesn't enable content discovery #406

merged 16 commits into from
Oct 7, 2020

Conversation

koivunej
Copy link
Collaborator

@koivunej koivunej commented Oct 6, 2020 •
edited
Loading

Fixes #405 and also:

  • makes sure ipfs.restore_bootstrappers is used in the dht_popular_content_discovery test
  • adds a test case making sure we can parse all config::BOOTSTRAP_NODES
  • adds some FIXMEs we couldn't decide yet on the other /bootstrap impl handled
  • expose ipfs::config to allow http to access ipfs::config::BOOTSTRAP_NODES to enable the http api semantics
  • keep exposing the delta semantics in ipfs.restore_bootstrappers
  • use ipfs.restore_bootstrappers in examples/fetch_and_cat.rs via new --default-bootstrappers option/mode
  • add tracing to *_bootstrapper methods

@koivunej koivunej requested a review from ljedrz October 6, 2020 10:48
@koivunej
Copy link
Collaborator Author

koivunej commented Oct 6, 2020

I'll update this when the #400 is merged so that I can add this to changelog but unless the build fails I dont have anything to add.

ljedrz
ljedrz approved these changes Oct 6, 2020
View reviewed changes
Copy link
Member

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM 👌

Joonas Koivunen added 13 commits October 7, 2020 13:45
refactor: reproduce issue with dht_popular_content_discovery
9d2f03e 
using the ipfs.restore_bootstrappers() should probably be the default
way in order to set the default bootstrappers.
fix: add peers when restoring bootstrappers
ce832d9 
fixes rs-ipfs#405 while also changing the return value to be the restored
values (at the moment 0 or 1 multiaddrs).
test: make sure bootstrap nodes are all valid
ddc891d
fix: only attempt to update kademlia for new bootstrap addresses
2b99183
doc: more fixmes
0c3c435
doc: unwrap => expect, explain dropping the peerid
8909700
refactor: remove extra clone (clippy)
27af542
fixup: report restored with multiaddr p2p part
e09f571
feat: make ipfs::config pub to allow access from http
6a06847
fix: return all known bootstrap nodes from http api
968eb3e 
return all instead of returning only the delta, as required by the
interface-http-core tests.
doc: add http api links for bootstrap
297eb45
fix: try removing bootstrappers, add tracing
a317a71 
this might not be a complete solution to removing bootstrap peers, as we
might need to disconnect from them as well but they shouldn't at least
be considered for dht queries anymore.
doc: comment on return value of add/remove bootstrapper
8c0aa1e
@koivunej koivunej changed the title fix restore_bootstrappers not enabling content discovery fix restore_bootstrappers doesn't enable content discovery Oct 7, 2020
@koivunej koivunej marked this pull request as ready for review October 7, 2020 13:23
@aphelionz
Copy link
Contributor

Tested locally, this is great. What do we need to get it out of draft status? :)

Also should we release 0.2.1 after this is merged?

@koivunej
Copy link
Collaborator Author

koivunej commented Oct 7, 2020

Tested manually that these still work ok:

  • cargo test -- --ignored dht_popular_content_discovery -- finds the logo
  • cargo run --example fetch_and_cat -- prints help
  • cargo run --example fetch_and_cat -- PATH -- prompts to connect another node
  • cargo run --example fetch_and_cat -- PATH ADDR -- connects to ADDR, downloads
  • cargo run --example fetch_and_cat -- --default-bootstrappers PATH -- finds content

@koivunej
Copy link
Collaborator Author

koivunej commented Oct 7, 2020

Yeah it's probably best to cut a release soon but maybe wait for #409.

ljedrz
ljedrz reviewed Oct 7, 2020
View reviewed changes
examples/fetch_and_cat.rs Outdated Show resolved Hide resolved
ljedrz
ljedrz reviewed Oct 7, 2020
View reviewed changes
examples/fetch_and_cat.rs Outdated Show resolved Hide resolved
ljedrz
ljedrz reviewed Oct 7, 2020
View reviewed changes
http/src/v0/bootstrap.rs Outdated Show resolved Hide resolved
ljedrz
ljedrz reviewed Oct 7, 2020
View reviewed changes
http/src/v0/bootstrap.rs Outdated Show resolved Hide resolved
ljedrz
ljedrz reviewed Oct 7, 2020
View reviewed changes
http/src/v0/bootstrap.rs Outdated Show resolved Hide resolved
ljedrz
ljedrz reviewed Oct 7, 2020
View reviewed changes
http/src/v0/bootstrap.rs Outdated Show resolved Hide resolved
@koivunej @ljedrz
doc: suggestions from code review
d026fff 
Co-authored-by: ljedrz <ljedrz@users.noreply.github.com>
@koivunej
Copy link
Collaborator Author

koivunej commented Oct 7, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 7, 2020

Build succeeded:

@bors bors bot merged commit bac542a into rs-ipfs:master Oct 7, 2020
@koivunej koivunej mentioned this pull request Oct 12, 2020
Open
@koivunej koivunej deleted the issue_405 branch January 12, 2021 10:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ljedrz ljedrz ljedrz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

calling Ipfs::restore_bootstrappers doesn't end up connecting to any bootstrapper
3 participants
@koivunej @aphelionz @ljedrz