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: ADR 14 Versioned Namespaces #1382

Merged
merged 15 commits into from
Mar 1, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Feb 14, 2023

ADR for #1282

@rootulp rootulp added the ADR item is directly relevant to writing or modifying an ADR label Feb 14, 2023
@rootulp rootulp self-assigned this Feb 14, 2023
@MSevey MSevey requested a review from a team February 14, 2023 19:04
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

awesome idea essentially pre-writing the ADR before the decision! I'm glad we're recording all of this!

had one or two questions that we might want to discuss quickly, but I'm tempted to approve already

docs/architecture/adr-014-versioned-namespaces.md Outdated Show resolved Hide resolved
docs/architecture/adr-014-versioned-namespaces.md Outdated Show resolved Hide resolved
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

only remaining thing imo, otherwise LGTM and I think we should merge

ERROR: 2 dead links found!
  [✖] https://proto.school/anatomy-of-a-cid → Status: 0
  [✖] https://cid.ipfs.tech/#QmcRD4wkPPi6dig81r5sLj9Zm1gDCL4zgpEj9CfuRrGbzF → Status: 0

edit: shoot nvm these weren't working for me the other day, but they are now, idk what is wrong with the link checker.

@MSevey MSevey requested a review from a team February 20, 2023 18:17
evan-forbes
evan-forbes previously approved these changes Feb 20, 2023
@MSevey MSevey requested a review from a team February 20, 2023 21:15
evan-forbes
evan-forbes previously approved these changes Feb 21, 2023
evan-forbes
evan-forbes previously approved these changes Feb 22, 2023
@MSevey MSevey requested a review from a team March 1, 2023 00:01
@rootulp rootulp merged commit 6d27b78 into celestiaorg:main Mar 1, 2023
@rootulp rootulp deleted the rp/adr-versioned-namespaces branch March 1, 2023 18:42
Comment on lines +36 to +37
1. Changes to the non-interactive default rules that don't break backwards compatibility with existing namespaces [celestia-app#1282](https://github.com/celestiaorg/celestia-app/issues/1282), [celestia-app#1161](https://github.com/celestiaorg/celestia-app/pull/1161)
- After mainnet launch, if we want to change the non-interactive default rules but retain the previous non-interactive default rules for backwards compatibility, it isn't possible to differentiate the namespaces that want to use the old rules vs the new rules.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to regurgitate this point for my own understanding: We want the rules that define the construction of a square (i.e. potentially the non-interactive default rules or any other algorithm) to be on a per namespace basis. Not per square (i.e. a version number that reflects the construction algorithm for the entire square - implying homogeneity for all txs in that square) and not per share (i.e. multiple shares for the same namespace could theoretically have different versions for interpreting where they should be placed in the square - this assumes that all messages are always tied to a namespace and are always ordered (ascending)).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I believe so.

this assumes that all messages are always tied to a namespace and are always ordered

messages => blobs ? If so, I agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well messages could also be an IBC transaction or a governance vote (they still have namespaces)


1. Changes to the non-interactive default rules that don't break backwards compatibility with existing namespaces [celestia-app#1282](https://github.com/celestiaorg/celestia-app/issues/1282), [celestia-app#1161](https://github.com/celestiaorg/celestia-app/pull/1161)
- After mainnet launch, if we want to change the non-interactive default rules but retain the previous non-interactive default rules for backwards compatibility, it isn't possible to differentiate the namespaces that want to use the old rules vs the new rules.
1. Changes to the format of a padding share.
Copy link
Contributor

Choose a reason for hiding this comment

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

Because here we would not be allowed to change the namespace ID?

Why couldn't we use the version number of the share to reflect a change in the format. That would seem most intuitive for me

Copy link
Collaborator Author

@rootulp rootulp Mar 9, 2023

Choose a reason for hiding this comment

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

Why couldn't we use the version number of the share to reflect a change in the format. That would seem most intuitive for me

I think we could also use the version number


An approach that addresses these issues is to prefix the namespace ID with version metadata.

### Option A: `Namespace Version | Namespace ID`
Copy link
Contributor

Choose a reason for hiding this comment

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

How are blobs ordered now? Do they include the namespace version or just the ID (and if it is including the version is it at the start or end)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How are blobs ordered now?

In the current implementation? Shares are ordered by namespace ID. There is no namespace version in the current implementation. There is a share version in the current implementation and that is after the namespace ID.

Ref:

- Option B is implementable at a later time if we adopt Option A.
- It isn't immediately clear how NMT can support variable length namespaces.

### Option C: `Share Version | Namespace ID`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still for this option. (I'm aware we've already decided. I just haven't yet been convinced that there is a specific need for namespace versioning that isn't covered by share versioning and square versioning).

I'm also a big fan for putting the version number in a fixed indexed position (like the first or last byte) as opposed to the 9th byte or 17th byte

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm also a big fan for putting the version number in a fixed indexed position (like the first or last byte) as opposed to the 9th byte or 17th byte

FWIW the accepted proposal has a namespace version at index 0 and a share version at index 33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADR item is directly relevant to writing or modifying an ADR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants