Skip to content

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

Merged
merged 100 commits into from
May 25, 2025
Merged

Signalling #89

merged 100 commits into from
May 25, 2025

Conversation

LeoPatOZ
Copy link
Collaborator

@LeoPatOZ LeoPatOZ commented Mar 27, 2025

This PR aims at adding additional signalling functionality.

  1. Adding a canonical ETH bridge
  2. Simplifying the signal service
  3. Added historical commitment tracking

Copy link
Contributor

@ggonzalez94 ggonzalez94 left a 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

/// @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;
Copy link
Contributor

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)
  }

Copy link
Member

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
        }
    }

simplify

comment

updated event

explain root
re-do order

comment
@LeoPatOZ
Copy link
Collaborator Author

LeoPatOZ commented Apr 16, 2025

  • In LibSignal, we could reorder the functions so the value is the first parameter and we can treat everything as operations on the value. At the moment we use the syntax value.signal() but not value.signaled(sender) or value.verifySignal(...). I think if we do the first then we should do the others too.

This is now fixed

  • isSignalStored has a comment that it returns false if the signal itself is zero. It also seems a bit weird that we save value as the storage value, but just check that it's non-zero when seeing if the signal exists. Should we check that it actually has the expected value for robustness? If so, we could save the hash of the value so that 0 is still a valid signal, or we could just save a specific non-zero constant for all signals.

Okay i went with the "hash the underlying value" approach i think its better this way - less edge cases

  • The SignalService._verifySignal function calls one of two different versions of LibSignal.verifySignal, depending on whether there is an account proof. I think it would be clearer to move that branch into the LibSignal.verifySignal function itself. I think that also fixes some naming issues (where root is described as a state root but it's a storage root, and there is a stateProof instead of a storageProof)

Okay they are now merged

@LeoPatOZ
Copy link
Collaborator Author

  • As far as I can tell, the deposit id is signalled like any other signal. This means that I can just call sendSignal with a valid deposit ID (without sending any ETH), and then redeem it on the other chain

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

remove log
@nikeshnazareth
Copy link
Collaborator

I just realised I updated LibTrieProof but it has Taiko as a security contact. Are we treating it like an external library that needs to remain identical? If not, should we push the whole "if there is an account proof get the storage root first" logic to the LibTrieProof to avoid duplicating all the parameters in LibSignal?

@nikeshnazareth nikeshnazareth mentioned this pull request Apr 21, 2025
4 tasks
Copy link

github-actions bot commented Apr 30, 2025

Changes to gas cost

Generated at commit: cfd2278b9d17176d241485ff1c4039875e71c028, compared to commit: 9561b98669bd663816aa832367790cc842c028f5

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
CheckpointTracker getProvenCheckpoint
proveTransition
+4,302 ❌
+23,067 ❌
+87.46%
+31.27%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
CheckpointTracker 895,952 (+21,395) getProvenCheckpoint
proveTransition
9,221 (+4,302)
96,835 (+23,067)
+87.46%
+31.27%
9,221 (+4,302)
96,835 (+23,067)
+87.46%
+31.27%
9,221 (+4,302)
96,835 (+23,067)
+87.46%
+31.27%
9,221 (+4,302)
96,835 (+23,067)
+87.46%
+31.27%
2 (0)
1 (0)
ETHProverManagerMock 3,185,904 (0) evictProver
finalizePastPeriod
prove
83,711 (+22)
57,325 (+22)
98,885 (+14,802)
+0.03%
+0.04%
+17.60%
83,711 (+22)
57,650 (+22)
113,023 (+25,461)
+0.03%
+0.04%
+29.08%
83,711 (+22)
57,650 (+22)
113,091 (+25,010)
+0.03%
+0.04%
+28.39%
83,711 (+22)
57,975 (+22)
118,401 (+27,904)
+0.03%
+0.04%
+30.83%
1 (0)
2 (0)
7 (0)
ERC20ProverManagerMock 0 (0) evictProver
finalizePastPeriod
prove
83,711 (+22)
57,325 (+22)
98,907 (+14,802)
+0.03%
+0.04%
+17.60%
83,711 (+22)
57,650 (+22)
113,045 (+25,461)
+0.03%
+0.04%
+29.07%
83,711 (+22)
57,650 (+22)
113,113 (+25,010)
+0.03%
+0.04%
+28.39%
83,711 (+22)
57,975 (+22)
118,423 (+27,904)
+0.03%
+0.04%
+30.83%
1 (0)
2 (0)
7 (0)
PublicationFeed 760,671 (0) getPublicationHash 2,936 (0) 0.00% 4,888 (-15) -0.31% 4,936 (0) 0.00% 4,936 (0) 0.00% 42 (-20)

Copy link
Contributor

@ggonzalez94 ggonzalez94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a final pass and I think this is ready to merged. We might want to exclude the fork tests(or have a separate command to run them), since they block the pipeline and make every forge test run fail.
I know we are changing the testing strategy for the SS and the bridge on #119 , so I'm fine with either approach as long as we keep main clean

@ggonzalez94 ggonzalez94 merged commit 46ea35e into main May 25, 2025
6 checks passed
@ggonzalez94 ggonzalez94 deleted the signal-service branch May 25, 2025 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants