chore!: make the row proof range end exclusive

celestiaorg / celestia-core

A fork of CometBFT

Apache License 2.0

481 stars 264 forks source link

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

Open rach-id opened 3 months ago

rach-id commented 3 months ago

Description

Closes https://github.com/celestiaorg/celestia-core/issues/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

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

cmwaters commented 3 months ago

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

rach-id commented 3 months ago

yes, it's a breaking change

rach-id commented 3 months ago

[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 commented 3 months ago

[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 commented 3 months ago

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.

evan-forbes commented 2 months ago

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

rach-id commented 2 months ago

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.