da: add submit, get context timeouts #1507
Conversation
WalkthroughThe recent updates to the Changes
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- da/da.go (4 hunks)
Additional comments: 3
da/da.go (3)
- 8-8: The import of the
timepackage is correctly added to support the new timeout functionality.- 123-125: The introduction of a timeout context in
SubmitBlocksusingsubmitTimeoutis correctly implemented. However, ensure that the cancellation function,cancel, is always called, even if theSubmitmethod does not return an error, to avoid potential leaks of context resources.- 175-177: Similar to
SubmitBlocks, theRetrieveBlocksmethod correctly applies a timeout context usingretrieveTimeout. The use ofcancelafter retrieving the blocks ensures that resources are properly released.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1507 +/- ##
==========================================
+ Coverage 50.23% 50.31% +0.08%
==========================================
Files 52 52
Lines 6762 6735 -27
==========================================
- Hits 3397 3389 -8
+ Misses 3024 3004 -20
- Partials 341 342 +1 ☔ View full report in Codecov by Sentry. |
tuxcanfly
added
T:bug
Manav-Aggarwal
force-pushed
the
tux/da-timeout
branch
from
61bb3c0
to
5f1c3ed
Compare
tuxcanfly
force-pushed
the
tux/da-timeout
branch
from
5f1c3ed
to
fc38b78
Compare
Manav-Aggarwal
force-pushed
the
tux/da-timeout
branch
from
16eee79
to
3603be5
Compare
|
@tuxcanfly is there a way to test this? i don't think it is addressing any problem. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- da/da.go (4 hunks)
- da/da_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- da/da.go
Additional comments: 5
da/da_test.go (5)
- 4-4: The addition of the
bytespackage is appropriate given the use ofbytes.Repeatin the testTestMockDAErrorsunder thesubmit_timeoutscenario. This ensures the necessary functionality is available for manipulating byte slices in tests.- 71-76: Modification to the
Submitmethod inMockDAto handle context cancellation is a crucial improvement. It allows the method to respect context timeouts or cancellations, aligning with the PR's objective to introduce context with timeouts for submission operations. This change ensures that the DA client can properly handle scenarios where a submission might take longer than expected, thereby preventing indefinite blocking.- 84-102: The refactoring of the test function
TestMockDAErrorsto include asubmit_timeoutscenario is well-implemented. It effectively tests the new context cancellation handling in theSubmitmethod by simulating a timeout. This is achieved by setting up the mock to return after a delay (After(100*time.Millisecond)) and then callingdoTestSubmitTimeoutwith a context that times out sooner (time.Second). This test ensures that theSubmitmethod behaves as expected under timeout conditions, which is critical for the reliability of the DA client.- 104-107: The addition of a
max_blob_size_errortest scenario withinTestMockDAErrorsis a good practice. It tests the DA client's behavior when an error occurs while fetching the maximum blob size, which is a potential failure point. By mocking an error response forMaxBlobSizeand then verifying the error handling indoTestMaxBlockSizeError, this test ensures that the DA client can gracefully handle errors related to maximum blob size constraints.- 161-169: The implementation of
doTestSubmitTimeoutfunction correctly tests the timeout behavior introduced in theSubmitmethod. By setting a shortersubmitTimeoutthan the mock's delay and asserting that the response message contains "context deadline exceeded", this test validates that the DA client correctly handles submission timeouts. This is a critical test for ensuring that the DA client's submission process is robust and can handle network or processing delays gracefully.
|
@gupadhyaya added test case for context timeout. The submit call relies on a sync endpoint in core and can stall the block submission loop if we don't use a context deadline, see: https://gist.github.com/tuxcanfly/207ab3dc1fac4bb9c38bedad2a053f18 for an example, but there can be other reasons the core endpoint can timeout e.g. during inscriptions flood or improperly configured reverse proxy. There's also a related issue here which is that if a transaction that is already in the mempool, then it should not be retried with backoff immediately, because it will error with |
tuxcanfly
force-pushed
the
tux/da-timeout
branch
from
6ff199c
to
42ffbb5
Compare
Overview
This PR adds context with sane default timeouts to submit and get blob methods on the DA client. Fixes #1036
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests