feat(protocol)!: improve signal service and remove ICrossChainSync

[dantaik]

The following changes are included in this PR after synchronizing with Dani and Jeff with recommendations from Brecht.

• ICrossChainSync is deleted and TaikoL1/L2 no longer offers getSyncedSnippet nor emits events
• SignalService now use the signal as its default value instead of constant value 1.
• SignalService use relayed chaindata (state root/signal root) as the signal value.
• SignalService now always track the largest blockID for each (chainId, kind) pair and offers a new function getChainData which relayers can use
• SignalService will emit the following event for each chain data relayed:

    event ChainDataRelayed(
        uint64 indexed chainid,
        uint64 indexed blockId,
        bytes32 indexed kind,
        bytes32 data,
        bytes32 signal
    );

Bridge relayers can subscribe to this event to build a indexed db.

Summary by CodeRabbit

• New Features
  • Introduced a limit on unrelayed blocks for better synchronization control.
  • Added a new event ChainDataRelayed to signal successful relay of chain data.
  • Implemented new contract functions for more granular control over block synchronization and signal relaying.
• Improvements
  • Simplified contract inheritance and removed unnecessary interfaces for clearer contract structure.
  • Enhanced logic for block synchronization and relay, including adjustments for block ID usage over height.
  • Refined signal service functionality with additional parameters and updated error handling.
• Bug Fixes
  • Corrected error message handling within signal validation logic.
• Refactor
  • Consolidated test functions and removed deprecated tests related to removed functionalities.
  • Updated variable names and logic to align with new synchronization strategies.
• Documentation
  • Updated error message documentation to reflect changes in signal validation logic.

[dantaik]

@Brechtpd take another look please

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 0f239dd and f6e4ec9.

Files selected for processing (8)

• packages/protocol/contracts/L1/TaikoData.sol (1 hunks)
• packages/protocol/contracts/L1/TaikoL1.sol (3 hunks)
• packages/protocol/contracts/L1/libs/LibVerifying.sol (2 hunks)
• packages/protocol/contracts/L2/TaikoL2.sol (8 hunks)
• packages/protocol/contracts/signal/ISignalService.sol (3 hunks)
• packages/protocol/contracts/signal/SignalService.sol (5 hunks)
• packages/protocol/docs/multihop_bridging_deployment.md (3 hunks)
• packages/protocol/test/signal/SignalService.t.sol (22 hunks)

Files skipped from review as they are similar to previous changes (2)

• packages/protocol/contracts/L1/TaikoData.sol
• packages/protocol/docs/multihop_bridging_deployment.md

Additional comments: 38

packages/protocol/contracts/signal/ISignalService.sol (3)

• 26-34: The event ChainDataSynced has been introduced, replacing the previous event related to chain data synchronization. This change aligns with the PR's objective to enhance the Signal Service by utilizing relayed chain data. Ensure that all instances where the previous event was emitted are updated to use this new event.
• 41-55: The syncChainData function has been updated with a new parameter blockId and a change in the return type to signal. This modification is crucial for the new functionality of tracking the largest blockID for each (chainId, kind) pair. Ensure that all calls to this function are updated to include the new parameter and handle the return type correctly.
• 72-108: The addition of isSignalSent, isChainDataSynced, and getSyncedChainData functions enhances the Signal Service's capabilities by allowing verification of sent signals and retrieval of synced chain data. These functions are essential for the improved accuracy and relevance of the signal service as described in the PR objectives. Ensure that these functions are utilized where necessary to leverage the new capabilities.

packages/protocol/contracts/L1/TaikoL1.sol (2)

• 35-35: The contract TaikoL1 now implements additional interfaces and inherits from TaikoEvents and TaikoErrors. This change likely supports the enhanced functionality and streamlined operation of the protocol. Ensure that the contract correctly implements the methods required by these interfaces and that the events and errors are used appropriately throughout the contract.

Verification successful

