-
Notifications
You must be signed in to change notification settings - Fork 22
Have MsgsBetweenSeqNums populate TxHash #1337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 rstout, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
9563ad1 to
ab672bc
Compare
|
|
It shouldn't be, the codecs are for encoding/decoding reports, and TxHash doesn't need to be included in the reports. The place that needs to populate TxHash is the chain readers, and this seems to be the case, so TxHash should be populated. |
ogtownsend
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
8f98529 to
7289c19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for populating the TxHash field in CCIP message headers, which is required for calling CCTPv2's HTTP API. The implementation includes a feature flag (PopulateTxHashEnabled) to control this behavior for backwards compatibility.
Key changes:
- Adds
PopulateTxHashEnabledconfiguration flag to control TxHash population - Updates
MsgsBetweenSeqNumsto populate TxHash fromtypes.Sequence.TxHashfield - Introduces a
ChainAccessorWrapperto conditionally clear TxHash based on the feature flag
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pluginconfig/execute.go | Adds PopulateTxHashEnabled configuration field |
| pkg/chainaccessor/default_accessor.go | Populates TxHash from sequence item using hexutil.Encode |
| pkg/chainaccessor/default_accessor_test.go | Comprehensive tests for TxHash population scenarios |
| execute/chainaccessor_wrapper.go | Wrapper implementation to conditionally clear TxHash |
| execute/chainaccessor_wrapper_test.go | Tests for wrapper's TxHash enable/disable logic |
| execute/factory.go | Integrates wrapper with configuration flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ef03c96 to
b3a4261
Compare
ogtownsend
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much cleaner, thanks!
|
We need TxHash in order to call CCTPv2's HTTP API: https://developers.circle.com/api-reference/cctp/all/get-messages-v-2
Naively setting
TxHashin message headers will cause OCR consensus issues, as seen when the Solana chain accessor populatedTxHash. In order to prevent consensus issues, settingTxHashis now behind a feature flag in the OffchainConfig (stored in CCIPHome). TheDefaultAccessornow populatesTxHashby default, butccipChainReaderclearsTxHashby default. This means initially, we'll setTxHashand then immediately clear it (maintaining the previous behavior of not having setTxHash).populateTxHashEnabledcan then be set for either Commit or Exec to then switch to the mode whereTxHashgets populated. This is done via an MCMS proposal to update CCIPHome config, which should be picked up by all nodes within 10s of each other and allow DONs to proceed without consensus issues.