-
Notifications
You must be signed in to change notification settings - Fork 44
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-19: Shwap #77
CIP-19: Shwap #77
Conversation
Wondertan
commented
Feb 12, 2024
•
edited
Loading
edited
- Rendered
- Discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to merge the whole spec into CIP instead of maintaining a separate doc. Having it externally seems cumbersome and prone to breaks. We would have to cross-link various required sections from CIP to the actual step. Instead, I will link celestia-node specs to CIP. In the worst case, we will copy the documents if that does not work for some reason. |
9a2ea7a
to
c5424dd
Compare
After spending a few hours, I was able to nicely integrate the CIP into specs, so there is no need to do any copying |
sharing this on github as well, I think it would be good if we align this CIP and the discussion from rsmt2d . based on what i can tell, it already is |
Connecting previous discussion from celestiaorg/celestia-node#3205 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments and committable suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a first pass to clarify some things
cips/cip-shwap_protocol.md
Outdated
```text | ||
RowID { | ||
Height: u64; | ||
RowIndex: u16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this 16 bit limit enforced elsewhere in the core protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I mean, is there a limit on how many rows there can be enforced somewhere else in the protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah not that I would be aware of. The u16 is chosen because it maxes at 2TB squares with 512shares, which is more than enough. Might be worth clarifying that as well in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @rootulp, do we have any row number limits defined in core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea! The max row number is equivalent to 2 * the max effective square size because the row number seems applicable to the extended data square. The max effective square size is currently 64 but it could be bumped to 128 via a governance proposal.
Related table comparing original data square size and extended data square size: https://gist.github.com/rootulp/bbf10f6e9cf114816aaa994eb64b63a4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving this discussion open to migrate into the forum
ac6e206
to
2d74d8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed the first half.
…re rules to the spec
Co-authored-by: Josh Stein <[email protected]>
Co-authored-by: Rootul P <[email protected]>
bc34ac3
to
0324e1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved, but before merging:
- 2 remaining comments should first be resolved
- latest changes need to be pulled in, and CIP-19 added to readme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be linted
the only remaining step is to link these 3 lines to the correct serialization heading, as there are now 2 different headings. before, there were 2 headings with the same content: "serialization"
additional context, we cannot have a link that goes back to 2 headings. it just will not work. how would a link direct you to a heading, if there are 2 identical headings? |