feat(protocol)!: cache cross-chain data in the signal service as signals #15736

dantaik · 2024-02-11T02:33:28Z

This PR shows the idea of improving the SignalService to achieve the following objectives:

Caching remote chain's state root or its signal service's storage root locally, inside the local signal service.
The proofs for multi-hop bridging can be full Merkle proofs, storage proof, or a mix of these two.
The transactor can specify exactly which chain/hop's signalRoot or stateRoot should be cached locally.
Caching a signalRoot/stateRoot is the same operation as relay the same root data to downstream chains, there is only one SSTORE, not two.

adaki2004 · 2024-02-11T08:06:44Z

2 general recommendations:

Still this will make A7 and A6 incompatible - better not to remove snippets from TaikoL2 just rename to snippets__deprecated, otherwise no upgrade possible.
Since it is an improvement in caching, better to base this on this branch since it has the MerkleProof verification working. Otherwise it will create again, lot of merge conflicts - especially because hop proof (structure) changed again - and it is not a straighforward , small change.

dantaik · 2024-02-11T09:42:16Z

2 general recommendations:

Still this will make A7 and A6 incompatible - better not to remove snippets from TaikoL2 just rename to snippets__deprecated, otherwise no upgrade possible.

A7 won't be compatible with A6 also because of other changes such as #15716 , b46269c, and ea33e65

Since it is an improvement in caching, better to base this on this branch since it has the MerkleProof verification working. Otherwise it will create again, lot of merge conflicts - especially because hop proof (structure) changed again - and it is not a straighforward , small change.
Will wait for your's PR to merge first.

…iko-mono into another_way_to_cache

adaki2004 · 2024-02-11T10:41:14Z

A7 won't be compatible with A6 also because of other changes such as #15716 , b46269c, and ea33e65

Oh yeah i see, tho 15716 is not causing incompatibility, but sure the rest does.
Anyway best to minimize the changes - if we dont want to run another public testnet - because we need to be sure there are no such changes where mass (public), stress testing could reveal any findings.

dantaik · 2024-02-11T10:52:49Z

A7 won't be compatible with A6 also because of other changes such as #15716 , b46269c, and ea33e65

Oh yeah i see, tho 15716 is not causing incompatibility, but sure the rest does. Anyway best to minimize the changes - if we dont want to run another public testnet - because we need to be sure there are no such changes where mass (public), stress testing could reveal any findings.

We'll talk about it in our sync up.

Brechtpd

I have to look more into it to understand what is happening.

The transactor can specify exactly which chain/hop's signalRoot or stateRoot should be cached locally

Should it be an option though?

Brechtpd · 2024-02-12T00:48:53Z

packages/protocol/contracts/L2/TaikoL2.sol

-        snippets[l1Height] = ICrossChainSync.Snippet({
-            remoteBlockId: l1Height,
-            syncedInBlock: uint64(block.number),
-            blockHash: l1BlockHash,
-            stateRoot: l1StateRoot
-        });


This does now make it impossible for smart contracts on L2 to just fetch this data from a smart contract or fetch specific L1 state roots. The block number and block hash info is now lost and unavailable, only now possible to verify that some L1 state root existed, not which and also not the corresponding block hash.

The applications that can make use of this info is low (thinking of things like axiom for historical data or some other alternative bridging solution), so probably not a big deal, still it felt good to have it there.

I also realized the differences. I think it's fine as most apps shall not assuming these data are available otherwise, the app will not be deployable on other chains.

But I do think it may be benefit to add the latest cached data and its block number to TaikoL1/L2 contract, and the cost is very small (writing to the same slot).

Indeed hard to predict how useful it will be.

But it all depends on what other L2s also make available, because the function to call to get the data will be different, but the dapp could depend on the data to be there easily by just pointing to the right contract for each chain. I haven't look too closely to what is available on optimism, but there at least the full block header of the last known L1 block is made available in a contract: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L2/L1Block.sol. That means that dapps that depend on this information can run on Optimism but would not be able to run on Taiko.

dong77 · 2024-02-12T01:57:21Z

I have to look more into it to understand what is happening.

The transactor can specify exactly which chain/hop's signalRoot or stateRoot should be cached locally

Should it be an option though?

I think so. As you said previously, if most the messages are processed by a relayer, the relayer should be smart enough to selectively cache things to lower his own cost; but normal users shall not pay extra fee to benefit others.

Brechtpd · 2024-02-12T22:22:23Z

packages/protocol/contracts/signal/SignalService.sol

+                (_signal,) = isFullProof
+                    ? _relayStateRoot( _chainId, hop.proof.rootHash)
+                    : _relaySignalRoot(_chainId, hop.proof.rootHash);


Should we ever cache the state root? Regardless if a full proof is provided or not, I think we should always just cache the signal root. Because I don't think the state root is anything useful here, we just store it from the rollup contract now because that's all that we have available, but otherwise I think it doesn't really have a purpose (at least not from the signal service perspective). Because whenever we have state root of a chain, the first thing we have to do is proof the signal root for that state root, because proving state inside the signal contract is all we care about.