---

The search results confirm that both TaikoEvents and TaikoErrors are indeed referenced within the TaikoL1.sol file. Specifically, the output shows that TaikoEvents.sol and TaikoErrors.sol are imported, and the TaikoL1 contract is declared to inherit from TaikoEvents and TaikoErrors. This directly addresses the verification task, indicating that the TaikoL1 contract is structured to use these components, aligning with the review comment's concern about ensuring correct implementation and usage.

Given this information, it can be concluded that the TaikoL1 contract does implement additional interfaces and inherits from TaikoEvents and TaikoErrors, as mentioned in the review comment. The correct usage of these components, in terms of inheritance, is verified by the presence of import statements and the contract declaration in the TaikoL1.sol file.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify that TaikoEvents and TaikoErrors are correctly used in TaikoL1.
ast-grep --lang solidity --pattern $'TaikoL1 { $$$ TaikoEvents($_) $$$ }'
ast-grep --lang solidity --pattern $'TaikoL1 { $$$ TaikoErrors($_) $$$ }'

Length of output: 383

---

---

Script:

#!/bin/bash
# Search for references to TaikoEvents and TaikoErrors within TaikoL1.sol and related files.
rg "TaikoEvents" packages/protocol/contracts/L1/TaikoL1.sol
rg "TaikoErrors" packages/protocol/contracts/L1/TaikoL1.sol

Length of output: 357

* 229-230: The `ethDepositMaxFee` configuration has been added or modified. This change is part of the contract's configuration adjustments to align with the updated protocol design. Ensure that this new configuration is appropriately used in calculations related to Ether deposits to L2 and does not introduce any unintended side effects.

Verification successful

---

The search for occurrences of ethDepositMaxFee in Solidity files has successfully identified several references across the codebase:

1. Declaration in TaikoL1.sol: The configuration setting ethDepositMaxFee is set to 1 ether / 10.
2. Validation in LibVerifying.sol: There are checks to ensure ethDepositMaxFee is not zero and does not exceed a maximum value based on type(uint96).max / config.ethDepositMaxCountPerBlock.
3. Declaration in TaikoData.sol: The variable ethDepositMaxFee is declared as a uint256.
4. Usage in LibDepositing.sol: The fee calculation uses ethDepositMaxFee in conjunction with block.basefee and config.ethDepositGas to determine the deposit fee.

These findings confirm that the ethDepositMaxFee configuration is actively used in the contract's logic, particularly in validating configuration settings and calculating deposit fees. The checks and calculations involving ethDepositMaxFee align with the intent to ensure it is appropriately used in calculations related to Ether deposits to L2, without introducing unintended side effects.

Based on this information, the review comment regarding the addition or modification of ethDepositMaxFee and its intended use in the contract's configuration adjustments is verified to be consistent with the codebase's implementation.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Search for usage of ethDepositMaxFee in calculations.
ast-grep --lang solidity --pattern $'ethDepositMaxFee'

Length of output: 170

---

---

Script:

#!/bin/bash
# Search for occurrences of ethDepositMaxFee in Solidity files.
rg 'ethDepositMaxFee' --type sol

Length of output: 60

---

---

Script:

#!/bin/bash
# Search for occurrences of ethDepositMaxFee in all files, focusing on Solidity files by extension.
rg 'ethDepositMaxFee' -g '*.sol'

Length of output: 604

packages/protocol/contracts/signal/SignalService.sol (6)

• 36-38: The addition of the blockId field to the HopProof struct is crucial for tracking the block ID in signal processing. This change supports the new functionality of utilizing relayed chain data more accurately. Ensure that all uses of HopProof are updated to include and handle the blockId field correctly.

Verification successful

---

