core/bootstrap: fix panic without backup bootstrap peer functions by gammazero · Pull Request #10029 · ipfs/kubo

gammazero · 2023-07-26T16:45:01Z

A panic occurs when the first bootstrap round runs is these functions are not assigned in the configuration:

LoadBackupBootstrapPeers
SaveBackupBootstrapPeers

This fix assumes that it is acceptable for these functions to be nil, as it may be desirable to disable the backup peer load and save functionality.

hacdias

Thanks for this. I think this is a valuable fix to be included in 0.22. CC @Jorropo

Jorropo · 2023-08-01T16:59:05Z

+	if cfg.LoadBackupBootstrapPeers == nil || cfg.SaveBackupBootstrapPeers == nil {
+		if cfg.LoadBackupBootstrapPeers != nil || cfg.SaveBackupBootstrapPeers != nil {
+			return nil, errors.New("LoadBackupBootstrapPeers and SaveBackupBootstrapPeers must both be defined or nil")
+		}
+	}


Is this not redundant now that you have:

if save == nil && load != nil || save != nil && load == nil { panic("both load and save backup bootstrap peers functions must be defined") }

?

Suggested change

if cfg.LoadBackupBootstrapPeers == nil || cfg.SaveBackupBootstrapPeers == nil {

if cfg.LoadBackupBootstrapPeers != nil || cfg.SaveBackupBootstrapPeers != nil {

return nil, errors.New("LoadBackupBootstrapPeers and SaveBackupBootstrapPeers must both be defined or nil")

}

}

It is only redundant if the WithBackupPeers option is used. Given that is an option, a BootstrapConfig may still be created where only one of the two functions is nil. Therefore, this check is still necessary.

... Unless BootstrapConfig.LoadBackupBootstrapPeers and BootstrapConfig.SaveBackupBootstrapPeers are made private. However, I suggest we do not do that. See next comment.

Jorropo

Except that one comment LGTM, we can get this for 0.22 by august 03

Jorropo · 2023-08-01T18:38:05Z

+		cfg.LoadBackupBootstrapPeers = load
+		cfg.SaveBackupBootstrapPeers = save


Oh and I realized, theses needs to be private now that you have an option.

The problem with making these private is that they are accessed directly here, and possibly elsewhere:
https://github.com/ipfs/kubo/blob/master/core/core.go#L172-L173
https://github.com/ipfs/kubo/blob/master/core/core.go#L181-L182

Also, since those items are exported in the current release, making them private could be considered a breaking API change as it would break any other software that sets these, expecting them to be exported (public).

@Jorropo Would you prefer I leave things the way they are, or would you prefer that I make the load and save functions private add a SetBackupPeers function with BootstrapConfig as a receiver to set those functions, and add a HasBackupPeers function to check if they are already set?

I would vote to leave things the way they are in this PR, because it does not cause any more risk with the error checks in place, and it avoids any breaking API changes.

We can fix the cases in Kubo.

Also, since those items are exported in the current release, making them private could be considered a breaking API change as it would break any other software that sets these, expecting them to be exported (public).

It was already a breaking API change to add them and make it panic if nil.
We are not carefull about not breaking consumers inside Kubo, boxo is meant for the pieces of code that we are carefull to not break so consumers can import them.

gammazero · 2023-08-01T20:30:54Z

@Jorropo I made the load and save backup peers private, and provided getter and setter methods.

Need this because of ipfs/kubo#10030. Can be removed when ipfs/kubo#10029 is available.

Jorropo · 2023-08-02T15:23:53Z

+func (cfg *BootstrapConfig) BackupPeers() (func(context.Context) []peer.AddrInfo, func(context.Context, []peer.AddrInfo)) {
+	return cfg.loadBackupBootstrapPeers, cfg.saveBackupBootstrapPeers
+}
+
+// SetBackupPeers sets the load and save backup peers functions.
+func (cfg *BootstrapConfig) SetBackupPeers(load func(context.Context) []peer.AddrInfo, save func(context.Context, []peer.AddrInfo)) {
+	opt := WithBackupPeers(load, save)
+	opt(cfg)
+}


That looks racy.
I meant Kubo should pass WithBackupPeers option.
I'll submit a patch given this is unfair to ask you to go rumble in the go internals, thx for this.

@Jorropo bump - Is there anything else you want me to do, or wait for your patch?

BigLep · 2023-08-22T20:13:24Z

@Jorropo : I know this isn't a priority for you this week given other things you're juggling. Lets make a statement whether this will be included in 0.23 or not though.

gammazero · 2023-08-25T20:12:02Z

