-
Notifications
You must be signed in to change notification settings - Fork 34
LO2302 support timeout #158
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
base: main
Are you sure you want to change the base?
Conversation
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 packet timeout functionality as part of LO2302. The key changes include introducing a new SortUnrelayedPackets function in the naive strategy, updating the RelayService logic to use the sorted relay packets, and adding tests to validate timeout behavior.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
core/naive-strategy.go | Added SortUnrelayedPackets method with logic to mark timeout packets and adjust packet routing based on channel order. |
core/service.go | Updated RelayService to use the sorted relay packets for further processing. |
chains/tendermint/query.go | Introduced QueryNextSequenceReceive along with its underlying helper functions. |
core/chain.go | Added go:generate directive and QueryNextSequenceReceive to the Chain interface. |
core/strategies.go | Added SortUnrelayedPackets to the StrategyI interface. |
core/types.go | Added a new Sort field to the PacketInfo struct to support timeout sorting. |
core/provers.go | Updated type references to use the correct ibcexported package. |
core/service_test.go | Added and modified tests to validate the new timeout support in various scenarios. |
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.
@dai1975 I have put some comments.
bfb0b8a
to
ffdce65
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 introduces support for packet timeouts in the IBC relayer by adding a new field to the PacketInfo struct and implementing corresponding timeout processing methods. Key changes include dependency upgrades, the addition of a TimedOut field in PacketInfo with associated logic in ProcessTimeoutPackets, and updates to services and tests to cover timeout scenarios.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
go.mod | Upgraded dependencies (notably ibc-mock-client) and added new mocks. |
core/types.go | Added a new "TimedOut" field to PacketInfo to track timeout status. |
core/strategies.go | Introduced the ProcessTimeoutPackets method in the Strategy interface. |
core/naive-strategy.go | Added the implementation of ProcessTimeoutPackets with timeout logic. |
core/service.go | Updated to use ProcessTimeoutPackets output in the relay service. |
core/service_test.go | Expanded test coverage to include timeout scenarios. |
cmd/tx.go | Integrated timeout processing call in relay message command. |
chains/tendermint/query.go | Added QueryNextSequenceReceive for new timeout-related queries. |
Makefile | Added target for generating test mocks. |
Comments suppressed due to low confidence (1)
go.mod:19
- [nitpick] The dependency upgrade for 'ibc-mock-client' looks appropriate; ensure that all indirect dependencies are consistent to avoid potential version conflicts.
github.com/datachainlab/ibc-mock-client v0.4.3
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 a new timeout mechanism for IBC packets by introducing a new timed-out flag in the packet info and refining the relay service logic to process timeout packets.
- Upgraded dependency versions and added new test utilities in go.mod.
- Added a new TimedOut field to PacketInfo and implemented ProcessTimeoutPackets in the naive strategy.
- Updated service and command logic to call ProcessTimeoutPackets before relaying packets.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
go.mod | Updated dependency versions and added new modules for testing. |
core/types.go | Introduced a new TimedOut field to indicate packet timeout status. |
core/strategies.go | Added ProcessTimeoutPackets interface documentation. |
core/service.go | Updated to use the processed timeout packets for proper relay execution. |
core/provers.go | Adjusted type aliases for exported interfaces. |
core/naive-strategy.go | Added ProcessTimeoutPackets implementation and updated packet handling logic. |
cmd/tx.go | Integrated ProcessTimeoutPackets into the tx command flow. |
chains/tendermint/query.go | Implemented QueryNextSequenceReceive for proper timeout handling. |
Makefile | Added a generate target for test mocks. |
Comments suppressed due to low confidence (1)
core/naive-strategy.go:273
- [nitpick] In ordered channels, using a break after processing the first timed-out packet implies that only one such packet will be handled. Please confirm that this behavior is intended.
if i == 0 { dstPackets = append(dstPackets, p) }
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 timeout support to packet processing by introducing a new boolean field (TimedOut) to PacketInfo, a ProcessTimeoutPackets method in the strategy interface and its implementation, and updates commands/tests and related components accordingly.
- Updated module dependencies in go.mod.
- Added timeout logic in core/types.go, core/strategies.go, and core/naive-strategy.go.
- Updated service and test code to integrate the new timeout processing.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
go.mod | Bumped dependency versions and added new dependencies. |
core/types.go | Added TimedOut field to PacketInfo. |
core/strategies.go | Introduced ProcessTimeoutPackets in the strategy interface. |
core/service.go | Updated relay service to use processed timeout packets. |
core/naive-strategy.go | Implemented ProcessTimeoutPackets and updated message collection for timeout packets. |
core/provers.go | Adjusted imports and type names for exported interfaces. |
core/service_test.go | Added tests to cover new timeout logic behavior. |
cmd/tx.go | Updated transaction command to process timeout packets. |
chains/tendermint/query.go | Added QueryNextSequenceReceive functionality. |
Makefile | Updated test target to generate mocks. |
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
Implements packet timeout processing end-to-end in the relayer, updating strategy, service, CLI, and tests
- Introduce
ProcessTimeoutPackets
inStrategyI
, implement timeout logic inNaiveStrategy
, and wire it intoRelayService
and CLI - Extend
PacketInfo
withTimedOut
flag and enhancecollectPackets
to emitMsgTimeout
for timed-out packets - Add thorough unit tests in
service_test.go
covering ordered-channel timeout scenarios
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
go.mod | Bump/mock dependencies required for new tests |
core/types.go | Add TimedOut field to PacketInfo |
core/strategies.go | Declare ProcessTimeoutPackets in the strategy interface |
core/naive-strategy.go | Implement ProcessTimeoutPackets and update collectPackets |
core/service.go | Integrate ProcessTimeoutPackets into the relay loop |
cmd/tx.go | Invoke ProcessTimeoutPackets in the CLI relay command |
chains/tendermint/query.go | Add QueryNextSequenceReceive implementation |
core/chain.go | Expose QueryNextSequenceReceive on the Chain interface |
core/provers.go | Alias import for ibcexported types |
core/service_test.go | Add unit tests for timeout behavior |
Makefile | Generate mocks before testing |
Comments suppressed due to low confidence (2)
core/service.go:122
- [nitpick] The variable name
pseqs2
is ambiguous; consider renaming it to something likeprocessedPackets
ortimeoutAwarePackets
for clarity.
pseqs2, err := srv.st.ProcessTimeoutPackets(ctx, srv.src, srv.dst, srv.sh, pseqs)
core/service_test.go:179
- Tests cover only ORDERED channels; consider adding test cases for UNORDERED channels to ensure timeout logic works in both modes.
func TestServe(t *testing.T) {
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.
@dai1975 I have put some comments. Please check them.
(I haven't review service_test.go
yet, but the other files have been reviewed)
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 implements packet timeout processing for IBC relaying by introducing timeout state tracking (TimedOut flag) in PacketInfo, updating the relay service to process timeout packets, and incorporating corresponding tests and dependency updates.
- Updated dependency versions and added new libraries in go.mod
- Added a TimedOut field to PacketInfo in core/types.go
- Implemented ProcessTimeoutPackets in core/naive-strategy.go and integrated it into service and tx commands
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
go.mod | Updated dependencies and removed duplicate indirect entries |
core/types.go | Added a new boolean field to track packet timeout status |
core/strategies.go | Introduced the ProcessTimeoutPackets method in the StrategyI interface |
core/service.go | Integrated timeout packet processing in the relay service |
core/provers.go | Renamed and updated import of IBC exported types |
core/naive-strategy.go | Added ProcessTimeoutPackets implementation and adjusted packet collection |
core/chain.go | Added a go:generate directive and new query method for next sequence receive |
cmd/tx.go | Incorporated timeout packet processing in the transaction relay command |
chains/tendermint/query.go | Added a helper method for querying the next sequence receive |
Makefile | Extended test mocks generation with go generate |
@dai1975 Could you resolve conflicts? |
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Daisuke Kanda <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
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
Implements packet timeout processing within the relay service, extending the naive strategy and updating all call sites and tests.
- Adds a
TimedOut
flag toPacketInfo
and implementsProcessTimeoutPackets
in the naive strategy to filter and mark timed-out packets. - Integrates timeout processing into the main service pipeline and CLI command before acknowledgements.
- Adds tests around timeout behavior and introduces new dependencies and mock generation.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
go.mod | Bump ibc-mock-client and add testify & mock deps |
core/types.go | Add TimedOut bool to PacketInfo |
core/strategies.go | Extend StrategyI with ProcessTimeoutPackets |
core/naive-strategy.go | Implement ProcessTimeoutPackets and timeout msgs |
core/service.go | Invoke ProcessTimeoutPackets in Serve |
cmd/tx.go | Invoke timeout processing in CLI relay command |
core/chain.go | Add QueryNextSequenceReceive to ICS04Querier |
chains/tendermint/query.go | Implement QueryNextSequenceReceive |
Makefile | Wire in go generate for mock chain before tests |
core/service_test.go | Add test cases for timeout behavior |
Comments suppressed due to low confidence (2)
core/naive-strategy.go:514
- Remove the stale TODO comment now that packet-timeout support has been implemented.
// TODO add packet-timeout support
core/service_test.go:182
- [nitpick] Tests currently only cover ORDERED channels; consider adding UNORDERED channel cases to verify timeout and relay behavior in that mode.
"ORDERED",
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.
@dai1975 I have added some comments on the core logic of timeout handling.
What do you think of this?
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
Signed-off-by: Daisuke Kanda <[email protected]>
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. Especially service_test.go
does and will contribute much to the code quality. Thank you!
(Excuse me, but I dismissed this approval)
Sorry for annoying, but I have a new question.
// In ordered channels, only the first timed-out packet is selected because | ||
// a timeout notification will close the channel. Subsequent packets cannot | ||
// be processed once the channel is closed. | ||
if i == 0 { |
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.
@dai1975 Sorry for late question, but ...
Why do we need this limitation (i == 0
)?
What happen if we remove this limitation?
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.
Before executing TimeoutPacket
for a packet on an ORDERED channel on ChainA, we need the following conditions.
- All preceding packets from ChainA have been
RecvPacket
'ed on ChainB. - The executions of
RecvPacket
all have been finalized on ChainB.
For checking these conditions, I think it is insufficient to check i == 0
, right?
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.
I guess, only if the first packet of packets returned from chain.QueryUnfinalizedRelayPackets
is timed out, we can execute TimeoutPacket
for the packet.
…rderd channel Signed-off-by: Daisuke Kanda <[email protected]>
Implements packet timeout processing.
Detailed change is described by copilot(bottom most review of copilot).