feat(protocol)!: re-implement multi-hop bridging with optional caching

[dantaik]

This is the best mutli-hop bridging optimization I can achieve.

@KorbinianK @cyberhorsey @xiaodino please note that the getSyncedSnippet now does not take any paramter and only returns the latest synced data. Please verify if this is going to work for Bridge relayer/dapp.

@Brechtpd @adaki2004 Please review SignalService. It manages all cross-chain multi-hop bridging functions without relying another contract.

See https://github.com/taikoxyz/taiko-mono/blob/improve_signal_service_and_multihop/packages/protocol/docs/multihop_bridging_deployment.md

Summary by CodeRabbit

• New Features

  • Enhanced cross-chain data relay functionality with additional parameters for improved specificity and security.

• Refactor

  • Streamlined signal verification process for increased efficiency.
  • Simplified genesis contract setup for better maintainability.

• Chores

  • Removed unused variables and functions to clean up the codebase.

[dantaik]

Ok, after going through the code i have a question, with an example:

No multi-hop, just simple L1-> L2.

• Alice sends ether at blockheight 100.
• Bob sends ether at blockheight 100.

Carol (relayer) is an EOA who processes the messages on the destination chain and it obviously waits till L2 anchor syncs the L1 state root for (at least) l1 blockHeight 100 -> Then Carol is about to process indeed those messages with the appropriate proofs.

Carol has the right to NOT cache any data when first processes Alice's sending. But in such case, next time he also has to pay (again) for the full merkle proof (account + storage) data on-chain plus the calculation of the storage root in LibTrieProof.sol

So instead Carol (and other relayers for the same blockheight - which is now already proven tho not cached) pays X + Y amount of fees (where X> Y), Carol now pays X + X.

Why not making caching default and not optional ? 🤔 In such case, Carol (and every other relayer) would save money if they are submitting proofs for same blockheight.

I understand that there is an extra storage write during caching, which adds up to 4.8% of the original X cost (current ether bridge costs 430K for the relayer/EOA to process that message), so i guess that is the reason, right ? Maybe with this flexible option EOAs/relayers can configure if they want to, because maybe they are only processing 1-2 messages per 5-10 minutes so does not worth that extra 4.8% cost and dont want to pay tht so that others could bridge cheaper ?

In any case, maybe official taiko relayers could/should be 'smart relayers' and indeed use caching when load/demand is high?

Dani, I think you've got it right. "Smart relayers" will certainly try to figure out what to cache on the destination chain to save gas, but this requires some extra logics in the relayer code and even our own relayer is not ready now due to other priorities.

[coderabbitai]

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 429285e and 3207979.

Files ignored due to path filters (1)

• packages/protocol/foundry.toml is excluded by: !**/*.toml

Files selected for processing (1)

• packages/protocol/genesis/GenerateGenesis.g.sol (4 hunks)

Additional comments: 1

packages/protocol/genesis/GenerateGenesis.g.sol (1)

• 301-301: Changing getPredeployedContractAddress function visibility to private view limits extensibility but enhances encapsulation. Ensure this change doesn't impact other test functions or external dependencies.

[Brechtpd]

Dani, I think you've got it right. "Smart relayers" will certainly try to figure out what to cache on the destination chain to save gas, but this requires some extra logics in the relayer code and even our own relayer is not ready now due to other priorities.

I guess some potential use case for optional caching that could make things more efficient is to not cache certain intermediate hops because only certain chains have more signals that need to be proven. Indeed caching is not that significant of a cost, but as long as the code difference between caching and not caching is small like it currently is, it could be worth it for smart relayers so they can take advantage of these types of smart optimizations that do squeeze some extra gas savings out of this option.

I guess one benefit of the approach in Dani's PR is that the smart contract automatically skipped proving the account proof if the signal root was already cached. So even with "dumb" relayers, we were able to save the account proof from being verified onchain. But of course, that was still less than ideal because the relayer would still put the account proof onchain and pay the gas cost for that, so the relayer should really just be made smarter to really make things more efficient in the best way.

[coderabbitai]

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 3207979 and b3fa5db.

Files ignored due to path filters (1)

• packages/protocol/foundry.toml is excluded by: !**/*.toml

Files selected for processing (11)

• packages/protocol/contracts/L1/TaikoErrors.sol (1 hunks)
• packages/protocol/contracts/L1/TaikoL1.sol (1 hunks)
• packages/protocol/contracts/L1/libs/LibUtils.sol (3 hunks)
• packages/protocol/contracts/L1/libs/LibVerifying.sol (2 hunks)
• packages/protocol/contracts/L2/TaikoL2.sol (3 hunks)
• packages/protocol/contracts/signal/ISignalService.sol (1 hunks)
• packages/protocol/contracts/signal/SignalService.sol (3 hunks)
• packages/protocol/genesis/GenerateGenesis.g.sol (4 hunks)
• packages/protocol/script/DeployOnL1.s.sol (1 hunks)
• packages/protocol/test/TaikoTest.sol (2 hunks)
• packages/protocol/test/signal/SignalService.t.sol (3 hunks)

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

• packages/protocol/contracts/L1/libs/LibVerifying.sol
• packages/protocol/contracts/L2/TaikoL2.sol
• packages/protocol/genesis/GenerateGenesis.g.sol
• packages/protocol/script/DeployOnL1.s.sol
• packages/protocol/test/TaikoTest.sol

