feat: Addresses.AppendAnnounce #8177

Jorropo · 2021-06-06T01:27:35Z

Fixes: #7791
This needs: ipfs/go-ipfs-config#135 too (and the replace rule to be removed)

lidel

Thank you @Jorropo for tackling this!

I spent some time trying to empathize with a novice user and think that an explicit list feels better than a boolean flag.

Compare below example:

  "Announce":       [],
  "AppendAnnounce": ["/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"],
  "NoAnnounce":     [],

  "Announce":       ["/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"],
  "AppendAnnounce": [],
  "NoAnnounce":     [],

vs

  "Announce":       ["/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"],
  "AppendAnnounce": false,
  "NoAnnounce":     [],

  "Announce":       ["/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"],
  "AppendAnnounce": true,
  "NoAnnounce":     [],

Second one is really confusing without reading the docs.
Easy to imagine user thinking "is Annouce setting disabled?" "Do I need to flip it to true?"

I suggest switching to an explicit string[], as it is both less ambiguous and takes less space visually when empty.

(cc @aschmahmann @Stebalien in case they feel otherwise)

Jorropo · 2021-06-26T23:59:50Z

@lidel I agree that my current proposal is bad.
But I don't like yours either, imagining myself as I noob I think : "wtf why is there 2 Announce ?"

Your solution doesn't make obvious that adding anything to Announce will remove auto detected addresses.
This could be solved by renaming Announce to OverwritingAnnounce but I have a better solution in mind.

Instead create a bool flag AnnounceAutodetectedAddresses, defaults to true, if true then append announces and auto detected addresses. (this is pretty much the same PR but with a rename)

lidel · 2021-07-06T14:13:43Z

I like the idea of self-describing flag AnnounceAutodetectedAddresses, pretty intuitive.

The only concern I have is that if we set it to true by default, then we effectively change the behavior of Announce for people who are already using it and expect it to only announce explicitly stated entries.

@aschmahmann do we have ability to do config migrations as part of usual repo migrations? If so, we could do one time 9-to-10 migration where AnnounceAutodetectedAddresses is set to false if Announce is not empty.

TheDiscordian · 2021-07-08T19:50:58Z

Compare below example:

  "Announce":       [],
  "AppendAnnounce": ["/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"],
  "NoAnnounce":     [],

  "Announce":       ["/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ"],
  "AppendAnnounce": [],
  "NoAnnounce":     [],

