-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup! Left two small comments, but I also would like someone else to look over this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're changing this, could you also do the following?
- remove
SignedRetrieableMessage
? - split up the messages in modules, the protocol messages have nothing to do with Ping/Pong, or matrix messages.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return sha3( | ||
pack_data( | ||
(self.cmdid.value, "uint8"), | ||
(b"\x00" * 3, "bytes"), # padding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummm, It looks this has a cycle in the signing of the monitoring request.
`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.encoding
is 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
cmdid
todos will be handled in #4297.