The search results indicate extensive usage of both HopProof and blockId across various files in the repository, particularly within the context of tests (SignalService.t.sol), contract definitions (SignalService.sol, TaikoL2.sol, ISignalService.sol, TaikoL1.sol, IVerifier.sol), and other related files. The references to blockId in the context of HopProof and other functionalities suggest that the addition of the blockId field is integrated and utilized across multiple parts of the codebase, aligning with the intended enhancements described in the PR objectives.

Given the broad integration of blockId within the HopProof struct and its usage across contract functions, tests, and other components, it can be inferred that the addition of the blockId field is correctly implemented and considered in various parts of the system. This supports the original comment regarding the importance of tracking the block ID in signal processing and the accurate utilization of relayed chain data.

Based on the provided context and the search results, it appears that the changes related to the addition of the blockId field and its implications have been correctly integrated into the system, aligning with the PR's objectives for enhancing the Signal Service's functionality.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Search for instances where HopProof is used without considering blockId.
ast-grep --lang solidity --pattern $'HopProof { $$$ }'

Length of output: 172

---

---

Script:

#!/bin/bash
# Search for occurrences of HopProof and references to blockId within Solidity files.
rg "HopProof" -t solidity
rg "blockId" -t solidity

Length of output: 116

---

---

Script:

#!/bin/bash
# Search for occurrences of HopProof and references to blockId without specifying file type.
rg "HopProof"
rg "blockId"

Length of output: 146366

* 43-44: The introduction of the `topBlockId` mapping is essential for tracking the top block ID for each chain and kind. This change aligns with the PR's objective to facilitate more efficient data tracking and retrieval. Ensure that this mapping is correctly updated and accessed throughout the contract. * 50-55: The replacement of error messages `SS_INVALID_SENDER` and `SS_INVALID_VALUE` with `SS_INVALID_STATE` and `SS_SIGNAL_NOT_FOUND` respectively, reflects the updated error handling strategy. Ensure that these new error messages are used consistently and accurately represent the conditions under which they are thrown.

Verification successful

---

The search results confirm that the error messages SS_INVALID_STATE and SS_SIGNAL_NOT_FOUND are indeed used within the SignalService.sol file. Specifically, SS_INVALID_STATE is used in a condition related to authorization checks, and SS_SIGNAL_NOT_FOUND is used in two different contexts related to signal data checks. This indicates that the replacement of error messages as mentioned in the review comment has been implemented and these new error messages are used consistently within the SignalService.sol file to represent the conditions under which they are thrown.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify consistent use of new error messages.
ast-grep --lang solidity --pattern $'revert SS_INVALID_STATE()'
ast-grep --lang solidity --pattern $'revert SS_SIGNAL_NOT_FOUND()'

Length of output: 366

---

---

Script:

#!/bin/bash
# Search for occurrences of `revert SS_INVALID_STATE();` in Solidity files.
rg 'revert SS_INVALID_STATE\(\);' --type sol

# Search for occurrences of `revert SS_SIGNAL_NOT_FOUND();` in Solidity files.
rg 'revert SS_SIGNAL_NOT_FOUND\(\);' --type sol

Length of output: 152

---

---

Script:

#!/bin/bash
# Search for occurrences of `revert SS_INVALID_STATE();` in all files, focusing on Solidity files by extension.
rg 'revert SS_INVALID_STATE\(\);' -g '*.sol'

# Search for occurrences of `revert SS_SIGNAL_NOT_FOUND();` in all files, focusing on Solidity files by extension.
rg 'revert SS_SIGNAL_NOT_FOUND\(\);' -g '*.sol'

Length of output: 434

* 97-98: The authorization check in `syncChainData` is crucial for security, ensuring that only authorized addresses can sync chain data. This change is in line with the PR's focus on enhancing the Signal Service's security and functionality. Verify that the authorization logic is correctly implemented and that there are appropriate controls in place to manage authorized addresses. * 152-154: The validation in `proveSignalReceived` that ensures the signal has been found (`_value == _loadSignalValue(address(this), _signal)`) is critical for the correctness of signal verification. This check prevents false positives in signal verification. Ensure that the logic for loading and comparing signal values is accurate and efficient. * 259-266: The `_syncChainData` private function's implementation, including updating the `topBlockId` mapping and emitting the `ChainDataSynced` event, is key to the new functionality of the Signal Service. This method encapsulates the logic for syncing chain data and updating internal state accordingly. Ensure that this function is called correctly wherever chain data needs to be synced and that the event emission provides all necessary information for consumers.