And so if we cache the signal root here directly then great because we immediately have the signal root for that chain! If we cache the state root for the chain, then if somebody wants to use it, the first thing they would have to do is to still prove the signal root for that state root.

Brechtpd · 2024-02-12T22:47:15Z

packages/protocol/contracts/libs/LibTrieProof.sol


-        if (rlpAccount.length == 0) revert LTP_INVALID_ACCOUNT_PROOF();
+            if (rlpAccount.length == 0) revert LTP_INVALID_ACCOUNT_PROOF();


Suggested change

if (rlpAccount.length == 0) revert LTP_INVALID_ACCOUNT_PROOF();

if (rlpAccount.length < MIN_LENGTH_ACCOUNT_LEAF) revert LTP_INVALID_ACCOUNT_PROOF();

With uint256 public constant MIN_LENGTH_ACCOUNT_LEAF = 70; this a good check to additionally make sure that a storage root cannot be used as a state root.

For the storage value the maximum length is 33 so we can also add a check on that in the input.

Brechtpd · 2024-02-12T22:52:14Z

packages/protocol/contracts/signal/SignalService.sol

+    function _relayStateRoot(
+        uint64 chainId,
+        bytes32 stateRoot
+    )
+        internal
+        returns (bytes32 signal, bytes32 slot)
+    {
+        if (chainId == block.chainid) revert SS_INVALID_PARAMS();
+        signal = signalForStateRoot(chainId, stateRoot);
+
+        console2.log("send signa:", msg.sender, uint256(signal));
+
+        slot = _sendSignal(resolve("taiko",false), signal);
+        emit StateRootRelayed(chainId, stateRoot, signal);
+    }
+
+    function _relaySignalRoot(
+        uint64 chainId,
+        bytes32 signalRoot
+    )
+        internal
+        returns (bytes32 signal, bytes32 slot)
+    {
+        if (chainId == block.chainid) revert SS_INVALID_PARAMS();
+        signal = signalForSignalRoot(chainId, signalRoot);
+        slot = _sendSignal(resolve("taiko", false), signal);
+        emit SignalRootRelayed(chainId, signalRoot, signal);
+    }


So the purpose of these functions is to allow logging that these values are cached? Seems reasonable if so, I think otherwise it could go through the standard sendSignal API if I'm not mistaken.

Brechtpd · 2024-02-12T22:54:37Z

packages/protocol/contracts/signal/SignalService.sol