@Jorropo I am planning on moving this package into boxo and I would like to get this PR completed before I do. Let me know if you want me to do anything more.

A panic occurs when the first bootstrap round runs is these functions are not assigned in the configuration: - `LoadBackupBootstrapPeers` - `SaveBackupBootstrapPeers` This fix assumes that it is acceptable for these functions to be nil, as it may be desirable to disable the bakcup peer load and save functionality.

Jorropo

I tried removing SetBackupPeers but I understand the problem now, this is not built by kubo but passed in by the consumer changing this is more breaking changes. This looks good thx.

Jorropo · 2023-09-21T16:29:17Z

Thx

gammazero requested a review from a team as a code owner July 26, 2023 16:45

gammazero mentioned this pull request Jul 26, 2023

Bootstrap panics if no config has no xBackupBootstrapPeers functions #10030

Closed

3 tasks

hacdias changed the title ~~Fix/no panic without temppeerfuncs~~ core/bootstrap: fix panic without backup bootstrap peer functions Jul 27, 2023

hacdias reviewed Jul 27, 2023

View reviewed changes

Comment thread docs/changelogs/v0.22.md

hacdias approved these changes Jul 27, 2023

View reviewed changes

hacdias added the skip/changelog This change does NOT require a changelog entry label Jul 27, 2023

Jorropo requested changes Jul 27, 2023

View reviewed changes

Comment thread core/bootstrap/bootstrap.go Outdated

gammazero requested a review from Jorropo July 27, 2023 17:53

BigLep mentioned this pull request Jul 27, 2023

Release 0.22 #9911

Closed

Jorropo reviewed Aug 1, 2023

View reviewed changes

gammazero requested a review from Jorropo August 1, 2023 19:33

gammazero added a commit to ipni/storetheindex that referenced this pull request Aug 2, 2023

Fix panic due to unassigned bootstrap config functions

0bb80cc

Need this because of ipfs/kubo#10030. Can be removed when ipfs/kubo#10029 is available.

gammazero mentioned this pull request Aug 2, 2023

Fix panic due to unassigned bootstrap config functions ipni/storetheindex#2191

Merged

gammazero added a commit to ipni/storetheindex that referenced this pull request Aug 2, 2023

Fix panic due to unassigned bootstrap config functions (#2191)

c06ba7c

Need this because of ipfs/kubo#10030. Can be removed when ipfs/kubo#10029 is available.

hacdias reviewed Aug 2, 2023

View reviewed changes

Comment thread core/bootstrap/bootstrap.go

hacdias approved these changes Aug 2, 2023

View reviewed changes

Jorropo requested changes Aug 2, 2023

View reviewed changes

BigLep assigned Jorropo Aug 22, 2023

gammazero force-pushed the fix/no-panic-without-temppeerfuncs branch from 5dc22d3 to 9b76229 Compare August 30, 2023 19:09

lidel requested a review from Jorropo September 4, 2023 13:51

BigLep mentioned this pull request Sep 5, 2023

Release 0.23 #10014

Closed

14 tasks

gammazero added 4 commits September 9, 2023 01:07

Review: options for setting backup load and save functions

53559da

fix comment

4ace928

Make load and save backup peers functions private

98d4895

gammazero added 2 commits September 9, 2023 01:07

format comment

3d25ad6

Fix lint warn

5e3ebf2

gammazero force-pushed the fix/no-panic-without-temppeerfuncs branch from 9b76229 to 5e3ebf2 Compare September 9, 2023 08:08

Jorropo approved these changes Sep 21, 2023

View reviewed changes

Jorropo merged commit c46cbec into master Sep 21, 2023

Jorropo deleted the fix/no-panic-without-temppeerfuncs branch September 21, 2023 16:29

		cfg.LoadBackupBootstrapPeers = load
		cfg.SaveBackupBootstrapPeers = save

Uh oh!

Conversation

gammazero commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hacdias left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jorropo Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

gammazero Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jorropo left a comment

Choose a reason for hiding this comment

Uh oh!

Jorropo Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

gammazero Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jorropo Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

gammazero commented Aug 1, 2023

Uh oh!

Uh oh!

Jorropo Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

gammazero Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

BigLep commented Aug 22, 2023

Uh oh!

gammazero commented Aug 25, 2023

Uh oh!

Jorropo left a comment

Choose a reason for hiding this comment

Uh oh!

Jorropo commented Sep 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gammazero commented Jul 26, 2023 •

edited

Loading

gammazero Aug 1, 2023 •

edited

Loading

gammazero Aug 1, 2023 •

edited

Loading