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.

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

fix: end exclusive data commitment range fix #1058

Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Aug 9, 2023

Description

Fixes celestiaorg/orchestrator-relayer#432

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use
    unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@rach-id rach-id self-assigned this Aug 9, 2023
@rach-id rach-id changed the title fix: Fix data commitment ranges fix: end exclusive data commitment range fix Aug 9, 2023
@@ -335,8 +340,8 @@ func (indexer mockBlockIndexer) Search(ctx context.Context, _ *query.Query) ([]i

// randomBlocks generates a set of random blocks up to the provided height.
func randomBlocks(height int64) []*types.Block {
blocks := make([]*types.Block, height)
for i := int64(0); i < height; i++ {
blocks := make([]*types.Block, height+1)
Copy link
Member Author

@rach-id rach-id Aug 9, 2023

Choose a reason for hiding this comment

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

[note to reviewers]
Here, when we want to generate blocks up to a certain height, then we gonna have height+1 number of blocks because of genesis

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

mind adding a bit more context to this change? for posterity, but also I'm missing some context here 😅 did we miss this before or accidentally change it to an incorrect range? can we do anything to make sure we are getting the correct range to ensure we don't do this in the future?

@rach-id
Copy link
Member Author

rach-id commented Aug 9, 2023

did we miss this before

Yes, we changed the whole implementation to support exclusive end ranges, but we missed this change. And since we didn't run orchestrators with any version of the app that support exclusive end ranges, we didn't see it.

can we do anything to make sure we are getting the correct range to ensure we don't do this in the future?

I added a test: https://github.com/SweeXordious/celestia-core/blob/5075e8d02d300af443aa214a198e88d51ab41f1c/rpc/core/blocks_test.go#L171-L175

rootulp
rootulp previously approved these changes Aug 9, 2023
rpc/core/blocks_test.go Outdated Show resolved Hide resolved
rpc/core/blocks.go Outdated Show resolved Hide resolved
rpc/core/blocks.go Outdated Show resolved Hide resolved
@rach-id rach-id merged commit 819b969 into celestiaorg:v0.34.x-celestia Aug 10, 2023
17 checks passed
@cmwaters
Copy link
Contributor

Can we also get this merged to main @sweexordious ?

@rach-id
Copy link
Member Author

rach-id commented Aug 10, 2023

Definitly, ill open a PR shortly

rach-id added a commit to rach-id/celestia-core that referenced this pull request Aug 10, 2023
## Description

Fixes celestiaorg/orchestrator-relayer#432

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

Co-authored-by: Rootul P <[email protected]>
rach-id added a commit that referenced this pull request Aug 11, 2023
## Description

Cherry pick #1058 to
main

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

## Description

_Please add a description of the changes that this PR introduces and the
files that
are the most critical to review._ 

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

Co-authored-by: Rootul P <[email protected]>
@faddat faddat mentioned this pull request Feb 22, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants