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-19: Shwap #77

Merged
merged 22 commits into from
Mar 20, 2024
Merged

CIP-19: Shwap #77

merged 22 commits into from
Mar 20, 2024

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Feb 12, 2024

Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

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

Missing required section to add:

  • Rationale

Optional sections:

  • Test Cases

cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
@Wondertan Wondertan marked this pull request as ready for review March 4, 2024 17:40
@Wondertan
Copy link
Member Author

Wondertan commented Mar 4, 2024

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.

@Wondertan
Copy link
Member Author

After spending a few hours, I was able to nicely integrate the CIP into specs, so there is no need to do any copying

@jcstein
Copy link
Member

jcstein commented Mar 4, 2024

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

@Wondertan
Copy link
Member Author

Connecting previous discussion from celestiaorg/celestia-node#3205

cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
Copy link
Member

@jcstein jcstein left a 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

Copy link
Collaborator

@ebuchman ebuchman left a 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 Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
```text
RowID {
Height: u64;
RowIndex: u16;
Copy link
Collaborator

@ebuchman ebuchman Mar 6, 2024

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?

Copy link
Member Author

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.

Copy link
Collaborator

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?

Copy link
Member Author

@Wondertan Wondertan Mar 7, 2024

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.

Copy link
Member Author

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?

Copy link
Collaborator

@rootulp rootulp Mar 7, 2024

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

Copy link
Member

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

cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
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.

Re-reviewed the first half.

cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
cips/cip-shwap_protocol.md Outdated Show resolved Hide resolved
@jcstein jcstein changed the title CIP-18: Shwap CIP-19: Shwap Mar 19, 2024
Copy link
Member

@jcstein jcstein left a 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

Copy link
Member

@jcstein jcstein left a 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

@jcstein
Copy link
Member

jcstein commented Mar 20, 2024

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"

cips/cip-19.md:154:1 MD051/link-fragments Link fragments should be valid [Context: "[Serialized](#serialization)"]
cips/cip-19.md:198:1 MD051/link-fragments Link fragments should be valid [Context: "[Serialized](#serialization)"]
cips/cip-19.md:256:1 MD051/link-fragments Link fragments should be valid [Context: "[Serialized](#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?

@jcstein jcstein merged commit 1ee93f7 into celestiaorg:main Mar 20, 2024
2 checks passed
@Wondertan Wondertan deleted the cip-shwap_protocol branch March 20, 2024 15:45
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.

6 participants