[Tech debt] error documentation for `p2p.IDTranslator`s

[AlexHentschel]

Looking at the IDTranslator interface:

• the interface does not document any error returns
• implementations return generic errors leaving higher-level code completely in the dark whether the error is critical or non-critical

The current implementation does not live up to our quality guidelines and best practises (see CodingConventions), i.e. we have technical debt. Furthermore, the current implementation can actually loos safety-critical errors in the following code: https://github.com/onflow/flow-go/blob/c07fdad6304260fc971cf9b74a3262d71720c00c/network/p2p/translator/hierarchical_translator.go#L27-L34

• One of the translators could return a safety-critical error. But since there is no error documentation or error convention, the application logic at this level cannot differentiate critical from benign errors. It just puts all the errors into a multierror.
• Then, if one of the later translators finds a mapping from FlowID to PeerID, all previously accumulated errors will be dropped, including the safety-critical one.

suggestions

• LibP2P provides primitives (-> example) that we could build on top

• We need a relatively broad set of sentinel errors that cover all existing implementations. Something like the following could be used as a starting point:

  • UnknownID: id is unknown
  • InvalidID the format of the ID is syntactically invalid

  These error returns should be specified in IDTranslator interface as benign errors

• All interface implementations should comply with the interface specification. This should be covered by tests where possible.

• Code calling into the IDTranslator should verify that only expected benign errors are being returned. We still have places with "log error and continue with best effort" in the code 😱 https://github.com/onflow/flow-go/blob/c07fdad6304260fc971cf9b74a3262d71720c00c/cmd/observer/node_builder/observer_builder.go#L744-L745

Design comment:

It seems we generate flow.Identifiers for non-staked nodes. I consider this a sub-optimal design, because only nodes that are part of the identity table have a flow.Identifier. It is not a big problem, but slightly unclean. Not sure if we can get around this problem by splitting up the interface into two:

1. flow.Identifier -> peer.ID converter
2. peer.ID -> flow.Identifier converter