I love this, very explicit and intuitive. Personally I think if the empty fields are included in the config, it will be quite obvious what these do (though tbh it'd be nice if we could add comments to the config).

Instead create a bool flag AnnounceAutodetectedAddresses, defaults to true, if true then append announces and auto detected addresses. (this is pretty much the same PR but with a rename)

I like this idea in spirit, but idk how I feel about changing the behaviour of Announce, I feel like that might introduce more confusion than the other proposal, and I'm not sure on how much benefit it'd bring.

aschmahmann · 2021-07-08T20:19:11Z

@aschmahmann do we have ability to do config migrations as part of usual repo migrations? If so, we could do one time 9-to-10 migration where AnnounceAutodetectedAddresses is set to false if Announce is not empty.

We can do config migrations as part of a repo migration (or even just have two back-to-back migrations). Writing migrations should be much easier given the recent refactoring, but there are still tradeoffs around deployment/when we do the migrations.

I don't have particularly strong feelings here about whether it's more intuitive for users if we change the option names to make them clearer (and make users relearn the names) vs just adding fields.

lidel

@Jorropo we discussed this during triage and:

Changing the default behavior of Announce array is not acceptable: too many unknowns.
I could introduce silent bugs in automated deployments where empty repo is initialized using some predefined template etc. But there are most likely unknown unknowns as well.
Without changing the default behavior of Annouce proposed config is more confusing than separate array.
Due to this, we can accept this PR if it is refactored to add separate AppendAnnounce array (as suggested in my original comment).
This is the best way, as it is additive and easy to document.

Jorropo · 2021-07-19T02:21:14Z

@lidel I still think a bool flag is clearer but I get the point of automated systems.
I've made the changes as in the end having the option is what matter, not really how it's done.

I'll let to the one merging the PR cleaning up the replace rule in the go.mod (you have push access on that PR) when ipfs/go-ipfs-config#135 is merged.

texfu

Thanks

Closes ipfs#7791

- nat.go needs to use OptionalDuration - libp2p config no longer use relay v1 setting

lidel

Thank you @Jorropo and apologies for keeping this out in cold for so long.
Did a quick pass and finished remaining work and checks:

rebased
docs
tests

@aschmahmann fysa this LGTM, but needs two things before merge:

go-ipfs-config with feat: add Addresses.AppendAnnounce go-ipfs-config#135
WIP: update go-libp2p to v0.16.0 #8522 to land first, to avoid the headache of dealing with go-ipfs-config changes.

lidel · 2021-11-15T14:05:12Z

core/node/groups.go

 		fx.Provide(libp2p.SmuxTransport(cfg.Swarm.Transports)),
-		fx.Provide(libp2p.Relay(enableRelay, cfg.Swarm.EnableRelayHop)),


ℹ️ this is ok because EnableRelayHop was removed in ipfs/go-ipfs-config#146 and won't be used when #8522 lands

lidel · 2021-11-23T21:03:12Z

Waiting for #8563 to land first.

core/node/libp2p/addrs.go

aschmahmann · 2021-11-30T14:16:26Z

core/node/libp2p/addrs.go

+
+	appendAnnAddrs := make([]ma.Multiaddr, len(appendAnnouce))
+	for i, addr := range appendAnnouce {
+		appendAnnAddrs[i], err = ma.NewMultiaddr(addr)


What's the intended behavior if both announce and appendannounce are set? Do we just need to quietly ensure that we don't duplicate addresses, but that otherwise it's fine to set both fields?

I believe deduplicating should be enough (if libp2p does not do that already?)

My mental model is:

Announce is "ignore infered ones, use these"

AppendAnnounce is "add these to the final list, no matter if static or inferred"

I think the same thing as @lidel

Added deduplication in 9673571

ipfs#8177 (comment) ipfs#8177 (comment)

lidel · 2021-11-30T16:58:47Z

@Jorropo it is happening 🥳

lidel requested changes Jun 9, 2021

View reviewed changes

lidel mentioned this pull request Jul 6, 2021

Append Announces #7791

Closed

lidel requested changes Jul 9, 2021

View reviewed changes

aschmahmann added the need/author-input Needs input from the original author label Jul 16, 2021

Jorropo force-pushed the feat/7791 branch from deb796e to cadd32f Compare July 19, 2021 01:59

Jorropo requested a review from lidel July 19, 2021 02:18

texfu approved these changes Jul 19, 2021

View reviewed changes

aschmahmann added need/maintainer-input Needs input from the current maintainer(s) and removed need/author-input Needs input from the original author labels Jul 30, 2021

Jorropo mentioned this pull request Aug 31, 2021

Release v0.10 #8176

Closed

62 tasks

Jorropo and others added 2 commits November 15, 2021 14:56

feat: append annouces

3aafda4

Closes ipfs#7791

refactor: go-ipfs-config

aec3c20

- nat.go needs to use OptionalDuration - libp2p config no longer use relay v1 setting

lidel force-pushed the feat/7791 branch from cadd32f to aec3c20 Compare November 15, 2021 14:04

lidel added 2 commits November 15, 2021 15:10

docs: Addresses.AppendAnnounce

8d6a5e5

test: Addresses.AppendAnnounce

aacf2fc

lidel approved these changes Nov 15, 2021

View reviewed changes

aschmahmann added the status/blocked Unable to be worked further until needs are met label Nov 19, 2021

lidel mentioned this pull request Nov 23, 2021

Release v0.11 #8343

Closed

80 tasks

lidel changed the title ~~feat: append annouces~~ feat: Addresses.AppendAnnounce Nov 23, 2021

lidel added this to the go-ipfs 0.11 milestone Nov 23, 2021

chore: go-ipfs-config v0.18.0

23c028b

lidel removed the status/blocked Unable to be worked further until needs are met label Nov 23, 2021

lidel removed the need/maintainer-input Needs input from the current maintainer(s) label Nov 23, 2021

Merge branch 'master' into feat/7791

81aeec2

aschmahmann requested changes Nov 30, 2021

View reviewed changes

lidel added a commit to Jorropo/go-ipfs that referenced this pull request Nov 30, 2021

fix: deduplicate Swarm.Announce and AppendAnnounce

ed1edb6

ipfs#8177 (comment) ipfs#8177 (comment)

fix: deduplicate Swarm.Announce and AppendAnnounce

9673571

ipfs#8177 (comment) ipfs#8177 (comment)

lidel force-pushed the feat/7791 branch from ed1edb6 to 9673571 Compare November 30, 2021 16:24

aschmahmann approved these changes Nov 30, 2021

View reviewed changes

lidel merged commit c2953ab into ipfs:master Nov 30, 2021

lidel mentioned this pull request Aug 17, 2023

test: fix "invert" commands in sharness tests #9652

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Addresses.AppendAnnounce #8177

feat: Addresses.AppendAnnounce #8177

Jorropo commented Jun 6, 2021

lidel left a comment

Jorropo commented Jun 26, 2021 •

edited

Loading

lidel commented Jul 6, 2021 •

edited

Loading

TheDiscordian commented Jul 8, 2021

aschmahmann commented Jul 8, 2021

lidel left a comment •

edited

Loading

Jorropo commented Jul 19, 2021

texfu left a comment

lidel left a comment •

edited by aschmahmann

Loading

lidel Nov 15, 2021 •

edited

Loading

lidel commented Nov 23, 2021

aschmahmann Nov 30, 2021

lidel Nov 30, 2021 •

edited

Loading

Jorropo Nov 30, 2021

lidel Nov 30, 2021 •

edited

Loading

lidel commented Nov 30, 2021

		fx.Provide(libp2p.SmuxTransport(cfg.Swarm.Transports)),
		fx.Provide(libp2p.Relay(enableRelay, cfg.Swarm.EnableRelayHop)),

feat: Addresses.AppendAnnounce #8177

feat: Addresses.AppendAnnounce #8177

Conversation

Jorropo commented Jun 6, 2021

lidel left a comment

Choose a reason for hiding this comment

Jorropo commented Jun 26, 2021 • edited Loading

lidel commented Jul 6, 2021 • edited Loading

TheDiscordian commented Jul 8, 2021

aschmahmann commented Jul 8, 2021

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Jorropo commented Jul 19, 2021

texfu left a comment

Choose a reason for hiding this comment

lidel left a comment • edited by aschmahmann Loading

Choose a reason for hiding this comment

lidel Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

lidel commented Nov 23, 2021

aschmahmann Nov 30, 2021

Choose a reason for hiding this comment

lidel Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Jorropo Nov 30, 2021

Choose a reason for hiding this comment

lidel Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

lidel commented Nov 30, 2021

Jorropo commented Jun 26, 2021 •

edited

Loading

lidel commented Jul 6, 2021 •

edited

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited by aschmahmann

Loading

lidel Nov 15, 2021 •

edited

Loading

lidel Nov 30, 2021 •

edited

Loading

lidel Nov 30, 2021 •

edited

Loading