Simpler signing #4284
Simpler signing #4284
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4284 +/- ##
===========================================
- Coverage 81.02% 80.91% -0.12%
===========================================
Files 105 102 -3
Lines 13784 13485 -299
Branches 2115 2084 -31
===========================================
- Hits 11169 10911 -258
+ Misses 2019 1982 -37
+ Partials 596 592 -4
Continue to review full report at Codecov.
|
It's in use. How should I remove it? Should I inherit from both
Not sure I'm the right one to do this, as others surely have a better overview of all the client messages and their relationships. But if I do it, I would like to do it in a separate PR. |
| return sha3( | ||
| pack_data( | ||
| (self.cmdid.value, "uint8"), | ||
| (b"\x00" * 3, "bytes"), # padding |
There was a problem hiding this comment.
Note: This doesn't need to be here.
The message_hash exists as a way to normalize the differences between the messages LockedTransfer, RemoveExpiredLock, UnlockTransfer. Roughly, the first has a lock, the second has a lockhash, and the third has a secret. This data must be validated, but it's not part of the balance proof in the smart contract, so we use the additional_hash as a way of allowing the off-chain client to check the integrity of the message.
The padding was here only because the code worked as it was, not because it was necessary. The message_hash only really needs the fields that are not in the balance proof.
`Ping`, `Pong`, `ToDevice`, `Delivered`.
Also remove the unnecessary mapping `CMDID_TO_CLASS`
The check for `reward_proof_signature` does make any sense, because we're just gathering the data to create that signature in that function. I've removed the check for `non_closing_signature` along with it because having it in this place seemed inconsistent (we have no such checks in `_data_to_sign` for any message) and I don't think this is a good place for such checks. Before this PR, the check was applied on serialization. I don't see a place where I could hook into to reproduce that behaviour and it probably makes more sense to move towards signed-by-design messages, anyway.
In #4284 I tried to keep all signatures identical. Now it's obvious that all `EnvelopeMessage` subclasses have a signature that is based on a BP with all additional fields put into the `additional_hash`. Due to this construction, the `message_hash` which is used to calculate the `additional_hash` can be reduced to all fields that are not already included in the BP. The padding is also unnecessary and was only included to preserve the old behaviour during the large refactoring PR mentioned above. It's usually a good idea to separate refactoring and behavior changes.
After removing the UDP transport, all of
raiden.encodingis only used for message signing. This can be done more easily by directly usingpack_data.This significantly reduces code size and the amount of abstraction and it should make implementing #4231 easier.
The
cmdidtodos will be handled in #4297.