chore: Fix offset calculation to correctly skip blocks from startHeight #4267
Conversation
fragchain
requested review from
cmwaters and
ninabarbakadze
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request involves a modification to the Changes
Sequence DiagramsequenceDiagram
participant Client
participant API
Client->>API: GET Request with Adjusted Offset
Note over Client, API: offset = originalOffset - startHeight
API-->>Client: Response
The sequence diagram illustrates how the client now sends a GET request to the API with an offset that is relative to the starting height, ensuring more accurate block range specification. ✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tools/bytes-in-block/main.go (3)
41-41: LGTM! The offset calculation fix looks correct.The change to subtract
startHeightfromoffsetensures that the API correctly skips the right number of blocks when paginating from the specified start height.However, consider making the API endpoint configurable:
+const defaultAPIEndpoint = "https://api-mocha.celenium.io/v1" + func main() { + apiEndpoint := os.Getenv("CELESTIA_API_ENDPOINT") + if apiEndpoint == "" { + apiEndpoint = defaultAPIEndpoint + } ... - url := fmt.Sprintf("https://api-mocha.celenium.io/v1/block?limit=%d&offset=%d&sort=asc&stats=true", numberOfBlocksPerRequest, offset-startHeight) + url := fmt.Sprintf("%s/block?limit=%d&offset=%d&sort=asc&stats=true", apiEndpoint, numberOfBlocksPerRequest, offset-startHeight)
Line range hint
27-36: Consider validating input parameters.The start and end heights are hardcoded. Consider adding validation to ensure:
- startHeight is positive
- endHeight is greater than startHeight
- numberOfBlocksPerRequest is within reasonable limits
func main() { startHeight := 4170798 endHeight := 4170898 numberOfBlocksPerRequest := 100 + if startHeight < 0 { + log.Fatal("startHeight must be positive") + } + if endHeight <= startHeight { + log.Fatal("endHeight must be greater than startHeight") + } + if numberOfBlocksPerRequest <= 0 || numberOfBlocksPerRequest > 1000 { + log.Fatal("numberOfBlocksPerRequest must be between 1 and 1000") + }
Line range hint
43-49: Enhance error handling for HTTP responses.The current error handling only checks if the request failed, but not the HTTP status code. Consider adding status code validation:
resp, err := http.Get(url) if err != nil { log.Fatalf("Failed to GET %s: %v", url, err) } + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + log.Fatalf("API returned non-200 status code %d: %s", resp.StatusCode, body) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/bytes-in-block/main.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
| @@ -38,7 +38,7 @@ func main() { | |||
| // you don't exceed the last block. But the endpoint might just return fewer | |||
| // blocks if you overshoot. In practice, `offset` is just how many items to skip. | |||
| // We'll keep it simple: | |||
| url := fmt.Sprintf("https://api-mocha.celenium.io/v1/block?limit=%d&offset=%d&sort=asc&stats=true", numberOfBlocksPerRequest, offset) | |||
| url := fmt.Sprintf("https://api-mocha.celenium.io/v1/block?limit=%d&offset=%d&sort=asc&stats=true", numberOfBlocksPerRequest, offset-startHeight) | |||
There was a problem hiding this comment.
I don't think this is how it's supposed to work. Apparently in this API, this request queries for the blocks between [offset, offset + numberOfBlocksPerRequest]. In this case offset starts at startHeight and keeps increasing by numberOfBlocksPerRequest on each iteration until it reaches the end.
So no change to this call is required.
There was a problem hiding this comment.
rach-id thank you for your feedback! I see your point, but I’d like to clarify why I believe this change is necessary:
The current implementation assumes that offset always starts at startHeight and increments by numberOfBlocksPerRequest. However, if offset isn't explicitly reset to account for startHeight, it can lead to incorrect behavior in scenarios where startHeight is modified dynamically or when the API is used in a context where offset doesn’t align perfectly with startHeight.
My change ensures that the calculation explicitly accounts for startHeight, guaranteeing that the query starts from the intended block range every time. Without this adjustment, there’s a risk of missing or duplicating blocks in edge cases, especially when the API is reused with varying startHeight values.
I’d love to hear your thoughts on this and can provide additional examples if needed.
There was a problem hiding this comment.
scenarios where startHeight is modified dynamically
I don't see when or how this can happen. The script works as-is. We want to include blocks starting at start height and querying 100 blocks each time.
Overview
The
offsetparameter wasn't properly accounting for thestartHeightwhen skipping blocks.This change replaces
offsetwithoffset-startHeightto ensure the correct number of blocks are skipped starting fromstartHeight.This should resolve any inconsistencies in block processing.