Additional comments: 35

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

• 32-32: LGTM!

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

• 19-22: LGTM!
• 24-36: LGTM!
• 39-51: LGTM!
• 53-67: LGTM!

packages/protocol/contracts/L1/libs/LibUtils.sol (3)

• 17-20: LGTM!
• 28-28: LGTM!
• 75-93: > 📝 NOTE

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

LGTM!

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

• 21-21: LGTM!
• 27-31: LGTM!
• 34-39: LGTM!
• 44-46: LGTM!
• 48-54: LGTM!
• 63-72: LGTM!
• 76-77: LGTM!
• 83-127: LGTM!
• 130-138: LGTM!
• 143-152: LGTM!
• 173-183: LGTM!
• 185-196: LGTM!
• 198-204: LGTM!
• 206-225: LGTM!
• 231-254: LGTM!

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

• 178-178: LGTM!

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

• 42-57: > 📝 NOTE

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

The setup function correctly replaces SignalServiceForTest with MockSignalService and updates deployment logic. Ensure all dependent tests are updated to reflect this change.

• 124-148: The test test_SignalService_proveSignalReceived_revert_src_signal_service_not_registered correctly simulates a scenario where the source signal service is not registered. This test is well-structured and follows best practices for clarity and maintainability.
• 150-165: The test test_SignalService_proveSignalReceived_revert_zero_size_proof effectively checks for empty proof arrays. This is a critical test case for ensuring the contract's robustness against invalid inputs.
• 168-185: The test test_SignalService_proveSignalReceived_revert_last_hop_incorrect_chainid accurately tests for incorrect chain IDs in the last hop. This is important for ensuring the integrity of multi-hop bridging.
• 188-204: The test test_SignalService_proveSignalReceived_revert_mid_hop_incorrect_chainid checks for incorrect chain IDs in mid hops. This test is crucial for validating the logic of multi-hop proof verification.
• 208-231: The test test_SignalService_proveSignalReceived_revert_mid_hop_not_registered simulates a scenario where a mid-hop signal service is not registered. This test enhances the contract's test coverage by checking for edge cases in multi-hop bridging.
• 235-266: The test test_SignalService_proveSignalReceived_local_chaindata_not_found covers scenarios where local chain data is not found. This test is essential for ensuring the contract can handle missing data gracefully.
• 269-301: The test test_SignalService_proveSignalReceived_one_hop_cache_signal_root introduces caching logic into the testing suite. Ensure this test accurately reflects the intended caching behavior and its impact on the contract's functionality.
• 303-344: The test test_SignalService_proveSignalReceived_one_hop_state_root correctly tests the caching of state roots. This test is important for validating the caching logic's correctness and its integration with the contract's existing functionality.
• 346-411: The test test_SignalService_proveSignalReceived_multiple_hops effectively simulates a multi-hop bridging scenario with various proof types. This test is crucial for ensuring the contract's multi-hop bridging functionality works as expected.
• 413-526: The test test_SignalService_proveSignalReceived_multiple_hops_caching thoroughly tests the caching functionality across multiple hops. This test is comprehensive and critical for ensuring the caching logic works correctly in complex multi-hop scenarios.

[dantaik]

Dani, I think you've got it right. "Smart relayers" will certainly try to figure out what to cache on the destination chain to save gas, but this requires some extra logics in the relayer code and even our own relayer is not ready now due to other priorities.

I guess some potential use case for optional caching that could make things more efficient is to not cache certain intermediate hops because only certain chains have more signals that need to be proven. Indeed caching is not that significant of a cost, but as long as the code difference between caching and not caching is small like it currently is, it could be worth it for smart relayers so they can take advantage of these types of smart optimizations that do squeeze some extra gas savings out of this option.

I guess one benefit of the approach in Dani's PR is that the smart contract automatically skipped proving the account proof if the signal root was already cached. So even with "dumb" relayers, we were able to save the account proof from being verified onchain. But of course, that was still less than ideal because the relayer would still put the account proof onchain and pay the gas cost for that, so the relayer should really just be made smarter to really make things more efficient in the best way.

I actually gave some thought to the idea of auto-optimizing the onchain verification logics even if the proof is redudant, but later I told myself: lets not do it in the smart contract. If the relayer can be optimized to do it, it should be done offchain.

[Brechtpd]

Optimistic approval. I don't think anybody will cache state roots (#15761 (comment)) so would still like to hear the expected use case, but in any case it doesn't hurt to have the option.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between b3fa5db and 32057a2.

Files selected for processing (5)

• packages/protocol/contracts/L1/libs/LibVerifying.sol (2 hunks)
• packages/protocol/contracts/L2/TaikoL2.sol (3 hunks)
• packages/protocol/contracts/bridge/Bridge.sol (2 hunks)
• packages/protocol/genesis/GenerateGenesis.g.sol (4 hunks)
• packages/protocol/test/bridge/Bridge.t.sol (1 hunks)

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

• packages/protocol/contracts/L1/libs/LibVerifying.sol
• packages/protocol/contracts/L2/TaikoL2.sol
• packages/protocol/contracts/bridge/Bridge.sol
• packages/protocol/genesis/GenerateGenesis.g.sol
• packages/protocol/test/bridge/Bridge.t.sol