feat(kademlia): static nodes

[aloknerurkar]

Add configuration for adding protected nodes on the bootnodes. This change will prevent kicking out of the protected nodes to ensure better connectivity.

---

This change is 

[acud]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aloknerurkar and @mrekucci)

---

cmd/bee/cmd/cmd.go, line 78 at r1 (raw file):

	optionNamePProfBlock                 = "pprof-profile"
	optionNamePProfMutex                 = "pprof-mutex"
	optionNameProtectedNodes             = "protected-nodes"

nit, consider static-nodes or static-node-overlays

---

cmd/bee/cmd/start.go, line 141 at r1 (raw file):

			protectedNodes := c.config.GetStringSlice(optionNameProtectedNodes)
			if len(protectedNodes) > 0 && !bootNode {

we might want to apply this for other nodes too

---

pkg/node/node.go, line 525 at r1 (raw file):

	var protectedNodes []swarm.Address

	for _, p := range o.ProtectedNodes {

we should probably do this parsing part before calling NewBee and just have the slice of swarm.Address passed on Options. Also, I would return the error in case there was a parsing error. If the provided address isn't correct it might cause us not to list some nodes as static which is something we shouldn't be permissive about

---

pkg/topology/kademlia/kademlia.go, line 893 at r1 (raw file):

	if _, overSaturated := k.saturationFunc(po, k.knownPeers, k.connectedPeers); overSaturated {
		if k.bootnode {
			for i := 0; i < bootNodeOverSaturationPeers; i++ {

I find this loop a bit confusing. Maybe it would be clearer to just change the randomPeer to do what we want here? which is:

• take all of the bin peers
• remove all static overlays from the set
• pick a random one

---

pkg/topology/kademlia/kademlia_test.go, line 1290 at r1 (raw file):

	*kademlia.BootnodeOverSaturationPeers = 1

	defer func(p int) {

no need for 4 defer statements, can all be condensed into one closure

[aloknerurkar]

---

cmd/bee/cmd/start.go, line 141 at r1 (raw file):

Previously, acud (acud) wrote…

we might want to apply this for other nodes too

In the current flow, the kicking-out behavior is only applicable for bootnodes. Which is why I added the check. This then becomes wrong configuration from user which is why I am returning error.

[aloknerurkar]

Dismissed @acud from 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acud and @mrekucci)

---

pkg/node/node.go, line 525 at r1 (raw file):

Previously, acud (acud) wrote…

we should probably do this parsing part before calling NewBee and just have the slice of swarm.Address passed on Options. Also, I would return the error in case there was a parsing error. If the provided address isn't correct it might cause us not to list some nodes as static which is something we shouldn't be permissive about

Done.

---

pkg/topology/kademlia/kademlia.go, line 893 at r1 (raw file):

Previously, acud (acud) wrote…

I find this loop a bit confusing. Maybe it would be clearer to just change the randomPeer to do what we want here? which is:

• take all of the bin peers
• remove all static overlays from the set
• pick a random one

Done.

---

pkg/topology/kademlia/kademlia_test.go, line 1290 at r1 (raw file):

Previously, acud (acud) wrote…

no need for 4 defer statements, can all be condensed into one closure

Done.

[acud]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aloknerurkar and @mrekucci)

---

pkg/topology/kademlia/kademlia.go, line 1356 at r2 (raw file):

		// do not consider protected peers
		if k.isProtected(p) {
			peers = append(peers[:idx], peers[idx+1:]...)

not sure how this plays along with the fact you're inside the for loop that iterates over the same slice you're modifying, but this is about the implementation of the range statement. You might want to for ..., i<len(peers), i++ {} manually to make this clearer, but then you'll have to i++ only in case you did not remove an element (since once you remove an element, you basically need to inspect the same index next time you iterate)

[acud]

also, test is flaking:

=== RUN   TestBootnodeProtectedNodes
    kademlia_test.go:1336: expected error oversaturated , got empty bin
--- FAIL: TestBootnodeProtectedNodes (0.08s)

[aloknerurkar]

---

pkg/topology/kademlia/kademlia.go, line 1356 at r2 (raw file):

Previously, acud (acud) wrote…

not sure how this plays along with the fact you're inside the for loop that iterates over the same slice you're modifying, but this is about the implementation of the range statement. You might want to for ..., i<len(peers), i++ {} manually to make this clearer, but then you'll have to i++ only in case you did not remove an element (since once you remove an element, you basically need to inspect the same index next time you iterate)

Yes, you are right, this can definitely cause issues. Fixed.

[acud]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aloknerurkar and @mrekucci)

---

pkg/topology/kademlia/kademlia.go, line 896 at r3 (raw file):

			randPeer, err := k.randomPeer(po)
			if err != nil {
				return fmt.Errorf("failed to get random peer to kick-out: %w", topology.ErrOversaturated)

i think that if the bin is empty for whatever reason, this should constitute as an error, we still want the peer no?

[mrekucci]

Reviewed 1 of 6 files at r1, 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aloknerurkar)

---

cmd/bee/cmd/start.go, line 146 at r3 (raw file):

				addr, err := swarm.ParseHexAddress(p)
				if err != nil {
					return errors.New("invalid swarm address configured for static node")

It would be helpful to also print the invalid address.

[acud]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aloknerurkar)

---

pkg/topology/kademlia/kademlia.go, line 896 at r3 (raw file):

Previously, acud (acud) wrote…

i think that if the bin is empty for whatever reason, this should constitute as an error, we still want the peer no?

also, it is misleading that an error of oversaturation is being returned here...

[aloknerurkar]

---

pkg/topology/kademlia/kademlia.go, line 896 at r3 (raw file):

Previously, acud (acud) wrote…

also, it is misleading that an error of oversaturation is being returned here...

We enter this loop only if the bin is overSaturated. The only reason why randomPeer would return empty bin error is if all the nodes are protected. This still means the bin is oversaturated, which is why I dont use the emptyBin error. Actually I was not sure why we would get empty bin here in the first place?

[acud]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aloknerurkar)

---

pkg/topology/kademlia/kademlia.go, line 896 at r3 (raw file):

Previously, aloknerurkar wrote…

We enter this loop only if the bin is overSaturated. The only reason why randomPeer would return empty bin error is if all the nodes are protected. This still means the bin is oversaturated, which is why I dont use the emptyBin error. Actually I was not sure why we would get empty bin here in the first place?

so... i am trying to think about some edge cases... imagine we have by coincidence few static nodes in one bin, it could be (even in the test) that you end up with no peers to remove. it is a bit of an edge case (it would require that the bin is oversaturated with static nodes). not completely unlikely but would require us to have a lot of bootnodes

[aloknerurkar]

---

pkg/topology/kademlia/kademlia.go, line 896 at r3 (raw file):

Previously, acud (acud) wrote…

so... i am trying to think about some edge cases... imagine we have by coincidence few static nodes in one bin, it could be (even in the test) that you end up with no peers to remove. it is a bit of an edge case (it would require that the bin is oversaturated with static nodes). not completely unlikely but would require us to have a lot of bootnodes

but then even in this case, I think returning oversaturated error makes more sense as randomPeer would fail with empty bin when we have saturated bin with protected nodes right?

[acud]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aloknerurkar)

---

pkg/topology/kademlia/kademlia.go, line 896 at r3 (raw file):

Previously, aloknerurkar wrote…

but then even in this case, I think returning oversaturated error makes more sense as randomPeer would fail with empty bin when we have saturated bin with protected nodes right?

well depends... lets assume that the bin is full of only oversaturationPeers bootnodes, then you would still like to accept the new peer since we don't want to account for the bootnodes...

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[acud]

Reviewed 1 of 1 files at r5, 2 of 3 files at r6, 1 of 1 files at r7, 4 of 4 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aloknerurkar)

[aloknerurkar]

Dismissed @acud from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aloknerurkar)

---

pkg/topology/kademlia/kademlia.go, line 896 at r3 (raw file):

Previously, acud (acud) wrote…

well depends... lets assume that the bin is full of only oversaturationPeers bootnodes, then you would still like to accept the new peer since we don't want to account for the bootnodes...

Done.