HP: Implement GroupId -> Registry config lookup

[chaehni]

Fixes #2887

---

This change is 

[lukedirtwalker]

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @chaehni)

---

go/hidden_path_srv/internal/util/registry_algo.go, line 15 at r1 (raw file):

// limitations under the License.

package util

I wonder if we should just create the request package already since we will only use this functionality in the request handler.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 28 at r1 (raw file):

// The algorithm runs in O(Registries*Groups^2) and is at most ln(Groups)+1 times worse
// than an optimal solution.
func GetRegistryMapping(groups []*hiddenpath.Group, localIA addr.IA) (

If we would have this method on a struct we woudln't have to pass in localIA everytime.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 37 at r1 (raw file):

	count := 0
	mapping := map[addr.IA][]hiddenpath.GroupId{}
	covered := map[hiddenpath.GroupId]bool{}

use map[hiddenpath.GroupId]struct{} instead. (see explanation in findDuplicates)

---

go/hidden_path_srv/internal/util/registry_algo.go, line 42 at r1 (raw file):

		if g.HasRegistry(localIA) {
			mapping[localIA] = append(mapping[localIA], g.Id)
			covered[g.Id] = true

Have you considered just removing the alrady covered ones from the group slice here?

---

go/hidden_path_srv/internal/util/registry_algo.go, line 45 at r1 (raw file):

		}
	}
	count += len(mapping[localIA])

