Secret message #830
Secret message #830
Conversation
hackaugusto
force-pushed
the
secret_message
branch
3 times, most recently
from
bb08e57
to
adfa456
Compare
the receiving end needs the channel address to verify the signature
the netting channel doesn't decode data anymore
hackaugusto
force-pushed
the
secret_message
branch
from
adfa456
to
4f482ed
Compare
- the updateTransfer function may be called only during the settlement period. - Extended the number of blocks to mine, the reduce test case was hard to reason because multiple mine operations were required. - Handling the out-of-gas when multiple transactions are executed in the same block.
|
|
||
| if result is None: | ||
| data_that_was_signed = ( |
There was a problem hiding this comment.
The decode() function of this class, EnvelopeMessage and the decode function of SignedMessage share some logic, especially in the try-catch of the recover_publickey function. Can you perhaps abstract it out?
| if (their_transfer.length != 0) { | ||
| (transfer_raw, transfer_address) = getTransferRawAddress(their_transfer); | ||
| counterparty_index = index_or_throw(self, transfer_address); | ||
| if (signature.length == 65) { |
There was a problem hiding this comment.
Question here: The change in logic here means that if a pariticpant calls close() with an invalid signature, he will have indeed closed the channel but it will be as if he never provided a transfer. So in the new logic if you make a mistake in the signature you are screwed.
I don't think that's bad as we would always assume people use Raiden clients and don't do calls on the contracts manually.
| ) | ||
|
|
||
| # increases likely hood of the mine op, while permitting transactions |
There was a problem hiding this comment.
likelyhood -> likelihood
| /// transfers. | ||
| function close(Data storage self, bytes their_transfer) | ||
| { | ||
| function close( |
There was a problem hiding this comment.
Both here and in the updateTransfer() function it would be nice to have a docstring entry for all the arguments. If someone is not familiar with Raiden then some of them may be difficult to figure out. You have also added this extra_hash argument which seems to be the sha3 hash of the first 65 bytes of the message?
|
|
||
| transfer_nonparticipant_hash = sha3(transfer_nonparticipant.packed().data[:-65]) |
There was a problem hiding this comment.
Since we use this hash now in many places for the message, perhaps make a method on the transfers that returns the hash, so that you don't have to explicitly always ask for the slice of the first 65 bytes of data?
| @@ -69,6 +68,7 @@ def make_message(message, **attrs): | |||
| target = make_field('target', 20, '20s') | |||
| initiator = make_field('initiator', 20, '20s') | |||
| sender = make_field('sender', 20, '20s') | |||
| channel = make_field('channel', 20, '20s') | |||
There was a problem hiding this comment.
Question to make sure I properly comprehend the changes.
You have added the channel to all messages but it is not being consumed anywhere.
It seems to be consumed indirectly though through the hash of the first 65 bytes of the transfer. That would make messages unique per channel and would make the nonce ranges check for "replay attacks" irrelevant.
Correct?
|
Merged it since I would like to see tests being ran with the new code. My questions/comments can also be answered afterwards since none of them is a blocker. Note that this PR prepares the ground for it but does not read the |
This PR removes the decoder logic from the netting channel.
It touches the following issues: