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

Improve peer hints for pin remote add #8143

Merged
merged 4 commits into from
Jun 25, 2021
Merged

Improve peer hints for pin remote add #8143

merged 4 commits into from
Jun 25, 2021

Conversation

brianstrauch
Copy link
Contributor

@brianstrauch brianstrauch commented May 17, 2021

Closes #7915. When running ipfs pin remote add <cid>,

  1. If CID isn't in blockstore, local node isn't origin, so send Pin.origins = []
  2. If address is loopback, remove address from Pin.origins
  3. If address is in NoAnnounce, remove address from Pin.origins

@welcome

This comment has been minimized.

@BigLep BigLep requested a review from lidel May 17, 2021 15:59
@BigLep BigLep added the P3 Low: Not priority right now label May 17, 2021
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

lgtm, but we should remove NoAnnounce filtering, as it is redundant (details below)

core/commands/pin/remotepin.go Outdated Show resolved Hide resolved
@lidel lidel added the need/author-input Needs input from the original author label May 18, 2021
@brianstrauch brianstrauch requested a review from lidel May 18, 2021 03:55
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM, @aschmahmann ready for your final review.

@lidel lidel requested a review from aschmahmann May 18, 2021 13:20
@aschmahmann aschmahmann self-assigned this May 24, 2021
@BigLep BigLep removed the need/author-input Needs input from the original author label May 24, 2021
@lidel
Copy link
Member

lidel commented Jun 7, 2021

Note with feedback we got during today's triage:

Ok, so (3) is not necessary (already filtered out), (2) is a bit problematic, because in some contexts, those multiaddrs could be useful.

This means to make this low-risk, low-controversy PR we shoul limit its behavior to (1).

@brianstrauch will you have a moment to refactor it to only do (1) and no multiaddr filtering?

@brianstrauch brianstrauch requested a review from lidel June 7, 2021 17:43
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @brianstrauch!

@aschmahmann ready for your final review.

node, err := cmdenv.GetNode(env)
if err != nil {
return err
}
if node.PeerHost != nil {

isInBlockstore, err := node.Blockstore.Has(rp.Cid())
Copy link
Contributor

Choose a reason for hiding this comment

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

@lidel this seems fine. I don't think we're introducing any new problems on the CoreAPI/Node boundary, but wanted to double check. WDYT?

The boundary issues we generally run into are that IIRC accessing the node directly doesn't respect the --offline flag, however I think we may already have that issue given that we use node.PeerHost below.

@aschmahmann aschmahmann merged commit 5ad0417 into ipfs:master Jun 25, 2021
@brianstrauch brianstrauch deleted the fix/pin-remote-add-origin-hints branch June 25, 2021 15:51
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Low: Not priority right now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pin remote add: improve Peer hints in Pin.origins
4 participants