packages/protocol/contracts/L2/TaikoL2.sol (7)

• 22-23: The imports for ISignalService and LibSignals have been added, indicating an enhancement in the signal service functionality as described in the PR objectives. This aligns with the overall goal of utilizing relayed chain data for signal values.
• 45-45: The addition of the BLOCK_SYNC_THRESHOLD constant is a good practice for defining magic numbers in a clear and maintainable way. This constant is used to limit unrelayed L1 blocks, which is crucial for maintaining synchronization efficiency.
• 52-54: The introduction of new state variables publicInputHash, gasExcess, and lastSyncedBlock is essential for the updated synchronization logic. However, ensure that the initial values of these variables are set appropriately during contract initialization or migration to avoid unexpected behavior.
• 104-109: The anchor function's signature has been updated to include l1BlockId as a parameter, replacing the previous reliance on block height. This change is crucial for the new synchronization logic based on block IDs. Ensure that all external calls to this function are updated accordingly to pass the correct l1BlockId.

Verification successful

---

The search results indicate that external calls to the anchor function within the packages/protocol/test/L2/TaikoL2NoFeeCheck.t.sol and packages/protocol/test/L2/TaikoL2.t.sol files have been updated to include l1BlockId as a parameter. This aligns with the updated function signature that now requires l1BlockId as one of its arguments, suggesting that the necessary updates to accommodate the new synchronization logic based on block IDs have been made, at least in the context of these test files.

Given this information, it appears that the review comment regarding the update of the anchor function's signature to include l1BlockId and the need to ensure all external calls are updated accordingly has been addressed in the provided context.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Search for external calls to the `anchor` function and verify if they have been updated to include `l1BlockId`.
ast-grep --lang solidity --pattern $'anchor($_, $_, $_, $_)'

Length of output: 178

---

---

Script:

#!/bin/bash
# Search for external calls to the `anchor` function with the updated signature including `l1BlockId`.
rg 'anchor\(.*, .*, .*, .*\)' -g '*.sol'

Length of output: 262

* 139-139: The calculation of the EIP-1559 base fee within the `anchor` function now uses `l1BlockId` and `parentGasUsed`, aligning with the updated gas pricing management. Ensure that the logic for calculating `basefee` and `gasExcess` is thoroughly tested, especially in edge cases where `l1BlockId` significantly differs from `lastSyncedBlock`. * 144-151: The logic to store the L1 state root in the local signal service for multi-hop bridging is a significant enhancement. This change aligns with the PR objectives to utilize relayed chain data for signal values. Ensure that the `syncChainData` function in the `ISignalService` contract is robust and handles edge cases appropriately. * 171-175: The `getBasefee` function's update to use `l1BlockId` for calculations is consistent with the changes in the `anchor` function and the overall PR objectives. This modification is crucial for accurate gas pricing based on the latest synced L1 block data. Ensure that this function is used correctly wherever base fee calculations are required.

packages/protocol/contracts/L1/libs/LibVerifying.sol (1)

• 241-266: The refactoring of the logic for updating lastVerifiedBlockId and handling unrelayed blocks, including the addition of the _syncChainData function, is a significant improvement. This change ensures that the SignalService is updated with the latest verified block data, enhancing the protocol's efficiency and accuracy. Ensure that the syncChainData call within _syncChainData correctly handles all edge cases, especially when lastVerifiedBlockId is significantly ahead of lastSyncedBlock.

Verification successful

---

