Refactor message hashing/signing

Zilliqa / zq2

Zilliqa 2.0 code base

Apache License 2.0

9 stars 0 forks source link

Refactor message hashing/signing #981

Closed JamesHinshelwood closed 2 months ago

JamesHinshelwood commented 4 months ago

For all messages that include signatures:

Move the code around to ensure that we're only calculating those signatures in a single place. See https://github.com/Zilliqa/zq2/issues/981#issuecomment-2134506270 for an example of where we're doing it in multiple places right now.
Ensure all fields in the message are signed over for the signature.

DrZoltanFazekas commented 4 months ago

BlockHeader hash doesn't include the timestamp either, but it should: https://github.com/Zilliqa/zq2/blob/main/zilliqa/src/message.rs#L586

DrZoltanFazekas commented 4 months ago

Btw, computing the block hash is duplicated at least 4 times - shouldn't we add a function which implements it at one place? https://github.com/Zilliqa/zq2/blob/main/zilliqa/src/message.rs#L586 https://github.com/Zilliqa/zq2/blob/main/zilliqa/src/message.rs#L618 https://github.com/Zilliqa/zq2/blob/main/zilliqa/src/message.rs#L654 https://github.com/Zilliqa/zq2/blob/main/zilliqa/src/message.rs#L690

DrZoltanFazekas commented 4 months ago

Imo, AggregateQC hash should include the view number too: https://github.com/Zilliqa/zq2/blob/main/zilliqa/src/message.rs#L359

DrZoltanFazekas commented 4 months ago

There are a lot more things missing from the block signature. Imo the signature should attest to all fields of the block header and e.g. Polygon's validators do sign everything afaic: https://github.com/maticnetwork/bor/blob/master/consensus/bor/bor.go#L1034 https://github.com/maticnetwork/bor/blob/master/core/types/block.go#L74

JamesHinshelwood commented 4 months ago

Agreed. We should centralise the computation of message hashes and ensure all fields are included. I'll edit this issue's title and description to state that.