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

docs: document the start/end row in row proof #1473

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Sep 11, 2024

Description

Adds a simple documentation to the start row and end row fields in row proof to show that they're not used for verification. But if anyone is using them downstream, they should validate them themselves

@rach-id rach-id added the C:documentation Improvements or additions to documentation label Sep 11, 2024
@rach-id rach-id self-assigned this Sep 11, 2024
@rach-id rach-id requested a review from a team as a code owner September 11, 2024 07:34
@rach-id rach-id requested review from rootulp and evan-forbes and removed request for a team September 11, 2024 07:34
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.

I'm pretty confused by the comment. Any chance it can be rephrased to clarify what it means?

Comment on lines 21 to 23
// Note: it's currently not used for verification, only for validation.
// If it's used downstream, make sure to validate it.
StartRow uint32 `json:"start_row"`
Copy link
Collaborator

@rootulp rootulp Sep 11, 2024

Choose a reason for hiding this comment

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

[question] can we please clarify "it" here? Is this correct? "it" is used four times but I think they mean different things.

Suggested change
// Note: it's currently not used for verification, only for validation.
// If it's used downstream, make sure to validate it.
StartRow uint32 `json:"start_row"`
// Note: StartRow is currently not used for verification, only for validation.
// Before using a RowProof, users are expected to call Validate().
StartRow uint32 `json:"start_row"`

If not, can we clarify this comment in another way?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Seems better to me.

  1. What does "extra validation depending on how it's used." entail?
  2. Why would this field be unused downstream? It seems like a necessary component of RowProof.

@rach-id
Copy link
Member Author

rach-id commented Sep 11, 2024

Extra validation depends on how those fields are used.
If, for example, they're using that row range to specify where the data is. They will need to check if the start and end index are exactly the ones where the data is.
So, it depends on what downstream repos are proving and how they're constructing their proofs.
Ultimately, we might remove the two fields. But that's a breaking change, and it's not a priority.

When verifying the inclusion of a set of rows to the data root, you don't necessarily need to know where they are. What you care about is that they're committed by the data root. This is done using the slice of proofs and row roots.

@rootulp
Copy link
Collaborator

rootulp commented Sep 11, 2024

If, for example, they're using that row range to specify where the data is.

Does any downstream repo use StartRow or EndRow to indicate anything besides the row range where the data is? It seems like these fields should only be used for that. If they're present they describe the row range.

When verifying the inclusion of a set of rows to the data root, you don't necessarily need to know where they are. What you care about is that they're committed by the data root. This is done using the slice of proofs and row roots.

Got it, thanks.

@rach-id
Copy link
Member Author

rach-id commented Sep 11, 2024

Currently, no. But when building zk-vms that create/verify these proofs, some teams asked about the fields whether they need any specific constraints. That's why we're clarifying that in the comment + letting them know that if they have something specific in mind, they should think about their constraints

@rach-id
Copy link
Member Author

rach-id commented Sep 11, 2024

CI fixed here: #1474

Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Added some comments/questions about some validation that can be relevant to the StartRow and EndRow.

types/row_proof.go Show resolved Hide resolved
@rach-id rach-id enabled auto-merge (squash) September 17, 2024 18:32
@rach-id rach-id merged commit 4392e84 into v0.34.x-celestia Sep 20, 2024
21 checks passed
@rach-id rach-id deleted the document-start-and-end-row branch September 20, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants