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

core/bootstrap: fix panic without backup bootstrap peer functions #10029

Merged
merged 6 commits into from
Sep 21, 2023

Conversation

gammazero
Copy link
Contributor

@gammazero gammazero commented Jul 26, 2023

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.

@gammazero gammazero requested a review from a team as a code owner July 26, 2023 16:45
@hacdias hacdias changed the title Fix/no panic without temppeerfuncs core/bootstrap: fix panic without backup bootstrap peer functions Jul 27, 2023
docs/changelogs/v0.22.md Outdated Show resolved Hide resolved
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

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

@hacdias hacdias added the skip/changelog This change does NOT require a changelog entry label Jul 27, 2023
core/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
@gammazero gammazero requested a review from Jorropo July 27, 2023 17:53
@BigLep BigLep mentioned this pull request Jul 27, 2023
Comment on lines 104 to 108
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")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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")
}
}

Copy link
Contributor Author

@gammazero gammazero Aug 1, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 94 to 95
cfg.LoadBackupBootstrapPeers = load
cfg.SaveBackupBootstrapPeers = save
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@gammazero gammazero Aug 1, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 gammazero requested a review from Jorropo August 1, 2023 19:33
@gammazero
Copy link
Contributor Author

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

gammazero added a commit to ipni/storetheindex that referenced this pull request Aug 2, 2023
Need this because of ipfs/kubo#10030.

Can be removed when ipfs/kubo#10029 is available.
gammazero added a commit to ipni/storetheindex that referenced this pull request Aug 2, 2023
Comment on lines +100 to +108
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)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@BigLep
Copy link
Contributor

BigLep commented Aug 22, 2023

@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
Copy link
Contributor Author

@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.

@gammazero gammazero force-pushed the fix/no-panic-without-temppeerfuncs branch from 5dc22d3 to 9b76229 Compare August 30, 2023 19:09
@lidel lidel requested a review from Jorropo September 4, 2023 13:51
@BigLep BigLep mentioned this pull request Sep 5, 2023
14 tasks
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.
@gammazero gammazero force-pushed the fix/no-panic-without-temppeerfuncs branch from 9b76229 to 5e3ebf2 Compare September 9, 2023 08:08
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Jorropo commented Sep 21, 2023

Thx

@Jorropo Jorropo merged commit c46cbec into master Sep 21, 2023
18 checks passed
@Jorropo Jorropo deleted the fix/no-panic-without-temppeerfuncs branch September 21, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does NOT require a changelog entry
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants