off-chain mismatch and initialize msgHasher in ccip provider #554
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses an off-chain mismatch issue and initializes the message hasher component in the CCIP provider. The changes ensure proper initialization of the MessageHasher and update handling of messages with empty token amounts.
Changes:
- Initializes MessageHasher in the CCIP provider configuration
- Updates token amount handling to avoid unnecessary allocations when empty
- Re-enables and fixes a test case for messages without extra args
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/ccip/provider/provider.go | Adds MessageHasher initialization to the CCIP provider |
| pkg/ccip/codec/msghasher_test.go | Re-enables test case for messages without extra args |
| pkg/ccip/codec/executecodec.go | Conditionally allocates token amounts slice only when needed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if tokenAmount.Amount.IsEmpty() { | ||
| return nil, fmt.Errorf("empty amount for token: %s", tokenAmount.DestTokenAddress) | ||
| } | ||
| var tokenAmounts []ocr.Any2TVMTokenTransfer |
There was a problem hiding this comment.
Could we add a comment here to clarify why explicit nil initialization is important?
This would help prevent accidentally reintroducing the bug in the future.
// Explicit nil for tokenAmounts is necessary for deterministic hash calculation to match with contracts.
// A nil slice encodes as absent optional reference, while make([]T, 0, ...) encodes as present-but-empty,
// causing hash mismatches and Merkle proof verification failures.
There was a problem hiding this comment.
Think that report analysis was not true, we don't have token transfer and the msgHasher is returning the same on-chain otherwise the msg report will not be accepted in our staging env. This is more like consistency improvement, not actually fixing bug.
There was a problem hiding this comment.
I believe the ccip report cell gets parsed to stack values in TVM and the hashing logic matches with the msgHasher off-chain. So having empty array is okay in the cell.
There was a problem hiding this comment.
Actually I think this does cause hash mismatch, I tested it locally, but I'm not sure why we never ran into this issue in staging msg passing test
There was a problem hiding this comment.
Agree that it's weird that if this i a real issue we have not seen it happen live.
There was a problem hiding this comment.
Thanks to @vicentevieytes, he found that the contract transmitter is decoding and encoding the execute report again after the executecodec, and "normalizing" empty slices to nil, which accidentally makes the hashes match.
We should fix the msgHasher and executecodec misalignment here, and good to understand how it worked previously. cc @archseer @krebernisak
….com:smartcontractkit/chainlink-ton into NONEVM-2350/offchain-execute-report-mismatch
vicentevieytes
left a comment
vicentevieytes
left a comment
There was a problem hiding this comment.
I think we can merge this. I am still unsure of why this did not cause an issue before but given that the tests were passing before and are passing now I am not too worried about it
huangzhen1997
deleted the
NONEVM-2350/offchain-execute-report-mismatch
branch
5 participants
No description provided.