chore: increase the data commitment blocks limit in the API

[rach-id]

Description

This is done to support 4 hours batches of attestations in the API without having to make a breaking change

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

[staheri14]

I don't have sufficient context to form a strong opinion about this change. @rach-id, given your greater familiarity with blobstream module, I trust your judgment and approve it.

[rootulp]

This seems like a breaking change because DataCommitmentBlocksLimit is exported by celestia-core and used by celestia-app here. If this is a breaking change, it needs a ! in the PR title.

[rootulp]

How was 2048 derived? I would expect 1200 instead of 2048 because:

4 (hours) * 60 (minutes) * 60 (seconds) = 14,400 seconds in 4 hours
14,400 / 12 (slot time on Ethereum) = 1,200
14,400 / 12 (block time on Celestia) = 1,200

[rach-id]

To have a small extra wiggle room. I am currently discussing bumping it to 10000, but we're still investigating the impacts with Succinct

[rootulp]

2048 * 12 = 24,576 / 60 / 60 = 6.8 hours so almost 7 hours. Given this isn't close to 4 hours, can the PR description be updated?

Also can the rationale for the value chosen be documented in a code comment next to the line if it's not immediately obvious.

[rach-id]

This seems like a breaking change because DataCommitmentBlocksLimit is exported by celestia-core and used by celestia-app here. If this is a breaking change, it needs a ! in the PR title.

Increasing this value is not breaking. But decreasing it requires version management etc. That's why I didn't add the ! as the change is backwards compatible

[rootulp]

Can you please expand on why this change is non-breaking? It's an exported constant from celestia-core so it still seems breaking to me.

[rach-id]

Can you please expand on why this change is non-breaking? It's an exported constant from celestia-core so it still seems breaking to me.

The way I see it is that if previously a data commitment window was able to be setup without error, then using the new value that would also be the case.
It's like when you have an API that returns, for example:

type Response struct {
    Resp1 string
}

And add a new field:

type Response struct {
    Resp1 string
    Extra string
}

Even if the API changed and the response is exported, the downstream existing implementations and code doesn't necessarily need to be updated to match the new API. Similar to this batch size increase, if anyone is using that constant, they don't need to update their logic for their existing logic to work with the new release.
Furthermore, this was only enforced in the genesis, so if a first block was generated based on a genesis that uses a window < 1000, it will still pass with < 2048 or whichever value that is higher.

This is my reasoning, let me know if I missed something so that I add the ! :D

making sure a change is breaking or not can be troublesome sometimes :D

[rootulp]

Furthermore, this was only enforced in the genesis

I disagree. This constant will be used to validate governance param changes because DataCommitmentWindow is considered governance modifiable (see specs). See this branch and this test output:

inside validate data commitment window
...
key: DataCommitmentWindow, value: "10000", err: invalid parameter value: data commitment window 10000 must be <= data commitment blocks limit 2048: invalid data commitment window: failed to set parameter

since the test fails, we can't adopt this change in a minor release of celestia-app because then we could have some consensus nodes consider a param change to DataCommitmentBlocksLimit = 2000 as valid and others would consider it invalid. This definitely seems consensus breaking to me so IMO we should mark this change as breaking. We should also document that we can't backport this to celestia-app v1.x and it must be introduced in v2.x.

Even if it's only enforced at genesis, I think it's breaking. Assume we mark this as non-breaking here and cut the next celestia-core release with it. Then assume we bump the celestia-core dep in celestia-app. The next celestia-app release (v1.8.0) would be incompatible with the current celestia-app release (v1.7.0) because the two versions would produce different results when validating a genesis file with DataCommitmentBlocksLimit = 2000. v1.7.0 would consider the genesis file invalid. v.1.8.0 would consider it valid.

[tangent] This constant seems used here so if it is changed, then I think it violates this comment at the top of the file. If we modify this constant, we should either: update that comment or move the constant out of that file.

[rootulp]

IMO this is still breaking. See the last comment.

[rach-id]

@rootulp Good catch 👍 switched to using a local limit in the API.

[rootulp]

On second thought, I'm not going to block on the comment for explaining 10_000 but I think it would be a nice addition.

[staheri14]

LGTM!