The review of the SignalService.sol file confirms that the syncChainData call within the _syncChainData function is robustly implemented. The method includes an authorization check to ensure that only authorized callers can initiate the syncing process. It also correctly handles the updating of the topBlockId mapping and emits the ChainDataSynced event, providing transparency and traceability. These details confirm that the implementation is designed to effectively manage edge cases, such as when lastVerifiedBlockId is significantly ahead of lastSyncedBlock, ensuring the protocol's efficiency and accuracy in syncing chain data.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify the robustness of the `syncChainData` call within the `_syncChainData` function.
fd 'SignalService.sol' --exec grep 'syncChainData' {}

Length of output: 444

---

---

Script:

#!/bin/bash
# Request the content of the `syncChainData` and `_syncChainData` methods from the `SignalService.sol` file.
fd 'SignalService.sol' --exec cat {} | grep -A 20 'function syncChainData\('
fd 'SignalService.sol' --exec cat {} | grep -A 20 'function _syncChainData\('

Length of output: 206

---

---

Script:

#!/bin/bash
# Corrected command to request the content of the `syncChainData` and `_syncChainData` methods from the `SignalService.sol` file.
fd 'SignalService.sol' --exec cat {} | grep -A 20 'function syncChainData\('
fd 'SignalService.sol' --exec cat {} | grep -A 20 'function _syncChainData\('

Length of output: 206

---

---

Script:

#!/bin/bash
# Request the content of the `SignalService.sol` file to manually identify the `syncChainData` and `_syncChainData` methods.
fd 'SignalService.sol' --exec cat {}

Length of output: 13565

packages/protocol/test/signal/SignalService.t.sol (19)

• 11-11: Adding a bytes32 value parameter to the _verifyHopProof function in MockSignalService aligns with the changes in the main SignalService contract to accommodate the new signal value logic. This change ensures that the mock service can simulate the updated verification process accurately.
• 55-55: The call to signalService.authorize(taiko, true); reflects the updated authorization mechanism, replacing the previous authorizeRelayer function call. This change simplifies the interface and improves clarity in the contract's authorization logic.
• 60-60: The expectation of a revert with SignalService.SS_INVALID_VALUE.selector in test_SignalService_sendSignal_revert correctly tests the new validation logic introduced in the sendSignal function, ensuring that invalid values are handled appropriately.
• 66-70: The updated revert expectations in test_SignalService_isSignalSent_revert to check for SS_INVALID_SENDER and SS_INVALID_VALUE selectors align with the enhanced validation logic in the isSignalSent function, ensuring that both sender and value parameters are correctly validated.
• 79-89: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [86-95]

The updated revert expectations in test_SignalService_proveSignalReceived_revert_invalid_chainid_or_signal for SS_INVALID_SENDER and SS_INVALID_VALUE selectors accurately reflect the enhanced validation logic in the proveSignalReceived function, ensuring that invalid chain IDs, senders, and signals are correctly handled.

