Implement IOutputsMerkleRootValidator
#133
Conversation
GCdePaula
force-pushed
the
feature/implement-root-validator
branch
2 times, most recently
from
ed8210e to
861a2c0
Compare
GCdePaula
force-pushed
the
feature/implement-root-validator
branch
from
861a2c0 to
cb0a068
Compare
| @@ -110,15 +137,15 @@ contract DaveConsensus is IDataProvider { | |||
| _inputIndexUpperBound = inputIndexUpperBound; | |||
| ITournament tournament = tournamentFactory.instantiate(initialMachineStateHash, this); | |||
| _tournament = tournament; | |||
| emit EpochSealed(0, 0, inputIndexUpperBound, initialMachineStateHash, tournament); | |||
| emit EpochSealed(0, 0, inputIndexUpperBound, initialMachineStateHash, bytes32(0), tournament); | |||
There was a problem hiding this comment.
Regarding the outputsMerkleRoot parameter of the EpochSealed event:
It is weird to have it as bytes32(0) (invalid).
If there is any valid initial value, it is the pristine Merkle root.
However, I don't see this new event parameter being used from off-chain.
So, do you think this event parameter is necessary?
There was a problem hiding this comment.
I think this parameter is super necessary on settle.
Here in instantiation, however, not so much. I think we could either:
- create a new event type without the
outputsMerkleRoot; - receive a Merkle proof that extracts whatever is there. We have to check whether the machine saves the
outputsMerkleRootin the first yield. It could make sense if we want there to be "initial outputs", (but I don't want this to hinge in the decision since we don't know whether it's a requirement); - emit a valid Merkle tree, but "empty" (whatever is the sentinel value we're currently using);
- emit bytes32(0).
I feel the last one has no apparent issues, and it was the simplest to implement.
There was a problem hiding this comment.
What do you think?
There was a problem hiding this comment.
Having bytes32(0) is harmless because it cannot be used to prove anything.
However, logically speaking, when an epoch is sealed, only the epoch boundaries are defined. Meanwhile, the final machine state after the epoch (and, by extension, the outputs Merkle root) is only defined when it is settled.
So, if you want to notify the off-chain node that an epoch was settled, I think adding an EpochSettled epoch makes more sense. And, besides the outputsMerkleRoot parameter, this event would also include the epoch number and the final machine state hash.
| emit EpochSealed(epochNumber, inputIndexLowerBound, inputIndexUpperBound, finalMachineStateHash, tournament); | ||
|
|
||
| // Extract and save settled output tree | ||
| _validateOutputTree(finalMachineStateHash, outputsMerkleRoot, proof); |
There was a problem hiding this comment.
We should follow the Checks-Effects-Interactions safety pattern:
Validating the Merkle proof (Checks) should happen before sealing the current accumulating epoch (Effects).
There was a problem hiding this comment.
Done! I've also simplified the logic, but with a very modest increase in gas costs (< 1%). This function is currently costly because it's instantiating another contract. I feel we should optimize this by using a proxy later.
I've also added a _tournament = ITournament(address(0)); before setting it back to a valid address. I don't think it's really needed (the factory should trusted, so there should be no reentrancy risks), but feels right. I don't know, what do you think?
There was a problem hiding this comment.
I find a < 1% increase in gas cost perfectly acceptable.
As for zeroing the tournament address before instantiation,
I don't think it's necessary for trusted tournament factories,
but it is a welcome security improvement in general.
GCdePaula
force-pushed
the
feature/implement-root-validator
branch
from
a71843a to
59993ae
Compare
No description provided.