Change type of light block's provider field from PeerId to net::Address

[romac]

See: #485
See: #99

---

From #485:

we might as well just use the address to identify the peer since we don't authenticate the peer_id

This PR does just that and changes the type of the provider field in a LightBlock from a PeerId to a net::Address, and does not forget to update the testgen crate accordingly.

On my machine, some of the model-based tests are still failing because the static JSON files cannot be parsed and would need to be regenerated. @andrey-kuprianov @Shivani912 How would I go about regenerating all the static MBT JSON tests?

---

• Referenced an issue explaining the need for the change
• Updated all relevant documentation in docs
• Updated all code comments where relevant
• Wrote tests
• Updated CHANGELOG.md

[Shivani912]

On my machine, some of the model-based tests are still failing because the static JSON files cannot be parsed and would need to be regenerated. @andrey-kuprianov @Shivani912 How would I go about regenerating all the static MBT JSON tests?

Hi @romac, if you run MBT with the model checker, it re-generates the test files and stores them under the subdirectory _test_run/MC4_4_faulty.json. You'll find subdirectories under there for each test which contains the relevant .tla and .json versions of it. From there, you can manually copy-paste them to the /single_step folder where the static tests are present.

Let me know if I can help more :)

[andrey-kuprianov]

A question: isn't it that provider used for fork detection and evidence reporting? If that's the case, does it make sense to report something transient, I suppose, like the network address, and not the peer id, which can be tracked in the blockchain, and the respective peer then could be punished? Will be grateful for the explanation about this aspect.

[andrey-kuprianov]

On my machine, some of the model-based tests are still failing because the static JSON files cannot be parsed and would need to be regenerated. @andrey-kuprianov @Shivani912 How would I go about regenerating all the static MBT JSON tests?

@romac the static tests fail because they still use the provider field which is peer_id, not address. They need indeed to be regenerated as @Shivani912 pointed out. If the full MBT setup is not working on your machine, for @Shivani912 it should not be a problem, I hope, to help you and regenerate them.

We should try in the near future to get rid of the static files completely -- this is already smth. like 5th time that we regenerate them

[romac]

A question: isn't it that provider used for fork detection and evidence reporting? If that's the case, does it make sense to report something transient, I suppose, like the network address, and not the peer id, which can be tracked in the blockchain, and the respective peer then could be punished? Will be grateful for the explanation about this aspect.

That's a good point, hadn't realized that and might indeed have misunderstood what @ebuchman meant in #485.

[romac]

@Shivani912 @andrey-kuprianov That worked, thanks a lot!

[ebuchman]

not the peer id, which can be tracked in the blockchain, and the respective peer then could be punished? Will be grateful for the explanation about this aspect.

Peers (full nodes) are transient and have no economic stake - they are not tracked on chain and are not likely to be. This is unlike the validators which are tracked on chain and can be slashed for misbehavious. Misbehaviour of peer nodes is thus quite distinct from that of validators. Clients are also unlikely to connect directly to validators, since validators will prefer to hide their network address behind sentry nodes - full nodes that serve as proxies to the public internet.

The problem of peer identity at the network level - of more permanent peer IDs, tracking their behaviour, and telling others about - remains an open an active area of research.

In the meantime, since our light client is just talking to arbitrary full nodes, and since we are not verifying an identity beyond that given by a TLS certificate, we can just use a network address equivalent to identify them (ie. IP address or DNS name). For simplicity for now we can just use an IP address, but as our peer management matures we may want to incorporate domain names. And if we ultimately use the Tendermint P2P system instead of HTTP, then the peer ID will be back (effectively replacing the TLS cert).

Hope that makes sense.

[brapse]

In our peer to peer work, we are specifically trying to avoid transport specific identities and standardize around PeerID. So at a high level i'm against this./cc @xla

[xla]

In the meantime, since our light client is just talking to arbitrary full nodes, and since we are not verifying an identity beyond that given by a TLS certificate, we can just use a network address equivalent to identify them (ie. IP address or DNS name). For simplicity for now we can just use an IP address, but as our peer management matures we may want to incorporate domain names. And if we ultimately use the Tendermint P2P system instead of HTTP, then the peer ID will be back (effectively replacing the TLS cert).

This is a sensible path forward. Not using peer ids in the context will help with the assumptions about the trust model.

In our peer to peer work, we are specifically trying to avoid transport specific identities and standardize around PeerID. So at a high level i'm against this./cc @xla

What @brapse is highlighting here is to pushback on physical networking concern leaks into the core domain. For that matter a peer id is a more stable key with less connotation. If there is a handshake mechanism, TLS in this case, maybe it can be used to establish an (stable-ish) identity.

[brapse]

Thanks for following up @xla

Just to be concrete here, it might make sense to introduce a transport agnostic type of ID for nodes which are not yet peers such that we don't have to encode transport level concerns in our dependency and in tests. Maybe NodeID is just an alias to net::Address to start but can be swapped out. Just a thought.

[thanethomson]

Is this PR still relevant and/or ready for review?

[romac]

It's outdated and the discussion has not come up again so let's close it.