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

CIP-22: Remove blobStartIndex from PFBs #160

Merged
merged 4 commits into from
Jul 3, 2024
Merged

Conversation

nashqueue
Copy link
Member

Overview

CIP about removing the blobstartIndex from the Wrapped Transaction

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.

Awesome work! Really well written. I'm in favor of removing the blobStartIndex (a.k.a share_indexes) if possible.


celestia-node does not consume this index. Although they have their own way of creating the same information, this proposal is not applicable to them.

None of the rollup teams are influenced by this change except Sovereign SDK. The circuit parsing the PFB reserved namespace would break and must be adapted. The circuit does not use the information from the blobStartIndex. If this change is accepted, the live rollup teams will have to upgrade their circuits when Celestia upgrades to the new version. Currently, no rollups are live on mainnet using Celestia, so a breaking change would not affect anyone directly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

no rollups are live on mainnet using Celestia

Is this statement only applicable to Sovereign SDK roll-ups?

Suggested change
None of the rollup teams are influenced by this change except Sovereign SDK. The circuit parsing the PFB reserved namespace would break and must be adapted. The circuit does not use the information from the blobStartIndex. If this change is accepted, the live rollup teams will have to upgrade their circuits when Celestia upgrades to the new version. Currently, no rollups are live on mainnet using Celestia, so a breaking change would not affect anyone directly.
None of the rollup teams are influenced by this change except Sovereign SDK. The circuit parsing the PFB reserved namespace would break and must be adapted. The circuit does not use the information from the blobStartIndex. If this change is accepted, the live rollup teams will have to upgrade their circuits when Celestia upgrades to the new version. Currently, no Sovereign SDK rollups are live on mainnet using Celestia, so a breaking change would not affect anyone directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes So far I only have seen them proving thinks through the PFB messages. This should also be not a concern when they move to Authored Blobs


## Security Considerations

No Celestia light nodes rely on the blobStartIndex to verify the square's correctness. No fraud proofs rely on the blobStartIndex, so removing it does not affect the network's security. Without the blobStartIndex, we won't be able to create compact fraud proofs anymore. This means that accepting this proposal is also a commitment to ZK prove the validity of the square layout.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


## Specification

Remove the blobStartIndex from the ['WrappedTransaction'](https://celestiaorg.github.io/celestia-app/specs/data_structures.html#wrappedtransaction).
Copy link
Collaborator

Choose a reason for hiding this comment

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

---
title: Removing the blobStartIndex
author: NashQueue @Nashqueue
discussions-to: URL
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM! I think this can be merged after you create a forum post and link here

@jcstein
Copy link
Member

jcstein commented Jun 27, 2024

is my understanding correct that to point to the actual blob you intend to in the square you have to use index + height + commitment? without index, there could be 2 blobs with identical commitments submitted by 2 different accounts where 1 is malicious and 2 is honest?


## Security Considerations

No Celestia light nodes rely on the blobStartIndex to verify the square's correctness. No fraud proofs rely on the blobStartIndex, so removing it does not affect the network's security. Without the blobStartIndex, we won't be able to create compact fraud proofs anymore. This means that accepting this proposal is also a commitment to ZK prove the validity of the square layout.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this can be moved to the forum discussion but has there been any analysis done on the feasibility of proving that a blob has been paid for

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you explain what you mean

@YazzyYaz YazzyYaz changed the title Remove blobStartIndex from PFBs CIP-21: Remove blobStartIndex from PFBs Jul 2, 2024
@YazzyYaz YazzyYaz changed the title CIP-21: Remove blobStartIndex from PFBs CIP-22: Remove blobStartIndex from PFBs Jul 2, 2024
cips/cip-22.md Outdated Show resolved Hide resolved
@nashqueue nashqueue requested a review from jcstein July 3, 2024 14:54
@jcstein jcstein merged commit db8fdb2 into celestiaorg:main Jul 3, 2024
2 checks passed
@jcstein
Copy link
Member

jcstein commented Jul 3, 2024

I think this needs a description based on CIP-1: https://github.com/celestiaorg/CIPs/blob/main/cips/cip-1.md#cip-header-preamble

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants