core/bootstrap: fix panic without backup bootstrap peer functions (#10029)
Fix panic when backup bootstrap peer load and save funcs are nil 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.
Andrew Gillis committed
Sep 21, 2023 at 09:29 UTC
c46cbecb832b9a25f74a275b946b3a0ff3aefaba
3 files changed
+161
-12
core/bootstrap/bootstrap.go
+43
-7
@@ -59,8 +59,8 @@ type BootstrapConfig struct {
59
// as backup bootstrap peers.
60
MaxBackupBootstrapSize int
61
62
- SaveBackupBootstrapPeers func(context.Context, []peer.AddrInfo)
63
- LoadBackupBootstrapPeers func(context.Context) []peer.AddrInfo
62
+ saveBackupBootstrapPeers func(context.Context, []peer.AddrInfo)
63
+ loadBackupBootstrapPeers func(context.Context) []peer.AddrInfo
64
}
65
66
// DefaultBootstrapConfig specifies default sane parameters for bootstrapping.
@@ -72,14 +72,41 @@ var DefaultBootstrapConfig = BootstrapConfig{
72
MaxBackupBootstrapSize: 20,
73
}
74
75
-func BootstrapConfigWithPeers(pis []peer.AddrInfo) BootstrapConfig {
75
+// BootstrapConfigWithPeers creates a default BootstrapConfig configured with
76
+// the specified peers, and optional functions to load and save backup peers.
77
+func BootstrapConfigWithPeers(pis []peer.AddrInfo, options ...func(*BootstrapConfig)) BootstrapConfig {
78
cfg := DefaultBootstrapConfig
79
cfg.BootstrapPeers = func() []peer.AddrInfo {
80
return pis
81
}
82
+ for _, opt := range options {
83
+ opt(&cfg)
84
+ }
85
return cfg
86
}
87
88
+// WithBackupPeers configures functions to load and save backup bootstrap peers.
89
+func WithBackupPeers(load func(context.Context) []peer.AddrInfo, save func(context.Context, []peer.AddrInfo)) func(*BootstrapConfig) {
90
+ if save == nil && load != nil || save != nil && load == nil {
91
+ panic("both load and save backup bootstrap peers functions must be defined")
92
+ }
93
+ return func(cfg *BootstrapConfig) {
94
+ cfg.loadBackupBootstrapPeers = load
95
+ cfg.saveBackupBootstrapPeers = save
96
+ }
97
+}
98
+
99
+// BackupPeers returns the load and save backup peers functions.
100
+func (cfg *BootstrapConfig) BackupPeers() (func(context.Context) []peer.AddrInfo, func(context.Context, []peer.AddrInfo)) {
101
+ return cfg.loadBackupBootstrapPeers, cfg.saveBackupBootstrapPeers
102
+}
103
+
104
+// SetBackupPeers sets the load and save backup peers functions.
105
+func (cfg *BootstrapConfig) SetBackupPeers(load func(context.Context) []peer.AddrInfo, save func(context.Context, []peer.AddrInfo)) {
106
+ opt := WithBackupPeers(load, save)
107
+ opt(cfg)
108
+}
109
+
110
// Bootstrap kicks off IpfsNode bootstrapping. This function will periodically
111
// check the number of open connections and -- if there are too few -- initiate
112
// connections to well-known bootstrap peers. It also kicks off subsystem
@@ -124,7 +151,11 @@ func Bootstrap(id peer.ID, host host.Host, rt routing.Routing, cfg BootstrapConf
151
doneWithRound <- struct{}{}
152
close(doneWithRound) // it no longer blocks periodic
153
127
- startSavePeersAsTemporaryBootstrapProc(cfg, host, proc)
154
+ // If loadBackupBootstrapPeers is not nil then saveBackupBootstrapPeers
155
+ // must also not be nil.
156
+ if cfg.loadBackupBootstrapPeers != nil {
157
+ startSavePeersAsTemporaryBootstrapProc(cfg, host, proc)
158
+ }
159
160
return proc, nil
161
}
@@ -185,7 +216,7 @@ func saveConnectedPeersAsTemporaryBootstrap(ctx context.Context, host host.Host,
216
217
// If we didn't reach the target number use previously stored connected peers.
218
if len(backupPeers) < cfg.MaxBackupBootstrapSize {
188
- oldSavedPeers := cfg.LoadBackupBootstrapPeers(ctx)
219
+ oldSavedPeers := cfg.loadBackupBootstrapPeers(ctx)
220
log.Debugf("missing %d peers to reach backup bootstrap target of %d, trying from previous list of %d saved peers",
221
cfg.MaxBackupBootstrapSize-len(backupPeers), cfg.MaxBackupBootstrapSize, len(oldSavedPeers))
222
@@ -209,7 +240,7 @@ func saveConnectedPeersAsTemporaryBootstrap(ctx context.Context, host host.Host,
240
}
241
}
242
212
- cfg.SaveBackupBootstrapPeers(ctx, backupPeers)
243
+ cfg.saveBackupBootstrapPeers(ctx, backupPeers)
244
log.Debugf("saved %d peers (of %d target) as bootstrap backup in the config", len(backupPeers), cfg.MaxBackupBootstrapSize)
245
return nil
246
}
@@ -241,9 +272,14 @@ func bootstrapRound(ctx context.Context, host host.Host, cfg BootstrapConfig) er
272
}
273
}
274
275
+ if cfg.loadBackupBootstrapPeers == nil {
276
+ log.Debugf("not enough bootstrap peers to fill the remaining target of %d connections", numToDial)
277
+ return ErrNotEnoughBootstrapPeers
278
+ }
279
+
280
log.Debugf("not enough bootstrap peers to fill the remaining target of %d connections, trying backup list", numToDial)
281
246
- tempBootstrapPeers := cfg.LoadBackupBootstrapPeers(ctx)
282
+ tempBootstrapPeers := cfg.loadBackupBootstrapPeers(ctx)
283
if len(tempBootstrapPeers) > 0 {
284
numToDial -= int(peersConnect(ctx, host, tempBootstrapPeers, numToDial, false))
285
if numToDial <= 0 {
core/bootstrap/bootstrap_test.go
+114
@@ -1,8 +1,14 @@
1
package bootstrap
2
3
import (
4
+ "context"
5
+ "crypto/rand"
6
+ "reflect"
7
"testing"
8
+ "time"
9
10
+ "github.com/libp2p/go-libp2p"
11
+ "github.com/libp2p/go-libp2p/core/crypto"
12
"github.com/libp2p/go-libp2p/core/peer"
13
"github.com/libp2p/go-libp2p/core/test"
14
)
@@ -23,3 +29,111 @@ func TestRandomizeAddressList(t *testing.T) {
29
t.Fail()
30
}
31
}
32
+
33
+func TestLoadAndSaveOptions(t *testing.T) {
34
+ loadFunc := func(_ context.Context) []peer.AddrInfo { return nil }
35
+ saveFunc := func(_ context.Context, _ []peer.AddrInfo) {}
36
+
37
+ bootCfg := BootstrapConfigWithPeers(nil, WithBackupPeers(loadFunc, saveFunc))
38
+ load, save := bootCfg.BackupPeers()
39
+ if load == nil {
40
+ t.Fatal("load function not assigned")
41
+ }
42
+ if reflect.ValueOf(load).Pointer() != reflect.ValueOf(loadFunc).Pointer() {
43
+ t.Fatal("load not assigned correct function")
44
+ }
45
+ if save == nil {
46
+ t.Fatal("save function not assigned")
47
+ }
48
+ if reflect.ValueOf(save).Pointer() != reflect.ValueOf(saveFunc).Pointer() {
49
+ t.Fatal("save not assigned correct function")
50
+ }
51
+
52
+ assertPanics(t, "with only load func", func() {
53
+ BootstrapConfigWithPeers(nil, WithBackupPeers(loadFunc, nil))
54
+ })
55
+
56
+ assertPanics(t, "with only save func", func() {
57
+ BootstrapConfigWithPeers(nil, WithBackupPeers(nil, saveFunc))
58
+ })
59
+
60
+ bootCfg = BootstrapConfigWithPeers(nil, WithBackupPeers(nil, nil))
61
+ load, save = bootCfg.BackupPeers()
62
+ if load != nil || save != nil {
63
+ t.Fatal("load and save functions should both be nil")
64
+ }
65
+}
66
+
67
+func TestSetBackupPeers(t *testing.T) {
68
+ loadFunc := func(_ context.Context) []peer.AddrInfo { return nil }
69
+ saveFunc := func(_ context.Context, _ []peer.AddrInfo) {}
70
+
71
+ bootCfg := DefaultBootstrapConfig
72
+ bootCfg.SetBackupPeers(loadFunc, saveFunc)
73
+ load, save := bootCfg.BackupPeers()
74
+ if load == nil {
75
+ t.Fatal("load function not assigned")
76
+ }
77
+ if reflect.ValueOf(load).Pointer() != reflect.ValueOf(loadFunc).Pointer() {
78
+ t.Fatal("load not assigned correct function")
79
+ }
80
+ if save == nil {
81
+ t.Fatal("save function not assigned")
82
+ }
83
+ if reflect.ValueOf(save).Pointer() != reflect.ValueOf(saveFunc).Pointer() {
84
+ t.Fatal("save not assigned correct function")
85
+ }
86
+
87
+ assertPanics(t, "with only load func", func() {
88
+ bootCfg.SetBackupPeers(loadFunc, nil)
89
+ })
90
+
91
+ assertPanics(t, "with only save func", func() {
92
+ bootCfg.SetBackupPeers(nil, saveFunc)
93
+ })
94
+
95
+ bootCfg.SetBackupPeers(nil, nil)
96
+ load, save = bootCfg.BackupPeers()
97
+ if load != nil || save != nil {
98
+ t.Fatal("load and save functions should both be nil")
99
+ }
100
+}
101
+
102
+func TestNoTempPeersLoadAndSave(t *testing.T) {
103
+ period := 500 * time.Millisecond
104
+ bootCfg := BootstrapConfigWithPeers(nil)
105
+ bootCfg.MinPeerThreshold = 2
106
+ bootCfg.Period = period
107
+
108
+ priv, pub, err := crypto.GenerateEd25519Key(rand.Reader)
109
+ if err != nil {
110
+ t.Fatal(err)
111
+ }
112
+ peerID, err := peer.IDFromPublicKey(pub)
113
+ if err != nil {
114
+ t.Fatal(err)
115
+ }
116
+ p2pHost, err := libp2p.New(libp2p.Identity(priv))
117
+ if err != nil {
118
+ t.Fatal(err)
119
+ }
120
+
121
+ bootstrapper, err := Bootstrap(peerID, p2pHost, nil, bootCfg)
122
+ if err != nil {
123
+ t.Fatal(err)
124
+ }
125
+
126
+ time.Sleep(4 * period)
127
+ bootstrapper.Close()
128
+
129
+}
130
+
131
+func assertPanics(t *testing.T, name string, f func()) {
132
+ defer func() {
133
+ if r := recover(); r == nil {
134
+ t.Errorf("%s: did not panic as expected", name)
135
+ }
136
+ }()
137
+
138
+ f()
139
+}
core/core.go
+4
-5
@@ -168,17 +168,15 @@ func (n *IpfsNode) Bootstrap(cfg bootstrap.BootstrapConfig) error {
168
return ps
169
}
170
}
171
- if cfg.SaveBackupBootstrapPeers == nil {
172
- cfg.SaveBackupBootstrapPeers = func(ctx context.Context, peerList []peer.AddrInfo) {
171
+ if load, _ := cfg.BackupPeers(); load == nil {
172
+ save := func(ctx context.Context, peerList []peer.AddrInfo) {
173
err := n.saveTempBootstrapPeers(ctx, peerList)
174
if err != nil {
175
log.Warnf("saveTempBootstrapPeers failed: %s", err)
176
return
177
}
178
}
179
- }
180
- if cfg.LoadBackupBootstrapPeers == nil {
181
- cfg.LoadBackupBootstrapPeers = func(ctx context.Context) []peer.AddrInfo {
179
+ load = func(ctx context.Context) []peer.AddrInfo {
180
peerList, err := n.loadTempBootstrapPeers(ctx)
181
if err != nil {
182
log.Warnf("loadTempBootstrapPeers failed: %s", err)
@@ -186,6 +184,7 @@ func (n *IpfsNode) Bootstrap(cfg bootstrap.BootstrapConfig) error {
184
}
185
return peerList
186
}
187
+ cfg.SetBackupPeers(load, save)
188
}
189
190
repoConf, err := n.Repo.Config()