@@ -133,45 +152,70 @@ contract SignalService is EssentialContract, ISignalService {
        // 2. using the verified hop stateRoot to verify that the source app on chainA has sent a
        // signal using its own signal service.
        // We always verify the proofs in the reversed order (top to bottom).


// We always verify the proofs in the reversed order (top to bottom).

Should be bottom to top now right? Start at the chain that stores the signal and then go "upwards" towards a verified state root/signal root.

Brechtpd · 2024-02-12T23:04:37Z

packages/protocol/contracts/L2/TaikoL2.sol

-        snippets[l1Height] = ICrossChainSync.Snippet({
-            remoteBlockId: l1Height,
-            syncedInBlock: uint64(block.number),
-            blockHash: l1BlockHash,
-            stateRoot: l1StateRoot
-        });


Indeed hard to predict how useful it will be.

But it all depends on what other L2s also make available, because the function to call to get the data will be different, but the dapp could depend on the data to be there easily by just pointing to the right contract for each chain. I haven't look too closely to what is available on optimism, but there at least the full block header of the last known L1 block is made available in a contract: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/L2/L1Block.sol. That means that dapps that depend on this information can run on Optimism but would not be able to run on Taiko.

Brechtpd · 2024-02-12T23:10:53Z

I have to look more into it to understand what is happening.

The transactor can specify exactly which chain/hop's signalRoot or stateRoot should be cached locally

Should it be an option though?

I think so. As you said previously, if most the messages are processed by a relayer, the relayer should be smart enough to selectively cache things to lower his own cost; but normal users shall not pay extra fee to benefit others.

Yep I don't think in practice it will matter much, but mostly just thinking if we should have it as an option if it is indeed a niche use case. But it's a reasonably small code difference, so seems okay.

Caching a signalRoot/stateRoot is the same operation as relay the same root data to downstream chains, there is only one SSTORE, not two.

Could you give some more information about this because I'm not sure I understand? The approach in this PR seems similar to the one in Dani's PR but I could very well miss something.

dantaik added 4 commits February 11, 2024 10:33

LibSignals

a3c14b4

more

08038bd

remove unused code

163abdb

Merge branch 'remove_code' into another_way_to_cache

9af711b

dantaik changed the title ~~LibSignals~~ feat(protocol): try another idea to cache Feb 11, 2024

dantaik changed the base branch from main to remove_code February 11, 2024 02:40

dantaik added 4 commits February 11, 2024 10:50

more

80cdc1f

more

9c4dfd3

more

e9cdfb6

more

fe4eda7

dantaik requested review from Brechtpd and adaki2004 February 11, 2024 03:31

dantaik added 8 commits February 11, 2024 11:35

more

216b565

fmt

8c54e20

Update SignalService.sol

4380fbe

Update SignalService.sol

6c9da20

more

7806d77

delete files

7517d7d

Update SignalService.sol

19b16a6

Update SignalService.sol

7640159

Base automatically changed from remove_code to main February 11, 2024 03:58

dantaik and others added 5 commits February 11, 2024 11:59

Update SignalService.sol

f85cc11

improve

c462319

Merge branch 'main' into another_way_to_cache

1668e49

Update SignalService.sol

40ddb74

Update SignalService.sol

731df4f

Merge branch 'main' into another_way_to_cache

4d33026

dantaik changed the title ~~feat(protocol): try another idea to cache~~ feat(protocol)!: try another idea to cache Feb 11, 2024

dantaik added 4 commits February 11, 2024 18:00

test

a600f92

Merge branch 'another_way_to_cache' of https://github.com/taikoxyz/ta…

c4326cf

…iko-mono into another_way_to_cache

Update Bridge.t.sol

244de26

test

f4ec267

dantaik changed the title ~~feat(protocol)!: try another idea to cache~~ feat(protocol)!: cache cross-chain data in the signal service as signals Feb 11, 2024

dantaik and others added 2 commits February 11, 2024 21:23

Merge branch 'main' into another_way_to_cache

37b9480

more

62b6f3d

Brechtpd reviewed Feb 12, 2024

View reviewed changes

dantaik and others added 6 commits February 12, 2024 10:03

Merge branch 'main' into another_way_to_cache

c4ca6cb

more

5dc4d49

Update SignalService.sol

550ba67

more

b34cf02

more

c0de501

Update SignalService.t.sol

08610b3

Brechtpd reviewed Feb 12, 2024

View reviewed changes

dantaik closed this Feb 13, 2024

Brechtpd mentioned this pull request Feb 13, 2024

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

Merged

dantaik deleted the another_way_to_cache branch March 1, 2024 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(protocol)!: cache cross-chain data in the signal service as signals #15736

feat(protocol)!: cache cross-chain data in the signal service as signals #15736

dantaik commented Feb 11, 2024 •

edited

Loading

adaki2004 commented Feb 11, 2024 •

edited

Loading

dantaik commented Feb 11, 2024

adaki2004 commented Feb 11, 2024

dantaik commented Feb 11, 2024

Brechtpd left a comment

Brechtpd Feb 12, 2024

dong77 Feb 12, 2024

Brechtpd Feb 12, 2024

dong77 commented Feb 12, 2024 •

edited by dantaik

Loading

Brechtpd Feb 12, 2024

Brechtpd Feb 12, 2024 •

edited

Loading

Brechtpd Feb 12, 2024

Brechtpd Feb 12, 2024

Brechtpd Feb 12, 2024

Brechtpd commented Feb 12, 2024


		if (rlpAccount.length == 0) revert LTP_INVALID_ACCOUNT_PROOF();
		if (rlpAccount.length == 0) revert LTP_INVALID_ACCOUNT_PROOF();

	if (rlpAccount.length == 0) revert LTP_INVALID_ACCOUNT_PROOF();
	if (rlpAccount.length < MIN_LENGTH_ACCOUNT_LEAF) revert LTP_INVALID_ACCOUNT_PROOF();

feat(protocol)!: cache cross-chain data in the signal service as signals #15736

feat(protocol)!: cache cross-chain data in the signal service as signals #15736

Conversation

dantaik commented Feb 11, 2024 • edited Loading

adaki2004 commented Feb 11, 2024 • edited Loading

dantaik commented Feb 11, 2024

adaki2004 commented Feb 11, 2024

dantaik commented Feb 11, 2024

Brechtpd left a comment

Choose a reason for hiding this comment

Brechtpd Feb 12, 2024

Choose a reason for hiding this comment

dong77 Feb 12, 2024

Choose a reason for hiding this comment

Brechtpd Feb 12, 2024

Choose a reason for hiding this comment

dong77 commented Feb 12, 2024 • edited by dantaik Loading

Brechtpd Feb 12, 2024

Choose a reason for hiding this comment

Brechtpd Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Brechtpd Feb 12, 2024

Choose a reason for hiding this comment

Brechtpd Feb 12, 2024

Choose a reason for hiding this comment

Brechtpd Feb 12, 2024

Choose a reason for hiding this comment

Brechtpd commented Feb 12, 2024

dantaik commented Feb 11, 2024 •

edited

Loading

adaki2004 commented Feb 11, 2024 •

edited

Loading

dong77 commented Feb 12, 2024 •

edited by dantaik

Loading

Brechtpd Feb 12, 2024 •

edited

Loading