routing: Add triert package with Xor Trie backed routing table #50

iand · 2023-07-04T13:25:38Z

Fixes #44

Trie-backed routing table, safe for concurrent use. Refresh capabilities will come with #45

Notes for follow up:

the routing table has no enforced maximum size
it may be populated with a crawl mechanism like FullRT but this process will be decoupled from the routing table
it might be better to copy the xor trie into the go-kademlia package to allow extension, specifically storing both the key and the node id in the trie for efficiency. Also we can then use our own Key type rather than relying on the coincidence that both are []byte

routing/triert/table.go

routing/triert/table_test.go

routing/triert/table.go

dennis-tra · 2023-07-04T15:53:19Z

routing/triert/table.go

+// RemoveKey tries to remove a peer identified by its Kademlia key from the
+// routing table
+func (rt *TrieRT) RemoveKey(ctx context.Context, kk key.KadKey) (bool, error) {
+	if kk.Size() != rt.self.Size() {


How likely is it that users do really operate with different KadKeys? How could this happen?

Users should not operate with different KadKey lengths. IMO it is good to check it to avoid bad surprises later.

It's a pity that Go doesn't allow generic types to be specified using specific slice lengths

I also hoped we could constrain the usage to one specific key type.

dennis-tra · 2023-07-04T15:55:19Z

routing/triert/table.go

+	return closestAtDepth(ctx, k, t.Branch[1-chosenDir], depth+1, n, found)
+}
+
+func (rt *TrieRT) Find(ctx context.Context, kk key.KadKey) (address.NodeID, error) {


What will this function be used for? I can only find usages in tests (I checked this repo + the go-libp2p-kad-dht/kbucket repos)

It could be used to verify if a peer is already in the routing table or not, e.g in the context of libp2p/go-libp2p-kad-dht#811 where we send a kademlia request to the remote peer before adding it to the routing table.

In go-libp2p-kbucket this logic is implemented in https://github.com/libp2p/go-libp2p-kbucket/blob/6d85d4e63aa3b8e04b9b55e1de64be8a52532f21/table.go#L119. So there is no use case now (except testing), but it may be useful in the future.

It's used in tests and potentially in other diagnostics / consistency checking. It's not part of the Table interface.

If it's possible we could un-export it.

I don't see the need to unexport it. It's a useful function for code that want to directly query the table

routing/triert/table.go

routing/triert/table_test.go

routing/triert/table.go

iand · 2023-07-05T16:13:51Z

Imported the xor trie into key/trie. I only imported the immutable version and I didn't bring over unions/intersections. We can add all those in the future if we need to. Rearranged the API a little to my taste and reworked all the tests.

iand · 2023-07-05T16:15:36Z

The imported trie with extra data field meant I could drop the map between key and node in the triert

iand · 2023-07-05T16:18:33Z

Error checking is in preparation for #51 (comment) if we decide to take that route

Co-authored-by: Guillaume Michel - guissou <[email protected]>

key/keyutil/keyutil.go

…of Go

iand · 2023-07-13T11:05:10Z

PTAL

Recent changes:

added config to TrieRT (also see Prefer simple config types over function-style options #64)
added KeyFilterFunc
removed concurrency handling from TrieRT, it mutates the Trie in place
added benchmarks

Initial benchmarks:

goos: linux
goarch: amd64
pkg: github.com/plprobelab/go-kademlia/key/trie
cpu: 13th Gen Intel(R) Core(TM) i7-13650HX
BenchmarkBuildTrieMutable/1000-20          5368      239713 ns/op    1286779 ns/key      186240 B/op        2910 allocs/op
BenchmarkBuildTrieMutable/10000-20          278     4296195 ns/op     119433 ns/key     1836428 B/op       28694 allocs/op
BenchmarkBuildTrieMutable/100000-20          39    29098613 ns/op      11348 ns/key    18480013 B/op      288750 allocs/op
BenchmarkBuildTrieImmutable/1000-20        2350      692483 ns/op    1627331 ns/key      787776 B/op       12309 allocs/op
BenchmarkBuildTrieImmutable/10000-20         96    13987724 ns/op     134282 ns/key     9982505 B/op      155976 allocs/op
BenchmarkBuildTrieImmutable/100000-20        10   103134639 ns/op      10313 ns/key    121270315 B/op     1894848 allocs/op
BenchmarkAddMutable/1000-20               25603       47707 ns/op    4885774 ns/key       46465 B/op         726 allocs/op
BenchmarkAddMutable/10000-20                806     1463271 ns/op     471757 ns/key      466178 B/op        7284 allocs/op
BenchmarkAddMutable/100000-20               100    10893145 ns/op      43572 ns/key     4603648 B/op       71932 allocs/op
BenchmarkAddImmutable/1000-20              9826      232817 ns/op    9150635 ns/key      232640 B/op        3635 allocs/op
BenchmarkAddImmutable/10000-20              297     3863193 ns/op     458946 ns/key     2744902 B/op       42889 allocs/op
BenchmarkAddImmutable/100000-20              52    23498035 ns/op      48876 ns/key    32888064 B/op      513876 allocs/op
BenchmarkRemoveMutable/1000-20            37214       34692 ns/op    5164169 ns/key           0 B/op           0 allocs/op
BenchmarkRemoveMutable/10000-20             960     1309283 ns/op     502763 ns/key           4 B/op           0 allocs/op
BenchmarkRemoveMutable/100000-20            100    10223293 ns/op      40893 ns/key           1 B/op           0 allocs/op
BenchmarkRemoveImmutable/1000-20           8344      249415 ns/op    8324454 ns/key      194496 B/op        3039 allocs/op
BenchmarkRemoveImmutable/10000-20           284     4433920 ns/op     503692 ns/key     2476353 B/op       38693 allocs/op
BenchmarkRemoveImmutable/100000-20           42    28681932 ns/op      48186 ns/key    29994752 B/op      468668 allocs/op
BenchmarkFindPositive/1000-20          18115773       63.32 ns/op                             0 B/op           0 allocs/op
BenchmarkFindPositive/10000-20         11876832       95.95 ns/op                             0 B/op           0 allocs/op
BenchmarkFindPositive/100000-20         5064666       232.6 ns/op                             0 B/op           0 allocs/op
BenchmarkFindNegative/1000-20          35327665       34.01 ns/op                             0 B/op           0 allocs/op
BenchmarkFindNegative/10000-20         15040240       75.56 ns/op                             0 B/op           0 allocs/op
BenchmarkFindNegative/100000-20         6080850       198.8 ns/op                             0 B/op           0 allocs/op


goos: linux
goarch: amd64
pkg: github.com/plprobelab/go-kademlia/routing/triert
cpu: 13th Gen Intel(R) Core(TM) i7-13650HX
BenchmarkBuildTable/1000-20              6938    272436 ns/op   1890159 ns/node    185072 B/op     2892 allocs/op
BenchmarkBuildTable/10000-20              256   4579850 ns/op   117244 ns/node    1836924 B/op    28702 allocs/op
BenchmarkBuildTable/100000-20              40   9681654 ns/op   11873 ns/node    18486147 B/op   288846 allocs/op
BenchmarkFindPositive/1000-20        20827332     55.88 ns/op                           0 B/op        0 allocs/op
BenchmarkFindPositive/10000-20       14346126     83.10 ns/op                           0 B/op        0 allocs/op
BenchmarkFindPositive/100000-20       6442490     189.8 ns/op                           0 B/op        0 allocs/op
BenchmarkFindNegative/1000-20        35187136     33.13 ns/op                           0 B/op        0 allocs/op
BenchmarkFindNegative/10000-20       15310812     73.62 ns/op                           0 B/op        0 allocs/op
BenchmarkFindNegative/100000-20       6700594     181.9 ns/op                           0 B/op        0 allocs/op
BenchmarkNearestPeers/1000-20          318604      5927 ns/op                        5965 B/op       44 allocs/op
BenchmarkNearestPeers/10000-20         129309      9016 ns/op                        5970 B/op       44 allocs/op
BenchmarkNearestPeers/100000-20        229111      5404 ns/op                        5983 B/op       44 allocs/op
BenchmarkChurn/1000-20                5453337     199.6 ns/op                          64 B/op        1 allocs/op
BenchmarkChurn/10000-20               3280557     375.0 ns/op                          63 B/op        0 allocs/op
BenchmarkChurn/100000-20              3017536     388.5 ns/op                          65 B/op        1 allocs/op

iand · 2023-07-13T11:05:34Z

Windows/macos tests are still flaky. I think this is quic related.

dennis-tra · 2023-07-13T11:14:17Z

key/key_test.go

+	require.Equal(t, 1, kk.BitAt(6))
+	require.Equal(t, 1, kk.BitAt(7))
+	require.Equal(t, 1, kk.BitAt(8))
+	require.Equal(t, 0, kk.BitAt(15))


not sure if it makes sense to also assert the behavior for invalid input like i < 0 and i >= len(bits(kk)). Just something that crossed my mind.

dennis-tra · 2023-07-13T11:15:44Z

key/keyutil/keyutil.go

@@ -0,0 +1,51 @@
+package keyutil


suggestion: could move to the key package into a rand.go file.

If it's just used for testing we could also move it to the testutil package.

I think I will move to testutil. I don't think we want to encourage creating random keys with math/rand for anything other than tests

dennis-tra · 2023-07-13T11:23:37Z

key/keyutil/keyutil.go

+}
+
+// RandomWithPrefix returns a KadKey of length l having a prefix equal to the bit pattern held in s.
+func RandomWithPrefix(s string, l int) key.KadKey {


the comment should mention the maximum prefix of 64 bits.

since there are input checks there could also be a check that len(s) <= l*8 is true

dennis-tra · 2023-07-13T11:25:10Z

key/keyutil/keyutil.go

+
+var rng = rand.New(rand.NewSource(299792458))
+
+// Random returns a KadKey of length l populated with random data.


here, length refers to the number of bytes instead of the number of bits. I haven't checked there rest but the usage of length in the context of a KadKey should be consistent throughout the code.

Alternatively we could use:

size -> bytes

length -> bits

dennis-tra · 2023-07-13T11:26:14Z

key/trie/trie.go

+	"github.com/plprobelab/go-kademlia/key"
+)
+
+var ErrMismatchedKeyLength = errors.New("key length does not match existing keys")


rm in favor of the key package error

I think it belongs here. The key package error is for invalid keys. Here all the keys are valid keys, but one does not match the format of the others

dennis-tra · 2023-07-13T11:30:04Z

key/trie/trie.go

+	return &Trie[T]{}
+}
+
+func (tr *Trie[T]) Key() key.KadKey {


Not sure about the implications but Trie conforms to the NodeID interface. Perhaps rather export the Key and Data fields?

I unexprted Key and Data when adding the mutable Add/Remove methods. My thinking is that we want to limit the chances for users to corrupt the trie by overwriting fields like key. In practice the Key method will be inlined so there's no real performance overhead.

Not sure I understand the point about conforming to NodeID interface - what would be the use case?

dennis-tra · 2023-07-13T11:47:10Z

routing/triert/table.go

+	return n
+}
+
+func countCpl(t *nodeTrie, kk key.KadKey, cpl int, depth int) (int, error) {


This could move to trie.Trie

dennis-tra · 2023-07-13T11:50:51Z

routing/triert/table.go

+func closestAtDepth(kk key.KadKey, t *nodeTrie, depth int, n int) []entry {
+	if t.IsLeaf() {


This could move to trie.Trie and return the trie instead of an entry

iand commented Jul 4, 2023

View reviewed changes

routing/triert/table.go Show resolved Hide resolved