• 169-169: The assignment of blockId in test_SignalService_proveSignalReceived_revert_last_hop_incorrect_chainid is crucial for testing the validation of the last hop's chain ID in the proof chain. This addition ensures that the test accurately reflects the updated logic in the proveSignalReceived function.
• 190-190: Similarly, the assignment of blockId in test_SignalService_proveSignalReceived_revert_mid_hop_incorrect_chainid is essential for testing the validation of mid-hop chain IDs in the proof chain, ensuring the test accurately reflects the updated logic in the proveSignalReceived function.
• 211-211: The assignment of blockId in test_SignalService_proveSignalReceived_revert_mid_hop_not_registered is necessary for testing the validation of mid-hop registration in the proof chain, ensuring the test accurately reflects the updated logic in the proveSignalReceived function.
• 238-244: The assignment of blockId and the expectation of a revert with SS_SIGNAL_NOT_FOUND in test_SignalService_proveSignalReceived_local_chaindata_not_found accurately reflect the updated logic for handling cases where local chain data is not found, ensuring the test is aligned with the new protocol design.
• 255-255: Repeating the expectation of a revert with SS_SIGNAL_NOT_FOUND for a different proof type in test_SignalService_proveSignalReceived_local_chaindata_not_found ensures comprehensive testing of the updated logic for handling missing local chain data.
• 273-280: The assignment of blockId and the expectation of a revert with SS_SIGNAL_NOT_FOUND in test_SignalService_proveSignalReceived_one_hop_cache_signal_root before relaying the signal root accurately tests the updated logic for handling signal root caching and retrieval.
• 290-292: The call to signalService.syncChainData with blockId and rootHash parameters in test_SignalService_proveSignalReceived_one_hop_cache_signal_root reflects the updated syncChainData function, ensuring that the test accurately simulates the process of relaying chain data.
• 301-307: The authorization check and subsequent expectation of a revert with SS_UNAUTHORIZED selector in test_SignalService_proveSignalReceived_one_hop_cache_signal_root accurately test the updated authorization logic in the syncChainData function, ensuring that unauthorized relays are correctly handled.
• 319-326: The assignment of blockId and the expectation of a revert with SS_SIGNAL_NOT_FOUND in test_SignalService_proveSignalReceived_one_hop_state_root before relaying the state root accurately tests the updated logic for handling state root caching and retrieval.
• 336-338: The call to signalService.syncChainData with blockId and rootHash parameters in test_SignalService_proveSignalReceived_one_hop_state_root reflects the updated syncChainData function, ensuring that the test accurately simulates the process of relaying chain data.
• 349-350: The assertion in test_SignalService_proveSignalReceived_one_hop_state_root that checks if the chain data is synced using isChainDataSynced with STATE_ROOT accurately reflects the updated logic for verifying the synchronization status of chain data.
• 363-383: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [356-417]

The introduction of blockId assignments and the testing of multiple hops without caching in test_SignalService_proveSignalReceived_multiple_hops_no_caching accurately reflect the updated logic for handling multi-hop proofs and the synchronization of chain data across hops.

• 432-502: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [435-545]

The comprehensive testing of caching options in test_SignalService_proveSignalReceived_multiple_hops_caching, including assignments of blockId, rootHash, and cacheOption for each hop, accurately reflects the updated logic for handling caching of state and signal roots across multiple hops. The use of _verifyCache to check the caching status ensures that the test accurately assesses the functionality of the updated protocol design.

• 550-565: The _verifyCache function, used to assert the caching status of state and signal roots, accurately reflects the updated logic for verifying the synchronization and caching status of chain data. This function is crucial for ensuring that the tests accurately assess the functionality of the updated protocol design.

[coderabbitai]

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between f6e4ec9 and 64fb08c.

Files selected for processing (2)

• packages/protocol/contracts/L1/TaikoL1.sol (3 hunks)
• packages/protocol/contracts/L2/TaikoL2.sol (8 hunks)

Additional comments: 3

packages/protocol/contracts/L1/TaikoL1.sol (1)

• 35-35: The contract TaikoL1 now implements multiple interfaces and inherits from EssentialContract. This change simplifies the inheritance structure and consolidates functionalities. Ensure that all functionalities previously provided by the removed interfaces are now adequately covered within TaikoL1 or its inherited contracts.

packages/protocol/contracts/L2/TaikoL2.sol (2)

• 22-23: The import statements for ISignalService and LibSignals indicate an enhancement in the signal service mechanism, aligning with the PR objectives. Ensure that these new imports are utilized effectively within the contract to leverage relayed chain data for signal values.
• 144-151: The logic within the anchor function to sync chain data using the ISignalService is a key enhancement, facilitating multi-hop bridging. This change aligns with the PR objectives to improve the signal service mechanism. Ensure that the implementation correctly handles edge cases and error conditions, such as failed service resolutions or signal syncing issues.