Signing root reqs

[arnetheduck]

Clarify expected hashes in req/resp of networking spec

[hwwhww]

We found "block root" is ambiguous in Trinity implementation - now we use custom type HashTreeRoot and SigningRoot to distinguish them. (In Python, they are HashTreeRoot = NewType("HashTreeRoot", Hash) and SigningRoot = NewType("SigningRoot", Hash)). Do you think this kind of annotation would be helpful to the networking spec?

[arnetheduck]

The name Hash is from the main spec, I think we should stay consistent with that (ie change this or change that) - not sure why we ended up using HashTreeRoot here (historical?).

I guess the problem is signing_root itself which has a pretty awkward definition, which leads to these name games. In practise we never use hash_tree_root with blocks, so there's no de facto ambiguity (or is there?) - one alternative would be to unify hash_tree_root and signing_root, or remodel the ssz objects so that the signature is external to the data being signed.

Having types for different hashes in the spec feels a bit over the top - they're the same kind of hash but over different data, really - though this is not an opinion I hold strongly, and I guess will depend on context.

[djrtwo]

Agree that we don't really need an additional type here. Let's keep as is with the note for now

[djrtwo]

Changed PR to new branch v09x to stage for release in next minor release

[hwwhww]

@arnetheduck @djrtwo I don't feel strongly about using new types, but I think there are still some unclarity of the spec. :)

In practise we never use hash_tree_root with blocks

https://github.com/ethereum/eth2.0-specs/blob/v09x/specs/networking/p2p-interface.md#beaconblocksbyrange

Requests count beacon blocks from the peer starting from start_slot on the chain defined by head_block_root (= signing_root(BeaconBlock)).

^^^^ it's signing root
but!

https://github.com/ethereum/eth2.0-specs/blob/v09x/specs/networking/p2p-interface.md#messages
In /eth2/beacon_chain/req/status/1/

head_root: The block hash tree root corresponding to the head of the chain as seen by the sending node.

1. To make sure, is it hash tree root?
2. If so, the client needs to keep both hash tree roots and signing roots for lookup? (That's what we do in Trinity, because we follow the v08 spec which uses hash tree root for block root.)
3. If so, it would be nice to distinguish both fields more precisely. e.g., head_beacon_signing_root and head_beacon_hash_tree_root. Or using the same way to describe hash tree root, e.g.:
   • Requests count beacon blocks from the peer starting from start_slot on the chain defined by SSZ signing root head_beacon_block_root (= signing_root(BeaconBlock)).
   • head_beacon_block_root: The block hash tree root (hash_tree_root(BeaconBlock)) corresponding to the head of the chain as seen by the sending node.

[hwwhww]

Also, it's odd to have both bytes32 and Hash for roots at the spec. I'd say align it with bytes32, otherwise, we should apply all custom types (Slot, Epoch... ) here, which seems to cross the design scopes.

[arnetheduck]

Good catch @hwwhww!

1. The intent when we were discussing it was signing root here as well, it is another oversight / naming confusion
2. I'd say let's keep blocks to signing root always - less confusion, fewer indices, easier to copy paste things in logs
3. Also fixed by always using signing root - the names were meant to be derived from / aligned with the main spec while conveying their special meaning. Clearly there's copy-writing to do here :)

[arnetheduck]

Follow up: #1459