Globally Unique Message IDs #209

michaelkaplan13 · 2023-12-20T23:04:39Z

Note: The diff of this PR is relatively large due to the number of tests that needed to be updated to account for a type change from uint256 to bytes32. When reviewing, I recommend starting with ITeleporterMessenger.sol and TeleporterMessenger.sol as those files contain the only core changes.

Why this should be merged

Globally unique Teleporter message IDs will provide a significant UX improvement for querying and debugging Teleporter messages.

How it works

Currently the TeleporterMessenger contract assigns a unique ID to each message sent by track how many messages have been sent to a given destination chain, and incrementing the "message ID" to be used messages sent to that chain by each time a message is sent to it. The message ID is used when TeleporterMessenger receives a message to provide replay protection by ensuring that the given message has never been received in the past.

While this is sufficient for the TeleporterMessenger contract itself, these message IDs are not unique across chains or TeleporterMessenger versions. The first message from one TeleporterMessenger instance to any other chain will alway have a message ID of 1, etc. On a global Avalanche level, in order to identify a specific Teleporter message, you must specify:

The origin blockchain ID
The destination blockchain ID
The Teleporter contract address
The message ID assigned to the message.

This makes indexing and querying for Teleporter messages more cumbersome.

In this PR, the unique messageID assigned to each message by the TeleporterMessenger contract is changed from a uint256 value to a bytes32 value. Instead of being a sequence number of the number of message that have been sent by the TeleporterMessenger instance to another blockchain ID, it is computed as:

keccak256(<teleporter_contract_address>, <current_blockchain_id>, <destination_blockchain_id>, <message_nonce>)

The message_nonce above is tracked as the total number of message sent from the given TeleporterMessenger instance to any other blockchain ID. The resulting bytes32 message ID is guaranteed to be unique for each message sent by the contract given that the message_nonce value will be different for every message. It can be used in the same exact way as the previous message ID uint256 was to provide replay protection.

The new message ID hash is not included in the TeleporterMessage that is sent to the destination. Only the message nonce used to create the message ID hash is included in the message. When receiveCrossChainMessage is called to receive a message on the destination, the message ID hash is recomputed by the receiving chain using the source blockchain ID of the Warp message, and the message nonce included in the message. This ensures that message IDs received from different chains will be unique, even if a sending subnet is corrupted.

Similarly, message receipts now only track message nonces, and the ID for the corresponding messages are recomputed when the receipt is received back on the origin chain. Destination chains are not able to forge receipts for messages that were sent to different chains because of the fact that the message ID depends on the destinationBlockchainID. If a corrupted subnet were to send a receipt with a message nonce for a message that was sent to a different chain, when that receipt is received back on the origin chain, the resulting message ID would not match any message that had been sent, and the "forged" receipt is effectively a no-op.

In order to account for sendSpecificedReceipts still taking message IDs as a parameter, which is the only value easily available to users, TeleporterMessenger now tracks _receivedMessageNonces to map received message IDs to the their nonce, and allow for looking up the nonce value to send for specific receipts. This mapping is know used to provide replay protection rather than relayerRewardAddresses, since message nonces will never be 0. This allows for removing the requirement that the relayerRewardAddress provided to receiveCrossChainMessage be non-zero.

These unique message IDs allow for simplifying the ITeleporterMessenger interface to no longer need the destinationBlockchainID as a parameter for many functions in order to look up a sent message.

How this was tested

Unit tests, E2E test

How is this documented

TODO

… tests

Co-authored-by: cam-schultz <[email protected]> Signed-off-by: Michael Kaplan <[email protected]>

…ge-id

Co-authored-by: Geoff Stuart <[email protected]> Signed-off-by: Michael Kaplan <[email protected]>

minghinmatthewlam · 2023-12-20T23:14:15Z

contracts/src/Teleporter/ITeleporterMessenger.sol


    /**
     * @dev Emitted when a Teleporter message is being delivered on the destination chain to an address,
     * but message execution fails. Failed messages can then be retried with `retryMessageExecution`
     */
    event MessageExecutionFailed(
-        bytes32 indexed originBlockchainID, uint256 indexed messageID, TeleporterMessage message
+        bytes32 indexed messageID, bytes32 indexed originBlockchainID, TeleporterMessage message


not sure if we'd care about originBlockchainID in this event and MessageExecuted if message ids are unique

Yeah I thought about removing it entirely, but think it could still be helpful in tracking down the source chain message since that is not known for a given messageID even though the message ID is unique.

contracts/src/Teleporter/TeleporterMessenger.sol

minghinmatthewlam · 2023-12-20T23:47:31Z

contracts/src/Teleporter/TeleporterMessenger.sol

-    function getNextMessageID(bytes32 destinationBlockchainID) external view returns (uint256) {
-        return _getNextMessageID(destinationBlockchainID);
+    function getNextMessageID(bytes32 destinationBlockchainID) external view returns (bytes32) {
+        bytes32 blockchainID_ = blockchainID;


what's the decision between this and calling initializeBlockChainID

If we call initializeBlockChainID, this function could not be view

I think I'm leaning towards initializeBlockChainID, since we'll want to intiailize it eventually, don't think having view is a must, and right now you can't getNextMessageID until it is initialized, so user's can't call it for messageNonce=1.

I feel pretty strongly about keeping any get method as view actually. I think it'd be an anti-pattern to have a get method alter the state of the contract. I agree it's odd to not be able to call getNextMessageID before a message has been sent or received though. I think the best option here is to just make initializeBlockchainID public, so that it can be explicitly initialized before sending/receiving messages, if desired.

good call, I rethought about it and it is good to have get methods as read only on state. Agree that we can just have initializeBlockchainID public.

minghinmatthewlam · 2023-12-20T23:49:47Z

contracts/src/Teleporter/TeleporterMessenger.sol

+     * value from the Warp precompile.
+     * @return The current blockchain ID.
+     */
+    function _initializeBlockchainID() private returns (bytes32) {


we initialized move the blockchainID initialization out of constructor for deployment reasons, but now that we call initialize on a bunch of places, what is the difference with setting it in the state variable declaration again?. i.e.
bytes32 public immutable blockchainID = WARP_MESSENGER.getBlockchainID();

The state variable would still be initialized during deployment, so the same consideration around WARP_MESSENGER.getBlockchainID reverting would still apply.

contracts/src/Teleporter/TeleporterMessenger.sol

…er into use-nonce-for-receipts

geoff-vball

👍 LGTM!

bernard-avalabs

Generally makes sense to me. I've left a few minor comments.

bernard-avalabs · 2023-12-28T15:56:54Z

contracts/src/Teleporter/ITeleporterMessenger.sol

@@ -8,7 +8,7 @@ pragma solidity 0.8.18;
 // A message receipt identifies the message ID that was delivered


Should the comment be updated to refer to a message nonce instead of a message ID?

bernard-avalabs · 2023-12-28T16:03:16Z

contracts/src/Teleporter/TeleporterMessenger.sol

+    mapping(bytes32 messageID => bytes32 messageHash) public receivedFailedMessageHashes;
+
+    // Tracks the message nonce for each message that has been received. The key is the message ID,
+    // and the value is the nonce value that was received as a part for that message.


typo: "as a part for" -> "as a part of"

bernard-avalabs · 2023-12-28T16:04:30Z

contracts/src/Teleporter/TeleporterMessenger.sol

+
+    // Tracks the message nonce for each message that has been received. The key is the message ID,
+    // and the value is the nonce value that was received as a part for that message.
+    // Note: the `messageNonce` values are also used to determine if a given message has been received or not.


Should a slightly more extended explanation of the messageNonce be provided here? This might be the first point that someone comes across the term messageNonce.

I think the comment above for messageNonce on lines 50-53 explain it in more detail, but happy to expand on it there if you think there's a piece that's missing.

bernard-avalabs · 2023-12-28T16:21:20Z

contracts/src/Teleporter/TeleporterMessenger.sol

+        // If the blockchain ID has yet to be initialized, no messages have ever been received by
+        // this contract, meaning that the receipt(s) will not be found in any event.
+        // Thus, don't need to initialize the blockchain ID here.
+        bytes32 blockchainID_ = blockchainID;


I agree in general that we should avoid code-level optimizations for small gas savings if they hurt readability. It really should be the job of the compiler to perform these optimizations. With that said, because of the reverts that Michael pointed out, the functions will need comments about initialized/uninitialized variables no matter which way we go. So perhaps just having the comments is enough.

minghinmatthewlam

LGTM 🚢

openzeppelin-code · 2024-01-03T17:01:46Z

Globally Unique Message IDs

Generated at commit: 01121aec5d9675add6a27d89e0ac1673bc35a946

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	0 0 0 6 25 31
Dependencies	Critical High Medium Low Note Total	0 0 0 1 33 34

For more details view the full report in OpenZeppelin Code Inspector

cam-schultz

LGTM!

michaelkaplan13 and others added 21 commits December 14, 2023 16:54

Change message ID type to bytes32

051d9f6

Update ABI Bindings

6d76d61

Fix unit tests

6bb4068

Fix SendSpecifiedReceiptsTests

a121c7c

Fix unit tests

890707c

Merge remote-tracking branch 'origin/main' into hash-message-id

0db5131

Message ID does not depend on destination chain. Fix GetNextMessageID…

01bb6e9

… tests

Update ABI bindings

14d8367

Fix E2E tests

03f1409

Clean up unit tests

3ca0822

Pulled latest main

0a8e995

Fix linter and ABI bindings

666bff7

Add _initializeBlockchainID helper. Fix merge

8510307

Update contracts/src/Teleporter/ITeleporterMessenger.sol

96d14d6

Co-authored-by: cam-schultz <[email protected]> Signed-off-by: Michael Kaplan <[email protected]>

Fix lint and ABI bindings

fe68064

Recompute hash when receiving

2cc5149

Merge branch 'main' of github.com:ava-labs/teleporter into hash-messa…

92af6bc

…ge-id

Update contracts/src/Teleporter/TeleporterMessenger.sol

8e780bf

Co-authored-by: Geoff Stuart <[email protected]> Signed-off-by: Michael Kaplan <[email protected]>

Fix abi bindings

74548de

Use nonce in receipts. Simplify interface

2cbeacd

Fix typos in comments

6eae283

michaelkaplan13 requested review from cam-schultz, minghinmatthewlam, gwen917, geoff-vball and bernard-avalabs as code owners December 20, 2023 23:04

michaelkaplan13 self-assigned this Dec 20, 2023

minghinmatthewlam reviewed Dec 20, 2023

View reviewed changes

Pulled main

9e7ea7c

minghinmatthewlam reviewed Dec 20, 2023

View reviewed changes

michaelkaplan13 added 3 commits December 27, 2023 11:27

Merge branch 'use-nonce-for-receipts' of github.com:ava-labs/teleport…

81deb90

…er into use-nonce-for-receipts

Update comments

59f59ad

Update comments

b504e53

geoff-vball previously approved these changes Dec 27, 2023

View reviewed changes

bernard-avalabs reviewed Dec 28, 2023

View reviewed changes

minghinmatthewlam previously approved these changes Jan 2, 2024

View reviewed changes

michaelkaplan13 added 2 commits January 3, 2024 11:45

Pulled main

41c64fb

Update comments

77c1831

michaelkaplan13 dismissed stale reviews from minghinmatthewlam and geoff-vball via 77c1831 January 3, 2024 16:51

cam-schultz mentioned this pull request Jan 3, 2024

Integrate message ID hash ava-labs/icm-services#123

Closed

minghinmatthewlam previously approved these changes Jan 3, 2024

View reviewed changes

cam-schultz previously approved these changes Jan 3, 2024

View reviewed changes

Pulled main

8ea6f20

michaelkaplan13 dismissed stale reviews from cam-schultz and minghinmatthewlam via 8ea6f20 January 3, 2024 19:52

michaelkaplan13 requested review from bernard-avalabs, minghinmatthewlam, geoff-vball and cam-schultz January 3, 2024 19:52

cam-schultz previously approved these changes Jan 3, 2024

View reviewed changes

Add BlockchainIDInitializedEvent. Fix docker example scripts

01121ae

michaelkaplan13 dismissed cam-schultz’s stale review via 01121ae January 3, 2024 20:44

michaelkaplan13 requested a review from cam-schultz January 3, 2024 20:55

cam-schultz approved these changes Jan 3, 2024

View reviewed changes

minghinmatthewlam approved these changes Jan 3, 2024

View reviewed changes

michaelkaplan13 merged commit 188cfef into main Jan 3, 2024
15 checks passed

michaelkaplan13 deleted the use-nonce-for-receipts branch January 3, 2024 21:24

minghinmatthewlam pushed a commit that referenced this pull request Oct 1, 2024

Bump google.golang.org/grpc from 1.64.0 to 1.64.1 (#209)

a80eb8d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Globally Unique Message IDs #209

Globally Unique Message IDs #209

michaelkaplan13 commented Dec 20, 2023 •

edited

Loading

minghinmatthewlam Dec 20, 2023

michaelkaplan13 Dec 20, 2023

minghinmatthewlam Dec 20, 2023

cam-schultz Dec 21, 2023

minghinmatthewlam Dec 21, 2023

michaelkaplan13 Dec 22, 2023

minghinmatthewlam Dec 22, 2023

minghinmatthewlam Dec 20, 2023

cam-schultz Dec 21, 2023

geoff-vball left a comment

bernard-avalabs left a comment

bernard-avalabs Dec 28, 2023

bernard-avalabs Dec 28, 2023

bernard-avalabs Dec 28, 2023

michaelkaplan13 Jan 3, 2024

bernard-avalabs Dec 28, 2023

minghinmatthewlam left a comment

openzeppelin-code bot commented Jan 3, 2024 •

edited

Loading

cam-schultz left a comment

		@@ -8,7 +8,7 @@ pragma solidity 0.8.18;
		// A message receipt identifies the message ID that was delivered

Globally Unique Message IDs #209

Globally Unique Message IDs #209

Conversation

michaelkaplan13 commented Dec 20, 2023 • edited Loading

Why this should be merged

How it works

How this was tested

How is this documented

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoff-vball left a comment

Choose a reason for hiding this comment

bernard-avalabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minghinmatthewlam left a comment

Choose a reason for hiding this comment

openzeppelin-code bot commented Jan 3, 2024 • edited Loading

Globally Unique Message IDs

🚨 Report Summary

cam-schultz left a comment

Choose a reason for hiding this comment

michaelkaplan13 commented Dec 20, 2023 •

edited

Loading

openzeppelin-code bot commented Jan 3, 2024 •

edited

Loading