count is always equals to len(covered) maybe it would make it clearer to use that.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 51 at r1 (raw file):

		// find Registry that can answer the most queries
		for _, g := range groups {
			if len(g.Registries) == 0 {

I think that should be a separate function at the beginning that validates the input.

func checkRegistries(groups []*hiddenpath.Group) error {
		for _, g := range groups {
			if len(g.Registries) == 0 {
				return nil, common.NewBasicError("Group does not have any Registries",
					nil, "group", g.Id)
			}
             }
}

executing this over and over again doesn't make sense and also unnecessarily complicates the code here.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 78 at r1 (raw file):

func findDuplicates(groups []*hiddenpath.Group) error {
	var seen = map[hiddenpath.GroupId]bool{}

usually in go you'd just map to struct{}.

seen := make(map[hiddenpath.GroupId]struct{}, len(groups))
for _, g := range groups {
     if _, ok := seen[g.Id]; ok { return // err duplicate }
     seen[g.Id] = struct{}{}
}
return nil

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 55 at r1 (raw file):

func TestCases(t *testing.T) {
	var testcases = []map[hiddenpath.GroupId][]addr.IA{

make this

tests := map[string]map[hiddenpath.GroupId][]addr.IA{
     "one remote Registry covers all Ids but needs splitting because of local rule": {
			id1: {reg1},
			id2: {reg1},
			id3: {reg1},
			id4: {reg1, regLocal},
},

so comments actually become part of the code/tests.

below you do

for name, test := range tests {
   t.Run(name, func(t *testing.T) {
   . ...
   })
}

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 90 at r1 (raw file):

	for _, testcase := range testcases {
		set := buildSet(t, testcase)

groups := buildGroups(t, test) ??

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 94 at r1 (raw file):

		require.NoError(t, err)

		t.Run("All Registries are valid", func(t *testing.T) {

That is not Convey no need to create a SubTests. I would instead move this to a helper method maybe. (Applies to 3 blocks below as well)

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 96 at r1 (raw file):

		t.Run("All Registries are valid", func(t *testing.T) {
			allRegs := []addr.IA{}
			for _, v := range set {

for_, g := range groups ?

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 99 at r1 (raw file):

				allRegs = append(allRegs, v.Registries...)
			}
			computed := make([]addr.IA, 0, len(allRegs))

maybe use actualRegs

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 111 at r1 (raw file):

				expected = append(expected, v.Id)
			}
			computed := make([]hiddenpath.GroupId, 0, len(set))

actual

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 131 at r1 (raw file):

			for r, ids := range mapping {
				for _, id := range ids {
					for _, e := range set {

for _, g := range groups{

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 141 at r1 (raw file):

	}

	t.Run("Test duplicates", func(t *testing.T) {

Move this to a separate Test function.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 151 at r1 (raw file):

	})

	t.Run("Test no Registries", func(t *testing.T) {

Move this to a separate Test function.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 162 at r1 (raw file):

}

func Benchmark(b *testing.B) {

do we need this? benchmarking has many traps. I can't tell if this is doing what you want it to do.
E.g. the setup shouldn't be timed so you should call b.ResetTimer() before the loop (see https://golang.org/pkg/testing/#hdr-Benchmarks)
Also it might be that the loop is optimized away since the return value of GetRegistryMapping is not stored, etc.
IMO we don't really need it so we can drop it. If you feel strong about having it I will try to make sure that it actually benchmark the correct thing.

[chaehni]

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @lukedirtwalker)

---

go/hidden_path_srv/internal/util/registry_algo.go, line 15 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I wonder if we should just create the request package already since we will only use this functionality in the request handler.

Done.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 28 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

If we would have this method on a struct we woudln't have to pass in localIA everytime.

Done.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 37 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

use map[hiddenpath.GroupId]struct{} instead. (see explanation in findDuplicates)

Done.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 42 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Have you considered just removing the alrady covered ones from the group slice here?

Yes, the downside is that it modifies the underlying array also for the caller which might have weird side effects.
However, by making GetRegistryMapping a method of the new struct type this is no longer a problem.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 45 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

count is always equals to len(covered) maybe it would make it clearer to use that.

Removed count variable altogether.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 51 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think that should be a separate function at the beginning that validates the input.

func checkRegistries(groups []*hiddenpath.Group) error {
		for _, g := range groups {
			if len(g.Registries) == 0 {
				return nil, common.NewBasicError("Group does not have any Registries",
					nil, "group", g.Id)
			}
             }
}

executing this over and over again doesn't make sense and also unnecessarily complicates the code here.

Done.

---

go/hidden_path_srv/internal/util/registry_algo.go, line 78 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

usually in go you'd just map to struct{}.

seen := make(map[hiddenpath.GroupId]struct{}, len(groups))
for _, g := range groups {
     if _, ok := seen[g.Id]; ok { return // err duplicate }
     seen[g.Id] = struct{}{}
}
return nil

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 55 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

make this

tests := map[string]map[hiddenpath.GroupId][]addr.IA{
     "one remote Registry covers all Ids but needs splitting because of local rule": {
			id1: {reg1},
			id2: {reg1},
			id3: {reg1},
			id4: {reg1, regLocal},
},

so comments actually become part of the code/tests.

below you do

for name, test := range tests {
   t.Run(name, func(t *testing.T) {
   . ...
   })
}

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 90 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

groups := buildGroups(t, test) ??

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 94 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

That is not Convey no need to create a SubTests. I would instead move this to a helper method maybe. (Applies to 3 blocks below as well)

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 96 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

for_, g := range groups ?

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 99 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

maybe use actualRegs

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 111 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

actual

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 131 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

for _, g := range groups{

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 141 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Move this to a separate Test function.

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 151 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Move this to a separate Test function.

Done.

---

go/hidden_path_srv/internal/util/registry_algo_test.go, line 162 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

do we need this? benchmarking has many traps. I can't tell if this is doing what you want it to do.
E.g. the setup shouldn't be timed so you should call b.ResetTimer() before the loop (see https://golang.org/pkg/testing/#hdr-Benchmarks)
Also it might be that the loop is optimized away since the return value of GetRegistryMapping is not stored, etc.
IMO we don't really need it so we can drop it. If you feel strong about having it I will try to make sure that it actually benchmark the correct thing.

You're right. This doesn't seem necessary.

[lukedirtwalker]

Reviewed 6 of 7 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @chaehni)

---

go/hidden_path_srv/internal/hpsegreq/registry_algo.go, line 48 at r2 (raw file):

	l := len(groups)
	for i := 0; i < l; {
		if groups[i].HasRegistry(gi.LocalRegistry) {

I think it would be much easier if you just check this in the loop above where you create the groups slice:

	groups := make([]*hiddenpath.Group, 0, len(ids))
	mapping := map[addr.IA][]hiddenpath.GroupId{}
	for _, id := range ids {
		group := gi.Groups[id]
		if group.HasRegistry(gi.LocalRegistry) {
			mapping[gi.LocalRegistry] = append(mapping[gi.LocalRegistry], id)
		} else {
			groups = append(groups, gi.Groups[id])
		}
	}

---

go/hidden_path_srv/internal/hpsegreq/registry_algo.go, line 102 at r2 (raw file):

				nil, "group", id)
		}
		seen[id] = struct{}{}

You fill it but don't use it. I don't think it is really needed.

---

go/hidden_path_srv/internal/hpsegreq/registry_algo_test.go, line 124 at r2 (raw file):

	}
	t.Run("unknown GroupId", func(t *testing.T) {
		testUnknownIds(t)

Um I meant moving those into separate tests:

func TestUnknownIds(t *testing.T) { ...

and not have a t.Run here.

[chaehni]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)

---

go/hidden_path_srv/internal/hpsegreq/registry_algo.go, line 48 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I think it would be much easier if you just check this in the loop above where you create the groups slice:

	groups := make([]*hiddenpath.Group, 0, len(ids))
	mapping := map[addr.IA][]hiddenpath.GroupId{}
	for _, id := range ids {
		group := gi.Groups[id]
		if group.HasRegistry(gi.LocalRegistry) {
			mapping[gi.LocalRegistry] = append(mapping[gi.LocalRegistry], id)
		} else {
			groups = append(groups, gi.Groups[id])
		}
	}

Done.

---

go/hidden_path_srv/internal/hpsegreq/registry_algo.go, line 102 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

You fill it but don't use it. I don't think it is really needed.

I think I use it 4 lines above (line 93).
Without that statement the algorithm wouldn't terminate on duplicate input.

---

go/hidden_path_srv/internal/hpsegreq/registry_algo_test.go, line 124 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Um I meant moving those into separate tests:

func TestUnknownIds(t *testing.T) { ...

and not have a t.Run here.

Done.

[lukedirtwalker]

Reviewed 1 of 2 files at r4.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @lukedirtwalker)

---

go/hidden_path_srv/internal/hpsegreq/registry_algo.go, line 102 at r2 (raw file):

Previously, chaehni (chaehni) wrote…

I think I use it 4 lines above (line 93).
Without that statement the algorithm wouldn't terminate on duplicate input.

Ah sorry nevermind me. Though I wonder from an API perspective it would be nicer if as much invalid input as possible would already be catched by the compiler. So I suggest to add:

type GroupIdSet map[hiddenpath.GroupId]struct{}

func GroupIdsToSet(ids []hiddenpath.GroupId) GroupIdSet {
....
}

and then change the method to only accept GroupIdSet.
callers can then easily convert as list of groupids and you get the guarantee that you can only have each group once.

[lukedirtwalker]

Reviewed 1 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[chaehni]

Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @lukedirtwalker)

---

go/hidden_path_srv/internal/hpsegreq/registry_algo.go, line 102 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Ah sorry nevermind me. Though I wonder from an API perspective it would be nicer if as much invalid input as possible would already be catched by the compiler. So I suggest to add:

type GroupIdSet map[hiddenpath.GroupId]struct{}

func GroupIdsToSet(ids []hiddenpath.GroupId) GroupIdSet {
....
}

and then change the method to only accept GroupIdSet.
callers can then easily convert as list of groupids and you get the guarantee that you can only have each group once.

Done!

[lukedirtwalker]

Reviewed 2 of 2 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

[lukedirtwalker]

Reviewable status: complete! all files reviewed, all discussions resolved