Make bls.Signer api fallible

[richardpringle]

• Make ProofOfPossession creation fallible
• Make bls.Signer api fallible

Why this should be merged

If we want to add any networking in between the keys, we'll need a fallible API.

I've implemented the rpcsigner here as a grpc client with a server implemented in a _test.go file. The idea here is that anyone who implements the interface.

Open Question:

There's one little gotchya here for anyone implementing the interface. It's that we expect the keys and signatures to be compressed when sent over the wire. I'm open to using the more general serialization OR adding the compressed prefix to the proto-message field names.

TODO:

Further, we should probably add a minimal test suite that an external implementor can run, however, I think could probably be implemented in a follow up PR as it is not a priority at the moment.

How this works

How this was tested

Need to be documented in RELEASES.md?

[joshua-kim]

I feel weird about this code owning the lifecycle of the connection since it's provided by the caller. Some of these errors don't look persistent + you can still re-use the connection since you can retry rpcs on different streams on the same http/2 connection even if this errored out.

You could probably make a strong case for some of these errors being persistent (i.e the empty public key case, the invalid pk case), but I would still think that you could handle all of these errors at the call-site through a connection close and have all the connection lifecycle managed there instead of having mixed ownership

[joshua-kim]

Related to my last comment but I think this can be removed as well

[StephenButtolph]

I think we should try to be in the habit of implementing helpers in better places. So if we actually want to add this it should probably be in a place that can be shared.

That being said, I don't really think map/filter are very standard for golang (they are more natural in a more functional language).

They were actually declined to be added to the stdlib here: golang/go#47203 (That being said, it wasn't a final decision. It was left as a potential future addition).

[richardpringle]

I didn't read the details as to why Map landed "Borderline" section here. It reduces a bunch of boilerplate in these tests in the same way that require.NoError does.

It's not like this should be a new programming concept to anyone, even if the primary language they work with is Go.

As for the location, I'll explain why I think helpers are more useful when they're defined in the same test file. If you still think I should move it, I'm happy to, but hear me out first.

I used to take the same side of the argument as you and push for any utility functions to be in a shared utils package/module. It should be available for anyone to use, right? In practice, test utilities have very low discoverability. If there are docs (there usually aren't), what are the odds that someone is reading them? It's more likely that when you're writing tests, you think, I could use a helper, then you go to the test_utils package or whatever it's called and have to read through the entire package in search of some particular functionality that may or may not be there. So, does that mean we should document all our test utils and have those docs published somewhere? Honestly, it would take a really useful utility function for me to justify spending time making sure I document it properly or whatever, then go hunt down other places in tests that it's meant to be used so we aren't doing things multiple different ways.

OR

We could just define the function in the file that it's being repeatedly used. If we see the exact same helper function pop up in multiple different test files, then and only then do we make a test utility. At that point, we have better discoverability because this is already a repeated pattern.

Sorry for the novel. This is just one of those things that I've tried both ways and found the latter to be significantly more efficient. But as I said, if I haven't convinced you and you prefer the former, I'm happy to move this function.

[StephenButtolph]

I personally find:

Suggested change

	sameSigs := Map(signers[1:], func(signer *LocalSigner) *bls.Signature {
	sig, err := signer.Sign(msg)
	require.NoError(err)
	return sig
	})
	sameSigs := make([]*bls.Signature, len(signers)-1)
	for i, signer := range signers[1:] {
	sig, err := signer.Sign(msg)
	require.NoError(err)
	sameSigs[i] = sig
	}

more natural for a golang dev. Using stdlib/language features should generally be preferred over (unless there is an actual gain out of said implementation).

Basically I think the boilerplate of:

newSlice := make([]T, len(oldSlice))
for i, v := range oldSlice {
    // XXX
   newSlice[i] = newV
}

Should be preferred over:

newSlice := Map(oldSlice, func(T) V {
    // XXX
   return newV
})

cc: @ARR4N would like to hear your thoughts here.

[StephenButtolph]

All that being said, I think the (*testing.T, []byte, []*LocalSigner) -> []*bls.Signature should be implemented as a helper function because it is used in a bunch of places.

[ARR4N]

TL;DR I could be convinced of both arguments.

I'll think "out loud":

1. My ultimate goal in readability / style is to reduce cognitive load for the reader of the code. This allows them (us) to more easily parse what's going on when trying to find bugs, make changes, etc.
2. Characteristics that feed into reduced cognitive load are idioms, abstractions, reduced nesting, blah blah blah. This could usually go unsaid, but I think it's important to call out idioms as "does this feel like Go" because if it doesn't then it's a distraction and that adds cognitive load.
3. For me, decisions like this boil down to cost-benefit analysis. For helper functions, the typical benefit is reducing code while the typical cost is distracting the reader by causing a cognitive branch as they read the helper. We don't have deep meat stacks in our skulls so that push/pop is expensive.

Usually I'd say the saving here (the make()) isn't worth the distraction and that a little repetition isn't the end of the world (i.e. with @StephenButtolph). However Map(), Reduce(), Filter()` are all extremely well-known concepts so I don't think it's a distraction. If anything, it's a signal of intent to the reader, which is akin to the benefit of an idiom (i.e. with @richardpringle).

That was a very long-winded way of saying that I'm on the fence and quite stable but if I were to fall it would be towards a shared package that introduces those functions. Just please give it a name that communicates something (maybe fn because these are functional concepts) and not "underscore" like in JS! It's also not appropriate to export bls.Map().

[richardpringle]

🤔 @StephenButtolph, you really think we should add a helper for signing?

func signTheSameMessageWithAllSigners(t *testing.T, signers []*Signer,  msg []byte) []*bls.Signature {
      // `Map` or `For`
      Map(signers, func(signer *Signer) *bls.Signature {
          sig, err := signer.Sign(msg)
          require.NoError(t, err)
          return sig
      } 
}

While the logic is repeated, I really don't think the logic here is intuitive. It's easier to read (and thus debug) a test if I see what's going on directly in the test. You could argue that MapSign might be a better name, but I'm not sure there's a name that's universally intuitive.

I tend not to adhere to the DRY principle when it comes to testing as abstraction in tests often increases the burden of maintenance (and I say that after having to change a ton of tests because of an API change). Also, when debugging, I am usually looking at a single test in isolation and prefer not having to jump around the codebase.

I agree with @ARR4N

...are all extremely well-known

Which is why Map the only function that I've abstracted that isn't some kind of setup function.

[ARR4N]

I missed that this was just in tests as I only read Stephen's comment. If this were my PR I'd keep the function but rename it to something like mapped(). Map() being exported is very suggestive of it being a function in the package, not a test helper, which is a bit jarring. As I said before, I really don't have strong opinions either way (but do want a green bike shed).

[StephenButtolph]

sgtm