Skip to content
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.

Sign up for GitHub

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

chore: Fix offset calculation to correctly skip blocks from startHeight #4267

Closed
wants to merge 1 commit into from
Closed

chore: Fix offset calculation to correctly skip blocks from startHeight #4267

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tools/bytes-in-block/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rootulp can confirm

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


resp, err := http.Get(url)
if err != nil {
Expand Down
Loading