Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

kademlia: use IsSameAs instead of Match for capability index comparison #2073

Merged
merged 3 commits into from
Jan 21, 2020

Conversation

acud
Copy link
Member

@acud acud commented Jan 9, 2020

Fixes a bug where full node gets added to the light node capability index because it satisfies the comparison in the capabilities Match function (because light nodes will always be fulfilling a subset of full node capabilities).
Instead, IsSameAs should be used when doing the comparison

@acud acud added the bug label Jan 9, 2020
@acud acud added this to the 0.5.5 milestone Jan 9, 2020
@acud acud requested review from janos and nolash January 9, 2020 10:17
@acud acud self-assigned this Jan 9, 2020
@acud acud added this to Backlog in Swarm Core - Sprint planning via automation Jan 9, 2020
@acud acud moved this from Backlog to In review (includes Documentation) in Swarm Core - Sprint planning Jan 9, 2020
@nolash
Copy link
Contributor

nolash commented Jan 9, 2020

Should there be a test that catches the case?

@acud
Copy link
Member Author

acud commented Jan 13, 2020

Should there be a test that catches the case?

added.

also, please note - the tests in capability packages are full of copy paste errors. please review the changes that i've made to comments (some of them feel a bit incomprehensible) and tests. also, as a sidenote, it is very easy to get lost in capability names called "42:001" and "42:101"; why not go with names that signify the actual capabilities?

Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Test TestCapabilityNeighbourhoodDepth is failing.

@nolash
Copy link
Contributor

nolash commented Jan 15, 2020

@acud So I am wondering now whether this is actually correct. The "light" and "full" client presets really are just temporary presets until we have proper capabilities negotiations logic in place.

Won't this change actually prohibit "light" nodes from connecting to full clients? In terms of capabilities "light" node preset is supposed to be a subset of "full" node preset. Instead of this, probably there should be some logic that prevents the full node from connecting to a light node, but not the other way around.

Sorry for not commenting this earlier, but my head has been elsewhere the last couple of weeks.

@acud
Copy link
Member Author

acud commented Jan 16, 2020

@nolash can you please refer me to the code sanctioning a light client from connecting to a full node?

I'm not sure if I am aware of such. It might at some point prevent a full node from connecting to a light client (although I think that the kademlia SuggestPeer code uses the default index which aggregates all nodes).

Looking at:

swarm/network/kademlia.go

Lines 173 to 180 in 0bc6bef

if vCap.Match(idxItem.Capability) {
log.Trace("Added peer to capability index", "conn", ok, "s", s, "v", vCap, "p", p)
if ok {
k.capabilityIndex[s].conns, _, _ = pot.Add(idxItem.conns, newEntryFromPeer(ePeer), Pof)
} else {
k.capabilityIndex[s].addrs, _, _ = pot.Add(idxItem.addrs, newEntryFromBzzAddress(eAddr), Pof)
}
}

addToCapabilityIndex does not return an error, so I'm not really sure where sanctioning takes place.

@acud acud merged commit 23cfdc0 into master Jan 21, 2020
Swarm Core - Sprint planning automation moved this from In review (includes Documentation) to Done Jan 21, 2020
@acud acud deleted the capability-index-bug branch January 21, 2020 04:00
tzdybal pushed a commit to etclabscore/swarm that referenced this pull request Feb 10, 2020
…on (ethersphere#2073)

* kademlia: use IsSameAs instead of Match for capability index comparison
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants