[Relayminer] refactor: relayerSessionsManager#waitForBlock()#648
Conversation
bryanchriswhite
added
bug
WalkthroughThe changes primarily involve renaming a parameter related to buffer capacity across multiple files for clarity and consistency. Additionally, there are updates to handle nil cases in relayer sessions and enhancements in test cases to improve coverage and reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Observable
participant RelayerSessionsManager
Client->>Observable: Call MapReplay with replayBufferCap
Observable->>Observable: Store replayBufferCap
Observable->>Client: Return ReplayObservable
Client->>RelayerSessionsManager: Trigger claim creation
RelayerSessionsManager->>RelayerSessionsManager: Check claimsWindowOpenBlock
alt claimsWindowOpenBlock is nil
RelayerSessionsManager->>Client: Log warning and send signal
else claimsWindowOpenBlock is valid
RelayerSessionsManager->>Client: Process claim
end
Client->>RelayerSessionsManager: Trigger proof creation
RelayerSessionsManager->>RelayerSessionsManager: Check proofsWindowOpenBlock
alt proofsWindowOpenBlock is nil
RelayerSessionsManager->>Client: Log warning and handle failure
else proofsWindowOpenBlock is valid
RelayerSessionsManager->>Client: Process proof
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 648) |
bryanchriswhite
force-pushed
the
issues/553/fix/relayer-session-manager
branch
from
a3fb7b3 to
ed9f13e
Compare
…elayer-session-manager * issues/553/fix/replay-observable: chore: simplify ReplayObservable#SubscribefromLatestBufferedOffset()
…elayer-session-manager * issues/553/fix/replay-observable: fix: failing tests
Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
…elayer-session-manager * issues/553/fix/replay-observable: chore: go imports chore: update comment chore: update comment chore: review feedback improvements [Relayminer] refactor: query `min_relay_difficulty_bits` param (#655) [Docs] Claim expiration (#649) [LoadTest] Passing non-existing plans variable (#661) Tiny: updating labels in tiltfile [Load Testing] fix: relay stress test duration calculation (#651) [LocalNet] Grafana stress test dashboard changes (#641)
red-0ne
left a comment
red-0ne
left a comment
There was a problem hiding this comment.
Small NIT, otherwise LGTM.
…elayer-session-manager
Co-authored-by: Redouane Lakrache <r3d0ne@gmail.com>
…elayer-session-manager
bryanchriswhite
changed the base branch from
issues/553/fix/replay-observable
to
main
…session-manager * pokt/main: [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
pkg/relayer/session/session_test.go (1)
164-168: Consider centralizing the sleep duration inwaitSimulateIO.The sleep duration of 50 milliseconds is hardcoded. Consider centralizing it in a local constant or function for better maintainability and readability.
- time.Sleep(50 * time.Millisecond) + const sleepDuration = 50 * time.Millisecond + time.Sleep(sleepDuration)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- pkg/observable/channel/map.go (1 hunks)
- pkg/observable/channel/replay.go (7 hunks)
- pkg/observable/channel/replay_test.go (8 hunks)
- pkg/observable/interface.go (1 hunks)
- pkg/relayer/session/claim.go (1 hunks)
- pkg/relayer/session/proof.go (1 hunks)
- pkg/relayer/session/session.go (1 hunks)
- pkg/relayer/session/session_test.go (4 hunks)
Files skipped from review due to trivial changes (1)
- pkg/observable/interface.go
Additional context used
GitHub Check: misspell
pkg/relayer/session/proof.go
[warning] 106-106: [misspell] pkg/relayer/session/proof.go#L106
"whic" is a misspelling of "which"
Additional comments not posted (32)
pkg/observable/channel/map.go (2)
71-75: The changes look good!The function parameter has been correctly renamed to
replayBufferCap, and the logic is consistent with the intended functionality.
67-68: Update the parameter name in the function signature.The parameter name in the comment has been updated to
replayBufferCap, but the function parameter itself should also be updated to maintain consistency.- // The resulting observable will receive the last replayBufferCap + // The resulting observable will receive the last replayBufferSizeLikely invalid or redundant comment.
pkg/relayer/session/session_test.go (11)
53-53: The changes look good!The setup for the dependencies in the
TestRelayerSessionsManager_Startfunction is correctly implemented.
103-105: The changes look good!The wait for the relayer sessions manager to start is correctly implemented using
waitSimulateIO.
110-111: The changes look good!The wait for the relayer sessions manager to create a session tree is correctly implemented using
waitSimulateIO.
118-119: The changes look good!The wait for the relayer sessions manager to process non-actionable blocks is correctly implemented using
waitSimulateIO.
124-126: The changes look good!The calculation and emission of the claim window open height are correctly implemented.
132-134: The changes look good!The emission of the claim open height block to the blockPublishCh is correctly implemented.
135-136: The changes look good!The wait for the relayer sessions manager to process the claim open height block is correctly implemented using
waitSimulateIO.
141-142: The changes look good!The wait for the relayer sessions manager to process the trigger claim block is correctly implemented using
waitSimulateIO.
145-148: The changes look good!The calculation and emission of the proof window open height block are correctly implemented.
149-150: The changes look good!The wait for the relayer sessions manager to process the proof window open height block is correctly implemented using
waitSimulateIO.
161-162: The changes look good!The wait for the relayer sessions manager to process the trigger proof block is correctly implemented using
waitSimulateIO.pkg/observable/channel/replay.go (8)
17-19: The changes look good!The variable has been correctly renamed to
replayBufferCap, and the comment is consistent with the new name.
32-40: The changes look good!The parameter has been correctly renamed to
replayBufferCap, and the logic is consistent with the intended functionality.
43-55: The changes look good!The parameter has been correctly renamed to
replayBufferCap, and the logic is consistent with the intended functionality.
73-77: The changes look good!The parameter has been correctly renamed to
replayBufferCap, and the warning log is consistent with the new name.
125-125: The changes look good!The
Subscribemethod correctly uses the newSubscribeFromLatestBufferedOffsetmethod with thereplayBufferCapparameter.
Line range hint
134-147: The changes look good!The
SubscribeFromLatestBufferedOffsetmethod is correctly implemented with the new parameter namereplayBufferCap.
179-184: The changes look good!The new
GetReplayBufferSizemethod is correctly implemented and provides a useful utility for retrieving the current size of the replay buffer.
201-204: The changes look good!The buffer management logic in the
initBufferingObservablemethod is consistent with the new parameter namereplayBufferCap.pkg/relayer/session/claim.go (1)
116-125: Handle the case whereclaimsWindowOpenBlockis nil.The added conditional block correctly handles the case where
claimsWindowOpenBlockis nil, which ensures robustness in scenarios where the block is not observed.pkg/relayer/session/proof.go (1)
104-116: Good handling of nilproofsWindowOpenBlock.The added conditional block properly handles the edge case where the block might not be observed. The warning log and sending the session trees to a failure channel are appropriate actions.
Tools
GitHub Check: misspell
[warning] 106-106: [misspell] pkg/relayer/session/proof.go#L106
"whic" is a misspelling of "which"pkg/observable/channel/replay_test.go (4)
66-67: Consistent variable naming.The variable name
replayBufferCapis consistent with the recent changes and improves clarity.
80-80: Correct usage ofreplayBufferCap.The usage of
replayBufferCapinchannel.ToReplayObservableis correct and aligns with the recent changes.
153-154: Consistent initialization of test parameters.The initialization of
replayBufferCapand related test parameters is consistent with the recent changes.
310-311: Correct usage ofreplayBufferCapandendOffsetin test cases.The usage of
replayBufferCapandendOffsetin test cases is correct and aligns with the recent changes.pkg/relayer/session/session.go (5)
314-346: Good handling of replay buffer size inwaitForBlock.The changes ensure that the target block is observed only if the replay buffer contains enough blocks. This prevents issues where the block might not be observed due to insufficient buffer size.
326-332: Informative comment onminNumReplayBlocks.The comment provides clarity on the purpose of
minNumReplayBlocksand the conditions under which the replay buffer is necessary.
334-343: Informative TODO comment on handling replay buffer size.The TODO comment is informative and outlines the steps to be taken once the block query client is implemented.
344-346: Appropriate handling of insufficient replay buffer size.The conditional block appropriately handles the case where the replay buffer size is insufficient to observe the target block.
348-351: Correct loop logic to observe target block.The loop logic is correct and ensures that the function blocks until the target block is observed.
| sharedtypes "github.com/pokt-network/poktroll/x/shared/types" | ||
| ) | ||
|
|
||
| // TODO_TEST: Add a test case which simulates a cold-started relayminer with unclaimed relays. |
There was a problem hiding this comment.
Reminder: Add a test case for cold-started relayminer.
The TODO comment indicates that a test case for simulating a cold-started relayminer with unclaimed relays is missing.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
|
E2E tests passing locally but failed to run in CI:
Prior run failed:
|
…ent-balances * pokt/main: [TODOs] refactor: proof path calculation (#659) [Dependencies] bump go-getter and ibc-go (#691) [Relayminer] refactor: `relayerSessionsManager#waitForBlock()` (#648) [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668) build(deps): bump ws from 7.5.9 to 7.5.10 in /docusaurus (#686) build(deps): bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /docusaurus (#688) build(deps): bump express from 4.18.2 to 4.19.2 in /docusaurus (#687) build(deps): bump follow-redirects from 1.15.3 to 1.15.6 in /docusaurus (#685) build(deps): bump braces from 3.0.2 to 3.0.3 in /docusaurus (#689) [CosmosSDK] Bump to v0.50.7 (#682)
…ation-overserviced * pokt/main: [TODOs] refactor: proof path calculation (#659) [Dependencies] bump go-getter and ibc-go (#691) [Relayminer] refactor: `relayerSessionsManager#waitForBlock()` (#648) [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668) build(deps): bump ws from 7.5.9 to 7.5.10 in /docusaurus (#686) build(deps): bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /docusaurus (#688) build(deps): bump express from 4.18.2 to 4.19.2 in /docusaurus (#687) build(deps): bump follow-redirects from 1.15.3 to 1.15.6 in /docusaurus (#685) build(deps): bump braces from 3.0.2 to 3.0.3 in /docusaurus (#689) [CosmosSDK] Bump to v0.50.7 (#682)
…ation-use-index * pokt/main: [TODOs] refactor: proof path calculation (#659) [Dependencies] bump go-getter and ibc-go (#691) [Relayminer] refactor: `relayerSessionsManager#waitForBlock()` (#648) [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668) build(deps): bump ws from 7.5.9 to 7.5.10 in /docusaurus (#686) build(deps): bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /docusaurus (#688) build(deps): bump express from 4.18.2 to 4.19.2 in /docusaurus (#687) build(deps): bump follow-redirects from 1.15.3 to 1.15.6 in /docusaurus (#685) build(deps): bump braces from 3.0.2 to 3.0.3 in /docusaurus (#689) [CosmosSDK] Bump to v0.50.7 (#682)
Co-authored-by: Redouane Lakrache <r3d0ne@gmail.com>
3 participants
Summary
relayerSessionsManager#waitForBlock()to usereplayObservable#SubscribeFromLatestBufferedOffset()to ensure that the awaited height is contained in block clients replay buffer.TestRelayerSessionsManager_Start()by slowing it down.Issue
ReplayObservable#SubscribeFromBufferEndOffset()method #553Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
Bug Fixes
nilvalues inclaimsWindowOpenBlockandproofsWindowOpenBlockto trigger appropriate warning messages and avoid errors.Refactor
replayBufferSizetoreplayBufferCapfor improved clarity across the codebase.Tests