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

chore!: make the row proof range end exclusive #1376

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Jun 5, 2024

Description

Closes #1375

It would be nice if we could cram this change also in v2. If not, then once this change makes it to a release, we need to remember to update the app implementation and any downstream repo to use end exclusive ranges for row proofs. Would creating an issue work?


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 Jun 5, 2024
@rach-id rach-id requested a review from a team as a code owner June 5, 2024 08:04
@rach-id rach-id requested review from staheri14 and evan-forbes and removed request for a team June 5, 2024 08:04
@cmwaters
Copy link
Contributor

cmwaters commented Jun 5, 2024

This is a breaking change right? I'm not sure if we've agreed on branch management / releases for celestia-core yet

@rach-id
Copy link
Member Author

rach-id commented Jun 5, 2024

yes, it's a breaking change

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

IMO we need a strategy on how to release breaking changes of celestia-core before we merge this

proto/tendermint/types/types.proto Outdated Show resolved Hide resolved
types/row_proof.go Outdated Show resolved Hide resolved
types/row_proof.go Outdated Show resolved Hide resolved
rach-id and others added 3 commits June 5, 2024 17:33
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
@rach-id
Copy link
Member Author

rach-id commented Jun 5, 2024

[thinking out loud]
Why not have a v0.34.x-v2 branch that contains the breaking changes. This branch should only contain the changes that are on main and we want to include in v2.
The existing branch v0.34.x could be renamed to v0.34.x-v1 to reflect that this is the verison used for V1.

@cmwaters
Copy link
Contributor

cmwaters commented Jun 6, 2024

[thinking out loud] Why not have a v0.34.x-v2 branch that contains the breaking changes. This branch should only contain the changes that are on main and we want to include in v2. The existing branch v0.34.x could be renamed to v0.34.x-v1 to reflect that this is the verison used for V1.

The problem is a little more complex then that given our current upgrading strategy. We could be running v2 binary on a v1 network, thus celestia-core also needs to be wary of the app version

@rootulp
Copy link
Collaborator

rootulp commented Jun 6, 2024

Why not have a v0.34.x-v2 branch that contains the breaking changes.

celestia-app v2 won't use any celestia-core breaking changes that aren't present in celestia-core v1.36.1-tm-v0.34.29 because celestia-app v2.0.0-rc1 is being audited and it depends on v1.36.1-tm-v0.34.29.

@rach-id rach-id enabled auto-merge (squash) July 15, 2024 13:27
@evan-forbes
Copy link
Member

are these proofs moved elsewhere for node @rach-id ? if so, can we close this and remove them here?

@rach-id
Copy link
Member Author

rach-id commented Jul 29, 2024

Once we release the proofs in node. I will start deprecating the endpoints. But for the proofs implementation, it will remain in core because it's used in tx proof. So we still need this PR to be part of v3 if possible.

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.

Consistent row proof range
5 participants