-
Notifications
You must be signed in to change notification settings - Fork 6
Signalling #89
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
base: main
Are you sure you want to change the base?
Signalling #89
Conversation
todo comments bytes memory
The value is what you want to signal i.e. a message. A signal is what actually ends up being stored
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 did a first pass on the signaling mechanism, will continue with the CheckpointSyncer and briding. The most important feedback so far is that we are missing some parameters when verifying the signals
src/libs/LibSignal.sol
Outdated
/// @dev Signal a `value` at a namespaced slot. See `deriveSlot`. | ||
function signal(uint64 chainId, address account, bytes32 value) internal returns (bytes32) { | ||
bytes32 slot = deriveSlot(chainId, account, value); | ||
slot.getBytes32Slot().value = value; |
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 we already have the storage slot, is there any gas savings from doing this directly with assembly? I'm not familiar with the implementation of getBytes32Slot
, I'll check it out - this is just not to miss the idea
assembly {
sstore(slot, value)
}
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.
Hey, the implementation is:
function getBytes32Slot(bytes32 slot) internal pure returns (Bytes32Slot storage r) {
assembly ("memory-safe") {
r.slot := slot
}
}
Co-authored-by: Gustavo Gonzalez <gustavo.gonzalez@openzeppelin.com>
Co-authored-by: Gustavo Gonzalez <gustavo.gonzalez@openzeppelin.com>
98f3244
to
89056bc
Compare
simplify comment updated event explain root
aa0ee38
to
d7da73f
Compare
re-do order comment
baa218e
to
96c4b94
Compare
…up into signal-service
Yes I was planning on updating this once the PR was merged as I just wanted to update it once 😄
Yes
Yes
Yes it should be deployed to the same address just to make verifying easier. I believe by deploying it with the same parameters it should deploy to the same address naturally (at least this happens during testing) - do you see an issue with this?
yes correct
Yes i believe Daniel / someone said that the SignalService / Bridge on the L2 holds unlimited ETH
That's my understanding yes At a high level this makes sense to me, but there are some things that seem weird:
The reason we do this is store store 'historical' checkpoints. I think we discussed that if checkpoints are updated rapidly and storage / account proofs generated based only of the latest state root, they might become invalid by the time you submit them to the SignalService. With he commitment store you can specify the height you're proof corresponds to. (I agree this might be an unnecessary overhead if checkpoints only update every hour or so). Or are you referring to something else ?
For now we have decided that the SS will only allow communication between two chains L1 <-> L2. We we have removed the notion of dest chain and source chain #99. I think we might re-introduce that in the future. Although if the current SS is deployed on multiple L2s at the same address, then i agree that
becomes an issue as now you're deposit it technically valid about all SS across all different rollups. Perhaps this is a good argument to re-introduce chain ids now.
Hmm yes actually this does seem like a huge issue... Im confused as i believe the current taiko implementation follows the same logic. Here https://github.com/taikoxyz/taiko-mono/blob/main/packages/protocol/contracts/shared/bridge/Bridge.sol#L173-L176 and here https://github.com/taikoxyz/taiko-mono/blob/main/packages/protocol/contracts/shared/bridge/Bridge.sol#L256-L262
Maybe i dont understand the exploit, as the storage proof will only be valid against the trusted storageRoot of the signal service? You can make a valid storage proof for another account but it wont match the trusted root the signal service gets correct? |
I have some other suggestions for things to improve. I'm happy to make the changes but I thought I'd mention it here so the rationale is clear.
|
I agree with the rationale for storing historical checkpoints - I'm objecting to duplicating it. I'll create a draft for moving the tracker storage into the commitment store. I think this would also naturally let rollups overwrite obsolete checkpoints.
This doesn't really make sense to me because we're treating it as a singleton and hardcoding the address. So this decision implicitly means that there can only ever be one rollup using this stack. I can understand making a simple version with core features and slowly expanding that over time (which we've already done several times in this repo) but I don't think that applies in this case because:
I think I'm saying something closer to this:
|
This is now fixed
Okay i went with the "hash the underlying value" approach i think its better this way - less edge cases
Okay they are now merged |
Okay i changed this now to have a separate namespace param - one for eth deposits and one for generic signals. (Im not convinced on all the namings so open to suggestions) but at least this isnt an issue |
33c93a8
to
e7ddb8d
Compare
This should have been part of the previous commit
I just realised I updated |
Changes to gas cost
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
This PR aims at adding additional signalling functionality.