-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
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.
As I understand you still continue thinking about the Recovery module. Let me know when you have finalized the changes from your side and I will have another look.
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.
Looks good to me! I just added some minor suggestions and then is ready for merge.
* `bitmap`: The bitmap corresponding to `storeValue` in the sparse Merkle tree as specified in [LIP 0039][lip-0039#proof-construction]. | ||
* `siblingHashes`: Array of bytes with the sibling hashes in the sparse Merkle tree for the inclusion proofs of `storeEntries` in the state of the sidechain as specified in [LIP 0039][lip-0039#proof-construction]. | ||
* `module = MODULE_NAME_INTEROPERABILITY`, | ||
* `command = COMMAND_STATE_RECOVERY`. |
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.
I usually use COMMAND_NAME_XXX
, same as MODULE_NAME_XXX
. I think this is what we agreed. Not a big deal, just we have to be consistent. If I am wrong, let me know to correct my PRs.
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.
In the interop LIPs (and also in LIP41) I used COMMAND_XXX, so for the moment I'd stick to that.
proposals/lip-0054.md
Outdated
@@ -52,6 +52,8 @@ This recovery mechanism requires these conditions to be valid: | |||
|
|||
There is an extra requirement in the case of recoveries in the sidechain context: The sidechain in which the recovery will happen needs to be aware of the `stateRoot` of the terminated sidechain. In general, this information is only available on mainchain (in the terminated state account of the terminated sidechain). A way to make sidechains aware of this specific information for state recoveries is needed. This recovery initialization process on sidechains can happen in two ways: | |||
|
|||
* **Liveness termination command**: This command is used to terminate a sidechain that violated the liveness condition on the mainchain, effectively allowing users to start the recovery process. | |||
|
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.
I find adding this here confusing, since the paragraph above explicitly describes "a way to make sidechains aware of ..." and assumes that a terminated state account already exists in mainchain. Since the liveness termination command is submitted only in mainchain to create the terminated state account, I think it is off context here.
Some extra comments for the lines around:
line 53: "in two ways" --> "in three ways" (in case you keep the liveness termination command..)
line 60: Replace "cross-chain message LIP" by LIP 0049
line 60: I would explicitly say that the sidechain terminated message is sent by the mainchain during forwarding.
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.
moved to a new section.
- it is in two ways, because we are discussing the sidechain case.
- done
- improved the description
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.
Just a couple of remarks (also from my previous review).
#### Verification | ||
|
||
```python | ||
def verify(trs: Transaction) -> None: |
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.
You don't verify explicitly that we are on the mainchain, as you say in the description of the command. Of course, isLive
function will return True
on the sidechain, but I would be more precise and add a check if OWN_CHAIN_ID != CHAIN_ID_MAINCHAIN: raise Exception()
.
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.
My point here is that this command would only exist in the mainchain interoperability module (so this check is unnecessary)
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.
Just one typo to fix, otherwise all is good. Parameter explanations help a lot, in my opinion.
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.
Looks good to me! Just mentioned two typos. Other than this, ready to merge from my side!
proposals/lip-0054.md
Outdated
@@ -55,21 +55,27 @@ There is an extra requirement in the case of recoveries in the sidechain context | |||
* **State recovery initialization command**: This command is used to prove on a sidechain the value that the `stateRoot` of the terminated sidechain has on mainchain. | |||
Any user on the corresponding sidechain can send a transaction with this command and initiate the state recoveries with respect to the terminated sidechain. | |||
A sidechain account can be terminated on a sidechain using the `terminateChain` function exposed by the Interoperability module. In this case, the state root is generally not available and the terminated state account is created without setting the sidechain state root. Instead, the account stores the mainchain state root at the time of termination. A state recovery initialization command will in the future set the sidechain state root by giving an inclusion proof against this mainchain state root. | |||
* **Sidechain terminated message**: As specified in the [cross-chain message LIP][lip-0049#terminatedMessage], when a CCM reaches a receiving chain that has been terminated, a sidechain terminated message is created and sent back to the sending chain carrying the `stateRoot` of the terminated sidechain. The application of this CCM on the sidechain will effectively initiate the recovery process. | |||
* **Sidechain terminated message**: As specified in the [LIP 0049][lip-0049#terminatedMessage], when a CCM tries to reach a receiving chain that has been terminated, the mainchain creates a sidechain terminated message and sends it back to the sending chain. This CCM carries the `stateRoot` of the terminated sidechain and its execution on the sending sidechain will effectively initiate the recovery process. |
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.
Here (and in a couple of more places) "In the LIP 00XXX" --> "In LIP 00XXX",
Update LIP 0054: