pkg/private/serrors: act the constructors deprecations

[jiceatscion]

Implement the deprecation of the serrors constructors.

Wrap() -> JoinNoStack()
WithCtx() -> WrapNoStack()/JoinNoStack()
WrapStr() -> Wrap()

To apply the matching changes to other go src code, use "gopatch -p patchfile ./..." where patchfile is one of (in that order):

@@
@@
- serrors.WithCtx(...)
+ serrors.WrapNoStack("error", ...)

@@
@@
- serrors.Wrap(...)
+ serrors.JoinNoStack(...)

@@
@@
- serrors.WrapStr(...)
+ serrors.Wrap(...)

[jiceatscion]

This change is 

[jiceatscion]

Most of it is mechanical, with the exception of chosing between WrapNoStack and JoinNoStack as the replacement for WithCtx. In most cases, JoinNoStack was the better choice as the wrapped error was a sentinel error. I also tried to improve the doc string to help user chose the most appropriate constructor.

Reviewable status: 0 of 214 files reviewed, all discussions resolved (waiting on @lukedirtwalker and @oncilla)

[oncilla]

Reviewable status: 0 of 214 files reviewed, 2 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

a discussion (no related file):
In https://github.com/scionproto/scion/releases/tag/v0.9.0 we attached a gopatch file to simplify transition for our downstreams.
I think it would be cool to do it here too.

---

a discussion (no related file):
Please update the title format to fit the usual pattern

---

[jiceatscion]

Reviewable status: 0 of 214 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

In https://github.com/scionproto/scion/releases/tag/v0.9.0 we attached a gopatch file to simplify transition for our downstreams.
I think it would be cool to do it here too.

The conversion with goPatch isn't 100%; there's the choice between Wrap and Join (although wrap is always OK as a starting point), and there are some test function names (like ExampleXyz() that I fixed by hand. But I can include the patch files.Where would I place them? In the PR description text?

---

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

Please update the title format to fit the usual pattern

Done Don't bother reminding. There's a automated check nowadays.

---

[oncilla]

Reviewable status: 0 of 214 files reviewed, 5 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

pkg/private/serrors/errors.go line 180 at r1 (raw file):

It can be used without an underlying
// cause by passing nil as the cause. In that case this behaves like New(), except that the
// underlying type of the returned interface is basicError, rather than a *basicError.

Do we really want to mention this. For one, basicError is package internal, and it is also not the intended use. The package client does not need to use this.

In fact, I think we should extend our linter to catch obviously wrong things like serrors.Wrap("msg", nil, ...)

---

pkg/private/serrors/errors.go line 188 at r1 (raw file):

// instead use
//
//	Join(sentinel, nil, ...)

You can use https://tip.golang.org/doc/comment#doclinks [Join] to link to Join from the prose text above

---

pkg/private/serrors/errors.go line 254 at r1 (raw file):

// true.
//
// This is best used as an alternative to Wrap when deriving an error from a sentinel error. If

here and below

Suggestion:

 [Wrap]

[oncilla]

Reviewable status: 0 of 214 files reviewed, 4 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

a discussion (no related file):
What we have done in the past for ambiguous choices is to force a compiler error and let the user resolve it:

##@@
##var x expression
##@@
##-x.Source.Host.IP()
##+x.Source.Host.XXX_MANUAL_INTERVENTION_REQUIRED_IP_NOW_PANICS()

Yes, just list it in the PR description and we will include it in the release notes.

(like ExampleXyz() that I fixed by hand.

API users don't really care about that. I think you only need to create the patch for the public API of the package.

---

[jiceatscion]

Reviewable status: 0 of 214 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 180 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

It can be used without an underlying
// cause by passing nil as the cause. In that case this behaves like New(), except that the
// underlying type of the returned interface is basicError, rather than a *basicError.

Do we really want to mention this. For one, basicError is package internal, and it is also not the intended use. The package client does not need to use this.

In fact, I think we should extend our linter to catch obviously wrong things like serrors.Wrap("msg", nil, ...)

I wouldn't go that far. I updated the comment to indicate that there's rarely a reason to do it, but it is still the only way to get the same as from New() but as a value rather than pointer.

---

pkg/private/serrors/errors.go line 188 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

You can use https://tip.golang.org/doc/comment#doclinks [Join] to link to Join from the prose text above

done

---

pkg/private/serrors/errors.go line 254 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

here and below

done

[jiceatscion]

Reviewable status: 0 of 214 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

What we have done in the past for ambiguous choices is to force a compiler error and let the user resolve it:

##@@
##var x expression
##@@
##-x.Source.Host.IP()
##+x.Source.Host.XXX_MANUAL_INTERVENTION_REQUIRED_IP_NOW_PANICS()

Yes, just list it in the PR description and we will include it in the release notes.

(like ExampleXyz() that I fixed by hand.

API users don't really care about that. I think you only need to create the patch for the public API of the package.

Nah, not for this. Migrating to Wrap is close enough to good. The other choice really is an embellishment. The shim was getting away with wrapStr well enough after all.

---

[jiceatscion]

Reviewable status: 0 of 214 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

pkg/private/serrors/errors.go line 180 at r1 (raw file):

Previously, jiceatscion wrote…

I wouldn't go that far. I updated the comment to indicate that there's rarely a reason to do it, but it is still the only way to get the same as from New() but as a value rather than pointer.

Tentatively resolving.

[oncilla]

Reviewed 213 of 214 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 34 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

gateway/control/sessionpolicy.go line 115 at r2 (raw file):

	p, err := parser.Parse(ctx, raw)
	if err != nil {
		return nil, serrors.WrapNoStack("error", err, "file", file)

This should have been a wrap in the first place.

Suggestion:

serrors.Wrap("parsing", err, "file", file)

---

pkg/experimental/hiddenpath/discovery.go line 106 at r2 (raw file):

	a, err := extractAddr(hps)
	if err != nil {
		return nil, serrors.WrapNoStack("error", err, "isd_as", ia)

This should have been a wrap in the first place.

Suggestion:

return nil, serrors.Wrap("extracting address", err, "isd_as", ia)

---

pkg/experimental/hiddenpath/group.go line 188 at r2 (raw file):

	c, err := config.LoadResource(location)
	if err != nil {
		return nil, serrors.WrapNoStack("error", err, "location", location)

Should have been a Wrap in the first place.

Suggestion:

return nil, serrors.Wrap("reading", err, "location", location)

---

pkg/experimental/hiddenpath/registrationpolicy.go line 99 at r2 (raw file):

	c, err := config.LoadResource(location)
	if err != nil {
		return nil, nil, serrors.WrapNoStack("error", err, "location", location)

ditto

Suggestion:

return nil, nil, serrors.Wrap("reading", err, "location", location)

---

pkg/private/serrors/errors.go line 188 at r1 (raw file):

Previously, jiceatscion wrote…

done

I don't think it works in code blocks (but I might be wrong). I was thinking more along the lines:

// To enrich a sentinel error with context only, do not use [Wrap]
//
//	Wrap("dummy message", sentinel, ...)
//
// instead use [Join]
//
//	Join(sentinel, nil, ...)

---

pkg/private/serrors/errors.go line 184 at r2 (raw file):

// difference is the underlying type of the returned interface.
//
// To enrich a sentinel error, do not use

nit: This makes it sound like we should never use Wrap for sentinel errors, but there are cases where it is actually preferred IMO.

Suggestion:

To enrich a sentinel error with context only, do not use

---

pkg/private/serrors/errors_test.go line 349 at r2 (raw file):

	// ErrNoSpace is an error defined at package scope. It should be an error from a lower layer,
	// with some context information attached.
	var ErrNoSpace = serrors.New("no space", "dev", "sd0")

nit: ErrorNoSpace makes it seem like this is a sentinel error, but given it has context it is unlikely to be sentinel.

I would change this example to be closer to the intended use of Join:
Adding sentinel error based on an error cause that we got.

Maybe something like this:

var cause = fmt.Errorf("sd0 unresponsive: %w", io.ErrNoProgress)
var ErrDB = errors.New("db")

wrapped := serros.Join(ErrDB, cause, "ctx", 1)

fmt.Println(errors.Is(wrapped, ErrDB))
fmt.Println(errors.Is(wrapped, io.ErrNoProgress))

This focuses on our intended use of Join. The second check against io.ErrNoProgress is just to showcase that we can get the underlying cause to. But we should focus more on our sentinel error here, which is ErrDB.

---

pkg/private/serrors/errors_test.go line 351 at r2 (raw file):

	var ErrNoSpace = serrors.New("no space", "dev", "sd0")
	// ErrDB is an error defined at package scope. Here it figures a sentinel error.
	var ErrDB = serrors.New("db")

nit: serrors.New should not be used to construct sentinel errors.
It adds a stacktrace which is simply noise.

It will also surpress stack trace during joining/wrapping, which is not the intended behavior.

I think we should use our preferred way of creating sentinel errors, which is errors.New or fmt.Errorf.

Suggestion:

var ErrDB = errors.New("db")

---

pkg/private/serrors/errors_test.go line 369 at r2 (raw file):

func ExampleWrapNoStack() {
	// ErrBadL4 is an error defined at package scope.
	var ErrBadL4 = serrors.New("Unsupported L4 protocol")

Suggestion:

var ErrBadL4 = errors.New("unsupported L4 protocol")

---

pkg/private/serrors/errors_test.go line 370 at r2 (raw file):

	// ErrBadL4 is an error defined at package scope.
	var ErrBadL4 = serrors.New("Unsupported L4 protocol")
	addedCtx := serrors.WrapNoStack("error", ErrBadL4, "type", "SCTP")

nit: let's use some values that are closer to the real usage.

Suggestion:

addedCtx := serrors.WrapNoStack("parsing packet", ErrBadL4, "type", "SCTP")

---

pkg/private/serrors/errors_test.go line 379 at r2 (raw file):

func ExampleJoinNoStack() {
	// errMsg is a sentinel error.
	var brokenPacket = serrors.New("broken")

nit: Let's use some values closer to real usage.

Suggestion:

var brokenPacket = errors.New("invalid packet")

---

pkg/private/serrors/errors_test.go line 381 at r2 (raw file):

	var brokenPacket = serrors.New("broken")
	// ErrBadL4 is an error defined at package scope.
	var ErrBadL4 = serrors.New("Unsupported L4 protocol")

Suggestion:

var ErrBadL4 = errors.New("Unsupported L4 protocol")

---

pkg/scrypto/cppki/certs.go line 164 at r2 (raw file):

					"trc_base", trc.ID.Base,
					"trc_serial", trc.ID.Serial,
				),

nit: revert formatting

Code quote:

					"trc_base", trc.ID.Base,
					"trc_serial", trc.ID.Serial,
				),

---

private/ca/renewal/ca_signer_gen.go line 246 at r2 (raw file):

func (l CACertLoader) CACerts(ctx context.Context) ([]*x509.Certificate, error) {
	if _, err := os.Stat(l.Dir); err != nil {
		return nil, serrors.WrapNoStack("error", err, "dir", l.Dir)

Suggestion:

return nil, serrors.Wrap("stating directory", err, "dir", l.Dir)

---

private/ca/renewal/ca_signer_gen.go line 250 at r2 (raw file):

	files, err := filepath.Glob(fmt.Sprintf("%s/*.crt", l.Dir))
	if err != nil {
		return nil, serrors.WrapNoStack("error", err, "dir", l.Dir)

Suggestion:

serrors.Wrap("searching certificates", err, "dir", l.Dir)

---

private/ca/renewal/request.go line 171 at r2 (raw file):

				"after verification failure with latest TRC", err,
				"trc_id", trc.TRC.ID,
				"grace_trc_id", graceID,

nit: undo formatting change

---

private/mgmtapi/cppki/api/api.go line 61 at r2 (raw file):

			q.IA = ia
		} else {
			errs = append(errs, serrors.WrapNoStack("error", err, "parameter", "isd_as"))

Suggestion:

serrors.Wrap("parsing isd_as", err, "input", *params.IsdAS)

---

private/trust/store.go line 50 at r2 (raw file):

func LoadChains(ctx context.Context, dir string, db DB) (LoadResult, error) {
	if _, err := os.Stat(dir); err != nil {
		return LoadResult{}, serrors.WrapNoStack("error", err, "dir", dir)

Suggestion:

 serrors.Wrap("stating directory", err, "dir", dir)

---

private/trust/store.go line 55 at r2 (raw file):

	files, err := filepath.Glob(fmt.Sprintf("%s/*.pem", dir))
	if err != nil {
		return LoadResult{}, serrors.WrapNoStack("error", err, "dir", dir)

Suggestion:

 serrors.Wrap("searching cerrtificates", err, "dir", dir)

---

private/trust/store.go line 119 at r2 (raw file):

func LoadTRCs(ctx context.Context, dir string, db DB) (LoadResult, error) {
	if _, err := os.Stat(dir); err != nil {
		return LoadResult{}, serrors.WrapNoStack("error", err, "dir", dir)

You know the drill, here and below:

---

private/trust/verifier.go line 115 at r2 (raw file):

			"query.isd_as", query.IA,
			"query.subject_key_id", fmt.Sprintf("%x", query.SubjectKeyID),
			"query.validity", query.Validity.String())

nit: undo formatting change

---

private/underlay/conn/conn.go line 197 at r2 (raw file):

			return serrors.Wrap("Error getting SO_SNDBUF socket option (before)", err,
				"listen", laddr,
				"remote", raddr)

nit: formatting

---

private/underlay/conn/conn.go line 204 at r2 (raw file):

			return serrors.Wrap("Error setting send buffer size", err,
				"listen", laddr,
				"remote", raddr)

nit: formatting

---

private/underlay/conn/conn.go line 211 at r2 (raw file):

			return serrors.Wrap("Error getting SO_SNDBUF socket option (after)", err,
				"listen", laddr,
				"remote", raddr)

nit: formatting

---

private/underlay/conn/conn.go line 238 at r2 (raw file):

			return serrors.Wrap("Error setting recv buffer size", err,
				"listen", laddr,
				"remote", raddr)

nit: formatting

---

private/underlay/conn/conn.go line 245 at r2 (raw file):

			return serrors.Wrap("Error getting SO_RCVBUF socket option (after)", err,
				"listen", laddr,
				"remote", raddr)

nit: formatting

---

scion-pki/conf/trc.go line 70 at r2 (raw file):

		read, err := cppki.ReadPEMCerts(certFile)
		if err != nil {
			return nil, serrors.WrapNoStack("error", err, "file", certFile)

Suggestion:

 serrors.Wrap("reading certificate", err, "file", certFile)

---

scion-pki/conf/trc.go line 75 at r2 (raw file):

			ct, err := cppki.ValidateCert(cert)
			if err != nil {
				return nil, serrors.WrapNoStack("error", err, "file", certFile)

Suggestion:

 serrors.Wrap("validating certificate", err, "file", certFile)

---

scion-pki/testcrypto/testcrypto.go line 489 at r2 (raw file):

		_, name := filepath.Split(trc)
		if err := copyFile(filepath.Join(out.base, "trcs", name), trc); err != nil {
			return serrors.WrapNoStack("error", err, "file", trc)

Suggestion:

return serrors.Wrap("copying", err, "file", trc)

---

scion-pki/testcrypto/testcrypto.go line 509 at r2 (raw file):

		_, name := filepath.Split(file)
		if err := copyFile(filepath.Join(out.base, "certs", name), file); err != nil {
			return serrors.WrapNoStack("error", err, "file", file)

Suggestion:

 serrors.Wrap("copying", err, "file", file)

---

scion-pki/testcrypto/update.go line 382 at r2 (raw file):

			return cppki.SignedTRC{}, serrors.WrapNoStack("error", err,
				"common_name", cert.Subject.CommonName)
		}

Suggestion:

		if err := sd.AddSignerInfo([]*x509.Certificate{cert}, key); err != nil {
			return cppki.SignedTRC{}, serrors.Wrap("adding signer info", err,
				"common_name", cert.Subject.CommonName)
		}

---

scion-pki/trcs/verify.go line 133 at r2 (raw file):

	certs, err := loadAnchorCerts(anchor)
	if err != nil {
		return serrors.WrapNoStack("error", err, "anchor", anchor)

Suggestion:

serrors.WrapNoStack("loading anchor", err, "anchor", anchor)

---

scion-pki/trcs/verify.go line 165 at r2 (raw file):

	for i, si := range signed.SignerInfos {
		if err := verifySignerInfo(si, signed.TRC.Raw, certs); err != nil {
			return serrors.WrapNoStack("error", err, "index", i)

Suggestion:

serrors.WrapNoStack("verifying signer info", err, "index", i)

---

tools/braccept/runner/run.go line 127 at r2 (raw file):

			}
			// Timeout receiving packets
			return serrors.WrapNoStack("error", errTimeout, "other err", errors.ToError())

Isn't this a use case for serrors.Join?

---

tools/end2end_integration/main.go line 259 at r2 (raw file):

				})
				if err != nil {
					err = serrors.WrapNoStack("error", err, "file", relFile(logFile))

Suggestion:

err = serrors.Wrap("running integration", err, "file", relFile(logFile))

[oncilla]

Reviewable status: all files reviewed, 36 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

control/beaconing/extender.go line 136 at r2 (raw file):

			return serrors.Wrap(
				"calculating expiry time from signer expiration time", err,
				"signer_expiration", signerExp)

nit: undo formatting change.

---

control/beaconing/extender.go line 239 at r2 (raw file):

		return seg.HopEntry{}, nil, serrors.Wrap("checking remote ingress interface (mtu)", err,
			"interfaces", ingress)

There are a lot of places where you introduced an empty new line.
Was this intentional or rather a search/replace gone wrong?

[jiceatscion]

Reviewable status: 194 of 215 files reviewed, 22 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

control/beaconing/extender.go line 136 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: undo formatting change.

Yeah, I'm afraid that's gopatch handy work. I will assume that you have pointed out every change you wanted undone, so I won't recheck 215 files redundantly. Let me know if that is not the case.

---

control/beaconing/extender.go line 239 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

There are a lot of places where you introduced an empty new line.
Was this intentional or rather a search/replace gone wrong?

Some of the tests are change detectors of the worst kind. They compare stack dumps. So, one line offset can require updating a dozen expected stack dumps! In those cases I actively tried to keep line numbers unchanged. Many new lines are indeed added by gopatch; I haven't found what rules it follows in adding or not adding new lines.

---

gateway/control/sessionpolicy.go line 115 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

This should have been a wrap in the first place.

Done.

---

pkg/experimental/hiddenpath/discovery.go line 106 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

This should have been a wrap in the first place.

Done.

---

pkg/experimental/hiddenpath/group.go line 188 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Should have been a Wrap in the first place.

Done.

---

pkg/experimental/hiddenpath/registrationpolicy.go line 99 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

ditto

Done.

---

pkg/private/serrors/errors.go line 188 at r1 (raw file):

Previously, oncilla (Dominik Roos) wrote…

I don't think it works in code blocks (but I might be wrong). I was thinking more along the lines:

// To enrich a sentinel error with context only, do not use [Wrap]
//
//	Wrap("dummy message", sentinel, ...)
//
// instead use [Join]
//
//	Join(sentinel, nil, ...)

done. Also cleaned-up other docstrings.

---

pkg/private/serrors/errors.go line 184 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: This makes it sound like we should never use Wrap for sentinel errors, but there are cases where it is actually preferred IMO.

Agreed. Clarified.

---

pkg/private/serrors/errors_test.go line 349 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: ErrorNoSpace makes it seem like this is a sentinel error, but given it has context it is unlikely to be sentinel.

I would change this example to be closer to the intended use of Join:
Adding sentinel error based on an error cause that we got.

Maybe something like this:

var cause = fmt.Errorf("sd0 unresponsive: %w", io.ErrNoProgress)
var ErrDB = errors.New("db")

wrapped := serros.Join(ErrDB, cause, "ctx", 1)

fmt.Println(errors.Is(wrapped, ErrDB))
fmt.Println(errors.Is(wrapped, io.ErrNoProgress))

This focuses on our intended use of Join. The second check against io.ErrNoProgress is just to showcase that we can get the underlying cause to. But we should focus more on our sentinel error here, which is ErrDB.

Agree. The examples are a bit too contrived.

---

pkg/private/serrors/errors_test.go line 351 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: serrors.New should not be used to construct sentinel errors.
It adds a stacktrace which is simply noise.

It will also surpress stack trace during joining/wrapping, which is not the intended behavior.

I think we should use our preferred way of creating sentinel errors, which is errors.New or fmt.Errorf.

Done, at least in the examples. I didn't go through all the test cases to find every case of some serrors.New() that could be changed to errors.New()

---

pkg/private/serrors/errors_test.go line 369 at r2 (raw file):

func ExampleWrapNoStack() {
	// ErrBadL4 is an error defined at package scope.
	var ErrBadL4 = serrors.New("Unsupported L4 protocol")

Done.

---

pkg/private/serrors/errors_test.go line 381 at r2 (raw file):

	var brokenPacket = serrors.New("broken")
	// ErrBadL4 is an error defined at package scope.
	var ErrBadL4 = serrors.New("Unsupported L4 protocol")

Done.

---

pkg/scrypto/cppki/certs.go line 164 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: revert formatting

done

---

private/ca/renewal/ca_signer_gen.go line 246 at r2 (raw file):

func (l CACertLoader) CACerts(ctx context.Context) ([]*x509.Certificate, error) {
	if _, err := os.Stat(l.Dir); err != nil {
		return nil, serrors.WrapNoStack("error", err, "dir", l.Dir)

Done.

---

private/ca/renewal/ca_signer_gen.go line 250 at r2 (raw file):

	files, err := filepath.Glob(fmt.Sprintf("%s/*.crt", l.Dir))
	if err != nil {
		return nil, serrors.WrapNoStack("error", err, "dir", l.Dir)

Done.

---

private/ca/renewal/request.go line 171 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: undo formatting change

done

---

private/mgmtapi/cppki/api/api.go line 61 at r2 (raw file):

			q.IA = ia
		} else {
			errs = append(errs, serrors.WrapNoStack("error", err, "parameter", "isd_as"))

done

---

private/trust/store.go line 50 at r2 (raw file):

func LoadChains(ctx context.Context, dir string, db DB) (LoadResult, error) {
	if _, err := os.Stat(dir); err != nil {
		return LoadResult{}, serrors.WrapNoStack("error", err, "dir", dir)

done

---

private/trust/store.go line 55 at r2 (raw file):

	files, err := filepath.Glob(fmt.Sprintf("%s/*.pem", dir))
	if err != nil {
		return LoadResult{}, serrors.WrapNoStack("error", err, "dir", dir)

done

---

private/trust/store.go line 119 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

You know the drill, here and below:

done

---

private/trust/verifier.go line 115 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: undo formatting change

done

---

private/underlay/conn/conn.go line 197 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

nit: formatting

done

---

scion-pki/conf/trc.go line 70 at r2 (raw file):

		read, err := cppki.ReadPEMCerts(certFile)
		if err != nil {
			return nil, serrors.WrapNoStack("error", err, "file", certFile)

Done.

---

scion-pki/conf/trc.go line 75 at r2 (raw file):

			ct, err := cppki.ValidateCert(cert)
			if err != nil {
				return nil, serrors.WrapNoStack("error", err, "file", certFile)

Done.

---

scion-pki/testcrypto/testcrypto.go line 489 at r2 (raw file):

		_, name := filepath.Split(trc)
		if err := copyFile(filepath.Join(out.base, "trcs", name), trc); err != nil {
			return serrors.WrapNoStack("error", err, "file", trc)

Done.

---

scion-pki/testcrypto/testcrypto.go line 509 at r2 (raw file):

		_, name := filepath.Split(file)
		if err := copyFile(filepath.Join(out.base, "certs", name), file); err != nil {
			return serrors.WrapNoStack("error", err, "file", file)

Done.

---

scion-pki/testcrypto/update.go line 382 at r2 (raw file):

			return cppki.SignedTRC{}, serrors.WrapNoStack("error", err,
				"common_name", cert.Subject.CommonName)
		}

Done.

---

scion-pki/trcs/verify.go line 133 at r2 (raw file):

	certs, err := loadAnchorCerts(anchor)
	if err != nil {
		return serrors.WrapNoStack("error", err, "anchor", anchor)

Done This was also the only one in the vicinity that wasn't adding a stack. I changed it to Wrap because I couldn't see any reason to want a stack for the others and not that one.

---

scion-pki/trcs/verify.go line 165 at r2 (raw file):

	for i, si := range signed.SignerInfos {
		if err := verifySignerInfo(si, signed.TRC.Raw, certs); err != nil {
			return serrors.WrapNoStack("error", err, "index", i)

Done.

---

tools/braccept/runner/run.go line 127 at r2 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Isn't this a use case for serrors.Join?

The other errors aren't necessarily the cause, so I assume you mean Join(timeout, nil,...), If so, done.

---

tools/end2end_integration/main.go line 259 at r2 (raw file):

				})
				if err != nil {
					err = serrors.WrapNoStack("error", err, "file", relFile(logFile))

Done.

[oncilla]

Reviewed 29 of 29 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jiceatscion and @lukedirtwalker)

---

control/beaconing/extender.go line 136 at r2 (raw file):

Previously, jiceatscion wrote…

Yeah, I'm afraid that's gopatch handy work. I will assume that you have pointed out every change you wanted undone, so I won't recheck 215 files redundantly. Let me know if that is not the case.

If we miss some we miss some 😆
I also don't want to recheck... 🏃‍♂️

---

private/segment/segverifier/segverifier.go line 162 at r3 (raw file):

		if err := segment.VerifyASEntry(ctx, verifier, i); err != nil {
			return serrors.JoinNoStack(ErrSegment, err,
				"seg", segment.String(), "as", asEntry.Local)

Q: With #4597 this should not be necessary anymore because the segment implements the stringer interface. Did something not look as expected?

Code quote:

segment.String()

---

private/trust/store.go line 132 at r3 (raw file):

		raw, err := os.ReadFile(f)
		if err != nil {
			return res, serrors.WrapNoStack("error", err, "file", f)

ditto

---

private/trust/store.go line 140 at r3 (raw file):

		trc, err := cppki.DecodeSignedTRC(raw)
		if err != nil {
			return res, serrors.WrapNoStack("error", err, "file", f)

ditto

---

private/trust/store.go line 148 at r3 (raw file):

		inserted, err := db.InsertTRC(ctx, trc)
		if err != nil {
			return res, serrors.WrapNoStack("error", err, "file", f)

ditto

---

tools/braccept/runner/run.go line 127 at r2 (raw file):

Previously, jiceatscion wrote…

The other errors aren't necessarily the cause, so I assume you mean Join(timeout, nil,...), If so, done.

Ah. Yeah, I missed that bit. But Join is more appropriate anyway 👍

[jiceatscion]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

private/segment/segverifier/segverifier.go line 162 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Q: With #4597 this should not be necessary anymore because the segment implements the stringer interface. Did something not look as expected?

That's the result of merging from master. It came from you: #4587

---

private/trust/store.go line 132 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

ditto

Done.

---

private/trust/store.go line 140 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

ditto

Done.

---

private/trust/store.go line 148 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

ditto

Done.

[jiceatscion]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

private/segment/segverifier/segverifier.go line 162 at r3 (raw file):

Previously, jiceatscion wrote…

That's the result of merging from master. It came from you: #4587

Want me to undo it here?

[oncilla]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @lukedirtwalker)

---

private/segment/segverifier/segverifier.go line 162 at r3 (raw file):

Previously, jiceatscion wrote…

Want me to undo it here?

Ha, look at my memory 😆 🤦

I think it would be good to revert since we are already here.

(making this comment non-blocking as it is unrelated to the PR.)

[oncilla]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)

[jiceatscion]

Reviewable status: 215 of 216 files reviewed, all discussions resolved (waiting on @lukedirtwalker and @oncilla)

---

private/segment/segverifier/segverifier.go line 162 at r3 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Ha, look at my memory 😆 🤦

I think it would be good to revert since we are already here.

(making this comment non-blocking as it is unrelated to the PR.)

Well...don't look at mine. ... and done.

[jiceatscion]

Reviewed 187 of 214 files at r1, 27 of 29 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)