Repository: scionproto/scion
Pull request #4770 (2025-05-15 09:27:30)

chore: fix typos
Fixing some typos and very minor code cleanups in the `beacon` package.

jiceatscion: 

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/4770)
<!-- Reviewable:end -->

romshark: 




*[`control/beacon/store.go` line 133 at r2](https://reviewable.io/reviews/scionproto/scion/4770#-OQIeFJmA2-rtlB7fz4O:-OQIeFJmA2-rtlB7fz4P:b-gub7ki) ([raw file](https://github.com/scionproto/scion/blob/a7ad79a02886c712a4ca2deee1fd13eec7f2a7be/control/beacon/store.go#L133)):*
> ```Go
> 
> // CoreStore provides abstracted access to the beacon database in a core AS. The
> // store helps to insert beacons and revocations, and selects the best beacons
> ```

I believe the right form is "helps insert" without the "to". AFAIK "helps to" technically isn't wrong but less idiomatic.


<!-- Sent from Reviewable.io -->

katyatitkova: 




*[`control/beacon/store.go` line 133 at r2](https://reviewable.io/reviews/scionproto/scion/4770#-OQIeFJmA2-rtlB7fz4O:-OQIlQNA1v0twfAZkCk3:bvntwhy) ([raw file](https://github.com/scionproto/scion/blob/a7ad79a02886c712a4ca2deee1fd13eec7f2a7be/control/beacon/store.go#L133)):*
<details><summary><i>Previously, romshark (Roman Sharkov) wrote…</i></summary><blockquote>

I believe the right form is "helps insert" without the "to". AFAIK "helps to" technically isn't wrong but less idiomatic.
</blockquote></details>

I used Cambridge Dictionary after my IDE suggested the initial change (as I previously heard the wrong "help doing" form quite a lot), and it seems to tell that both forms are equally correct: https://dictionary.cambridge.org/grammar/british-grammar/help-somebody-to-do


<!-- Sent from Reviewable.io -->

romshark: 




*[`control/beacon/store.go` line 133 at r2](https://reviewable.io/reviews/scionproto/scion/4770#-OQIeFJmA2-rtlB7fz4O:-OQIrljMDXvaYUHiby5-:b8cc6k5) ([raw file](https://github.com/scionproto/scion/blob/a7ad79a02886c712a4ca2deee1fd13eec7f2a7be/control/beacon/store.go#L133)):*
<details><summary><i>Previously, katyatitkova (Katya Titkova) wrote…</i></summary><blockquote>

I used Cambridge Dictionary after my IDE suggested the initial change (as I previously heard the wrong "help doing" form quite a lot), and it seems to tell that both forms are equally correct: https://dictionary.cambridge.org/grammar/british-grammar/help-somebody-to-do
</blockquote></details>

I'm fine with either :+1: 


<!-- Sent from Reviewable.io -->


Patch:
diff --git a/control/beacon/BUILD.bazel b/control/beacon/BUILD.bazel
index 673472347..c34899861 100644
--- a/control/beacon/BUILD.bazel
+++ b/control/beacon/BUILD.bazel
@@ -44,6 +44,7 @@ go_test(
         ":go_default_library",
         "//control/beacon/mock_beacon:go_default_library",
         "//pkg/addr:go_default_library",
+        "//pkg/private/ptr:go_default_library",
         "//pkg/private/xtest/graph:go_default_library",
         "//pkg/segment:go_default_library",
         "@com_github_golang_mock//gomock:go_default_library",
diff --git a/control/beacon/chain_checker.go b/control/beacon/chain_checker.go
index 276952080..7eef6d04f 100644
--- a/control/beacon/chain_checker.go
+++ b/control/beacon/chain_checker.go
@@ -43,7 +43,7 @@ const (
 	defaultCacheMissExpiration = 30 * time.Second
 )
 
-var _ infra.Verifier = (chainChecker{})
+var _ infra.Verifier = chainChecker{}
 
 // chainChecker checks that the certificate chain is available locally. This is used
 // to ensure we do not propagate beacons that are not verifiable.
diff --git a/control/beacon/db.go b/control/beacon/db.go
index 6d85fb4ac..e7a3a943c 100644
--- a/control/beacon/db.go
+++ b/control/beacon/db.go
@@ -44,7 +44,7 @@ type DB interface {
 	) ([]Beacon, error)
 	// BeaconSources returns all source ISD-AS of the beacons in the database.
 	BeaconSources(ctx context.Context) ([]addr.IA, error)
-	// Insert inserts a beacon with its allowed usage into the database.
+	// InsertBeacon inserts a beacon with its allowed usage into the database.
 	InsertBeacon(ctx context.Context, beacon Beacon, usage Usage) (InsertStats, error)
 }
 
@@ -78,7 +78,7 @@ func UsageFromPolicyType(policyType PolicyType) Usage {
 	}
 }
 
-// None indicates whether the beacons is not allowed to be used anywhere.
+// None indicates whether the beacons are not allowed to be used anywhere.
 func (u Usage) None() bool {
 	return u&0x0F == 0
 }
diff --git a/control/beacon/policy.go b/control/beacon/policy.go
index 44b2fa8bd..6fdb6bf14 100644
--- a/control/beacon/policy.go
+++ b/control/beacon/policy.go
@@ -17,7 +17,7 @@ package beacon
 import (
 	"os"
 
-	yaml "gopkg.in/yaml.v2"
+	"gopkg.in/yaml.v2"
 
 	"github.com/scionproto/scion/pkg/addr"
 	"github.com/scionproto/scion/pkg/private/ptr"
diff --git a/control/beacon/selection_algo.go b/control/beacon/selection_algo.go
index a5c49b974..d4724b294 100644
--- a/control/beacon/selection_algo.go
+++ b/control/beacon/selection_algo.go
@@ -25,7 +25,7 @@ import (
 )
 
 type selectionAlgorithm interface {
-	// SelectBeacons selects the `n` best beacons from the provided slice of beacons.
+	// SelectBeacons selects the `resultSize` best beacons from the provided slice of beacons.
 	SelectBeacons(ctx context.Context, beacons []Beacon, resultSize int) []Beacon
 }
 
@@ -34,9 +34,9 @@ type selectionAlgorithm interface {
 type baseAlgo struct{}
 
 // SelectBeacons implements a very simple selection algorithm. The best beacon
-// is the one with a shortest path. The slice contains the k-1 shortest
+// is the one with the shortest path. The slice contains the resultSize-1 shortest
 // beacons. The last beacon is either the most diverse beacon from the remaining
-// beacons, if the diversity exceeds what has already been served. Or the
+// beacons, if the diversity exceeds what has already been served, or the
 // shortest remaining beacon, otherwise.
 func (a baseAlgo) SelectBeacons(_ context.Context, beacons []Beacon, resultSize int) []Beacon {
 	if len(beacons) <= resultSize {
@@ -52,8 +52,7 @@ func (a baseAlgo) SelectBeacons(_ context.Context, beacons []Beacon, resultSize
 	if diversityRest > diversity {
 		return append(result, mostDiverseRest)
 	}
-	// If the most diverse beacon was already served, serve shortest from the
-	// rest.
+	// If the most diverse beacon was already served, serve shortest from the rest.
 	return append(result, beacons[resultSize-1])
 }
 
diff --git a/control/beacon/store.go b/control/beacon/store.go
index f6c04312d..989ec4f63 100644
--- a/control/beacon/store.go
+++ b/control/beacon/store.go
@@ -59,7 +59,7 @@ func applyStoreOptions(opts []StoreOption) storeOptions {
 }
 
 // Store provides abstracted access to the beacon database in a non-core AS.
-// The store helps inserting beacons and revocations, and selects the best beacons
+// The store helps to insert beacons and revocations, and selects the best beacons
 // for given purposes based on the configured policies. It should not be used in a
 // core AS.
 type Store struct {
@@ -85,14 +85,14 @@ func NewBeaconStore(policies Policies, db DB, opts ...StoreOption) (*Store, erro
 	return s, nil
 }
 
-// BeaconsToPropagate returns a slice  all beacons to propagate at the time of the call.
+// BeaconsToPropagate returns a slice of all beacons to propagate at the time of the call.
 // The selection is based on the configured propagation policy.
 func (s *Store) BeaconsToPropagate(ctx context.Context) ([]Beacon, error) {
 	return s.getBeacons(ctx, &s.policies.Prop)
 }
 
 // SegmentsToRegister returns a channel that provides all beacons to register at
-// the time of the call. The selections is based on the configured policy for
+// the time of the call. The selections are based on the configured policy for
 // the requested segment type.
 func (s *Store) SegmentsToRegister(ctx context.Context, segType seg.Type) ([]Beacon, error) {
 	switch segType {
@@ -130,7 +130,7 @@ func (s *Store) MaxExpTime(policyType PolicyType) uint8 {
 }
 
 // CoreStore provides abstracted access to the beacon database in a core AS. The
-// store helps inserting beacons and revocations, and selects the best beacons
+// store helps to insert beacons and revocations, and selects the best beacons
 // for given purposes based on the configured policies. It should not be used in
 // a non-core AS.
 type CoreStore struct {
@@ -138,7 +138,7 @@ type CoreStore struct {
 	policies CorePolicies
 }
 
-// NewCoreBeaconStore creates a new beacon store for a non-core AS.
+// NewCoreBeaconStore creates a new beacon store for a core AS.
 func NewCoreBeaconStore(policies CorePolicies, db DB, opts ...StoreOption) (*CoreStore, error) {
 	policies.InitDefaults()
 	if err := policies.Validate(); err != nil {
@@ -163,7 +163,7 @@ func (s *CoreStore) BeaconsToPropagate(ctx context.Context) ([]Beacon, error) {
 }
 
 // SegmentsToRegister returns a slice of all beacons to register at the time of the call.
-// The selections is based on the configured policy for the requested segment type.
+// The selection is based on the configured policy for the requested segment type.
 func (s *CoreStore) SegmentsToRegister(ctx context.Context, segType seg.Type) ([]Beacon, error) {
 
 	if segType != seg.TypeCore {


Test patch:
diff --git a/control/beacon/beacondbtest/beacondbtest.go b/control/beacon/beacondbtest/beacondbtest.go
index 32e379b7c..ac229bb62 100644
--- a/control/beacon/beacondbtest/beacondbtest.go
+++ b/control/beacon/beacondbtest/beacondbtest.go
@@ -237,13 +237,13 @@ func testUpdateOlderIgnored(t *testing.T, db beacon.DB) {
 }
 
 func testCandidateBeacons(t *testing.T, db Testable) {
-	// Insert beacons from longest to shortest path such that the insertion
+	// Insert beacons from the longest to the shortest path such that the insertion
 	// order is not sorted the same as the expected outcome.
 	var beacons []beacon.Beacon
 	insertBeacons := func(t *testing.T, db beacon.DB) {
 		for i, info := range [][]IfInfo{Info3, Info2, Info1} {
 			b := InsertBeacon(t, db, info, 12, uint32(i), beacon.UsageProp)
-			// Prepend to get beacons sorted from shortest to longest path.
+			// Prepend to get beacons sorted from the shortest to the longest path.
 			beacons = append([]beacon.Beacon{b}, beacons...)
 		}
 	}
diff --git a/control/beacon/policy_test.go b/control/beacon/policy_test.go
index 7647d24e3..63d7b96f7 100644
--- a/control/beacon/policy_test.go
+++ b/control/beacon/policy_test.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/scionproto/scion/control/beacon"
 	"github.com/scionproto/scion/pkg/addr"
+	"github.com/scionproto/scion/pkg/private/ptr"
 	seg "github.com/scionproto/scion/pkg/segment"
 )
 
@@ -32,9 +33,6 @@ var (
 	ia210 = addr.MustParseIA("2-ff00:0:210")
 	ia310 = addr.MustParseIA("3-ff00:0:310")
 	ia311 = addr.MustParseIA("3-ff00:0:311")
-
-	false_val = false
-	true_val  = true
 )
 
 func TestLoadPolicyFromYaml(t *testing.T) {
@@ -83,7 +81,7 @@ func TestFilterApply(t *testing.T) {
 		MaxHopsLength: 2,
 		AsBlackList:   []addr.AS{ia112.AS()},
 		IsdBlackList:  []addr.ISD{2},
-		AllowIsdLoop:  &false_val,
+		AllowIsdLoop:  ptr.To(false),
 	}
 	testCases := []struct {
 		Name         string
@@ -110,39 +108,39 @@ func TestFilterApply(t *testing.T) {
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "Blacklisted ISD[2-ff00:0:210]",
+			Name:         "Blacklisted ISD: [2-ff00:0:210]",
 			Beacon:       newTestBeacon(ia210),
 			Filter:       defaultFilter,
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "AS Repeated [1-ff00:0:110, 1-ff00:0:110]",
+			Name:         "AS Repeated: [1-ff00:0:110, 1-ff00:0:110]",
 			Beacon:       newTestBeacon(ia110, ia110),
 			Filter:       defaultFilter,
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "AS loop [1-ff00:0:110, 1-ff00:0:111, 1-ff00:0:113, 1-ff00:0:110]",
+			Name:         "AS loop: [1-ff00:0:110, 1-ff00:0:111, 1-ff00:0:113, 1-ff00:0:110]",
 			Beacon:       newTestBeacon(ia110, ia111, ia113, ia110),
-			Filter:       &beacon.Filter{MaxHopsLength: 8, AllowIsdLoop: &false_val},
+			Filter:       &beacon.Filter{MaxHopsLength: 8, AllowIsdLoop: ptr.To(false)},
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "ISD loop [1-ff00:0:110, 3-ff00:0:310, 3-ff00:0:311, 1-ff00:0:111]",
+			Name:         "ISD loop: [1-ff00:0:110, 3-ff00:0:310, 3-ff00:0:311, 1-ff00:0:111]",
 			Beacon:       newTestBeacon(ia110, ia310, ia311, ia111),
-			Filter:       &beacon.Filter{MaxHopsLength: 8, AllowIsdLoop: &false_val},
+			Filter:       &beacon.Filter{MaxHopsLength: 8, AllowIsdLoop: ptr.To(false)},
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "ISD/AS Loop [1-ff00:0:110, 3-ff00:0:311, 1-ff00:0:110]",
+			Name:         "ISD/AS Loop: [1-ff00:0:110, 3-ff00:0:311, 1-ff00:0:110]",
 			Beacon:       newTestBeacon(ia110, ia311, ia110),
-			Filter:       &beacon.Filter{MaxHopsLength: 8, AllowIsdLoop: &true_val},
+			Filter:       &beacon.Filter{MaxHopsLength: 8, AllowIsdLoop: ptr.To(true)},
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "ISD Loop allowed [1-ff00:0:110, 3-ff00:0:311, 1-ff00:0:111]",
+			Name:         "ISD Loop allowed: [1-ff00:0:110, 3-ff00:0:311, 1-ff00:0:111]",
 			Beacon:       newTestBeacon(ia110, ia311, ia111),
-			Filter:       &beacon.Filter{MaxHopsLength: 8, AllowIsdLoop: &true_val},
+			Filter:       &beacon.Filter{MaxHopsLength: 8, AllowIsdLoop: ptr.To(true)},
 			ErrAssertion: assert.NoError,
 		},
 	}
@@ -162,29 +160,29 @@ func TestFilterLoop(t *testing.T) {
 		ErrAssertion assert.ErrorAssertionFunc
 	}{
 		{
-			Name:         "AS Repeated [1-ff00:0:110, 1-ff00:0:110]",
+			Name:         "AS Repeated: [1-ff00:0:110, 1-ff00:0:110]",
 			Beacon:       newTestBeacon(ia110),
 			Next:         ia110,
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "AS loop [1-ff00:0:110, 1-ff00:0:111, 1-ff00:0:113, 1-ff00:0:110]",
+			Name:         "AS loop: [1-ff00:0:110, 1-ff00:0:111, 1-ff00:0:113, 1-ff00:0:110]",
 			Beacon:       newTestBeacon(ia110, ia111, ia113, ia110),
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "ISD loop [1-ff00:0:110, 3-ff00:0:310, 3-ff00:0:311, 1-ff00:0:111]",
+			Name:         "ISD loop: [1-ff00:0:110, 3-ff00:0:310, 3-ff00:0:311, 1-ff00:0:111]",
 			Beacon:       newTestBeacon(ia110, ia310, ia311, ia111),
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "ISD/AS Loop [1-ff00:0:110, 3-ff00:0:311, 1-ff00:0:110]",
+			Name:         "ISD/AS Loop: [1-ff00:0:110, 3-ff00:0:311, 1-ff00:0:110]",
 			Beacon:       newTestBeacon(ia110, ia311, ia110),
 			AllowIsdLoop: true,
 			ErrAssertion: assert.Error,
 		},
 		{
-			Name:         "ISD Loop allowed [1-ff00:0:110, 3-ff00:0:311, 1-ff00:0:111]",
+			Name:         "ISD Loop allowed: [1-ff00:0:110, 3-ff00:0:311, 1-ff00:0:111]",
 			Beacon:       newTestBeacon(ia110, ia311, ia111),
 			AllowIsdLoop: true,
 			ErrAssertion: assert.NoError,
diff --git a/control/beacon/testdata/HPGCfg_ff00_0_110-69b5.json b/control/beacon/testdata/HPGCfg_ff00_0_110-69b5.json
deleted file mode 100644
index ac22774f2..000000000
--- a/control/beacon/testdata/HPGCfg_ff00_0_110-69b5.json
+++ /dev/null
@@ -1,18 +0,0 @@
-{
-    "group_id": "ff00:0:110-69b5",
-    "version": 1,
-    "owner": "1-ff00:0:110",
-    "writers": [
-        "1-ff00:0:111",
-        "1-ff00:0:112"
-    ],
-    "readers": [
-        "1-ff00:0:113",
-        "1-ff00:0:114"
-    ],
-    "registries": [
-        "1-ff00:0:110",
-        "1-ff00:0:111",
-        "1-ff00:0:113"
-    ]
-}
diff --git a/control/beacon/testdata/HPGCfg_ffaa_0_222-abcd.json b/control/beacon/testdata/HPGCfg_ffaa_0_222-abcd.json
deleted file mode 100644
index eb38c1a6b..000000000
--- a/control/beacon/testdata/HPGCfg_ffaa_0_222-abcd.json
+++ /dev/null
@@ -1,18 +0,0 @@
-{
-    "group_id": "ffaa:0:222-abcd",
-    "version": 1,
-    "owner": "1-ffaa:0:222",
-    "writers": [
-        "1-ff00:0:111",
-        "1-ff00:0:112"
-    ],
-    "readers": [
-        "1-ff00:0:113",
-        "1-ff00:0:114"
-    ],
-    "registries": [
-        "1-ff00:0:110",
-        "1-ff00:0:111",
-        "1-ff00:0:113"
-    ]
-}
diff --git a/control/beacon/testdata/hp_policy.yml b/control/beacon/testdata/hp_policy.yml
deleted file mode 100644
index cc0b0efc5..000000000
--- a/control/beacon/testdata/hp_policy.yml
+++ /dev/null
@@ -1,29 +0,0 @@
----
-HPGroups:
-  "ff00:0:110-69b5":
-    CfgFilePath: testdata/HPGCfg_ff00_0_110-69b5.json
-  "ffaa:0:222-abcd":
-    CfgFilePath: testdata/HPGCfg_ffaa_0_222-abcd.json
-SegmentRegistration:
-  DefaultAction: register
-  HiddenAndPublic: true
-  Policies:
-    2:
-      PS:
-        RegUp: true
-        RegDown: true
-        MaxExpiration: 1h
-      HPS:
-        "ff00:0:110-69b5":
-          RegUp: true
-          RegDown: true
-          MaxExpiration: 1h
-        "ffaa:0:222-abcd":
-          RegUp: true
-          RegDown: true
-          MaxExpiration: 1h
-    3:
-      PS:
-        RegUp: true
-        RegDown: true
-        MaxExpiration: 1h