-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Improve peer hints for pin remote add #8143
Conversation
This comment has been minimized.
This comment has been minimized.
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.
lgtm, but we should remove NoAnnounce
filtering, as it is redundant (details below)
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.
LGTM, @aschmahmann ready for your final review.
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? |
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.
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()) |
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.
@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.
Closes #7915. When running
ipfs pin remote add <cid>
,Pin.origins = []
If address is loopback, remove address fromPin.origins
If address is inNoAnnounce
, remove address fromPin.origins