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

doc: revises the namespace specifications and includes some clarifications #2124

Merged
merged 22 commits into from
Jul 19, 2023

Conversation

staheri14
Copy link
Contributor

@staheri14 staheri14 commented Jul 18, 2023

Overview

I have made clarifications and revisions to the namespace specifications in the PR as part of my effort to audit the namespace module. Additionally, I have included inline questions primarily intended for the spec owner. Once these questions are resolved, I will promptly take the necessary actions accordingly.
This PR also adheres to semantic line breaks and also makes wording updates according to RFC 2119.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@github-actions
Copy link

github-actions bot commented Jul 18, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://celestiaorg.github.io/celestia-app/pr-preview/pr-2124/
on branch gh-pages at 2023-07-19 17:49 UTC

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2023

Codecov Report

Merging #2124 (05c587d) into main (dec9253) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2124   +/-   ##
=======================================
  Coverage   25.05%   25.05%           
=======================================
  Files         121      121           
  Lines       13811    13811           
=======================================
  Hits         3460     3460           
  Misses       9995     9995           
  Partials      356      356           

@staheri14 staheri14 changed the title doc: revises the namespace specifications (inline with the internal audit) doc: revises the namespace specifications Jul 18, 2023
@staheri14 staheri14 changed the title doc: revises the namespace specifications doc: revises the namespace specifications and includes some clarifications Jul 18, 2023
@staheri14 staheri14 added this to the Mainnet milestone Jul 18, 2023
@staheri14 staheri14 marked this pull request as ready for review July 18, 2023 22:29
@staheri14 staheri14 requested review from rootulp and removed request for rootulp July 18, 2023 22:29
@MSevey MSevey requested a review from a team July 18, 2023 22:37
Comment on lines +90 to +92
## Protobuf Definition

<!-- TODO: Add protobuf definition for namespace -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Does namespace actually have protobuf definitions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure either, it is a placeholder just in case we have, or we want to have a protobuf definition. Please see #2128.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't see the use. I think it's adequate to have it represented as an array of bytes


## Reserved Namespaces

Celestia reserves certain namespaces with specific meanings.
Applications SHOULD refrain from using these reserved namespaces for their blob data. <!-- the implication of this is still under investigation, but it is prudent to advise app developers not using this namespace -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it straight up infeasible to use these reserved namespaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sentence aims to provide clarity to app developers regarding the consequences of overlooking reserved namespaces and using them for their blobs.

Isn't it straight up infeasible to use these reserved namespaces

Can you please expand on this part? Why it is infeasible? Is it part of the transaction validity rules? where this check is done? What happens if a blob's namespace has a reserved namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, found the answer in the code, changed SHOULD to MUST in f171508.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question @rootulp: The expected behavior is to reject blob transactions with reserved namespaces? right? asking this for the code audit's sake

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it would be rejected in ProcessProposal

Copy link
Collaborator

Choose a reason for hiding this comment

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

The expected behavior is to reject blob transactions with reserved namespaces?

Yes.

Copy link
Contributor Author

@staheri14 staheri14 Jul 19, 2023

Choose a reason for hiding this comment

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

Yes, it would be rejected in ProcessProposal

@cmwaters If it is checked during ProcessProposal, then how would the tx sender be informed that its transaction was invalid? or they are not informed?

specs/src/specs/namespace.md Show resolved Hide resolved
specs/src/specs/fraud_proofs.md Outdated Show resolved Hide resolved
specs/src/specs/namespace.md 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.

A few questions and one blocking comment regarding usage of "SHOULD"

Comment on lines +24 to +25
The version is used to determine the format of the namespace and
is encoded as a single byte.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] semantic line breaks are new for me so I don't understand why a line break was introduced in the middle of this sentence.

Was it added to conform to maximum line length character requirements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line break is added here because the second part, "is encoded as a single byte," conveys a separate and independent message from the first part. By moving it to the next line, we ensure clarity and emphasize the distinction between the two ideas.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for clarifying! TBH it's not immediately obvious to me when to introduce a semantic line break but I think it's safe to keep one here :)

I guess we could also rewrite this as two sentences:

The version is used to determine the format of the namespace.
The version is encoded as a single byte.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH it's not immediately obvious to me when to introduce a semantic line break but I think it's safe to keep one here :)

As a general guideline, it is usually recommended to add a line break after each ending period. However, there are other instances where we may want to break a line in the middle. In such cases, a helpful mental model is to view it as commit messages. Similar to keeping each commit short and self-contained, we can break down different semantically independent messages of a line into separate lines, using line breaks.

I guess we could also rewrite this as two sentences:

Yes, Your suggested approach also works.

specs/src/specs/namespace.md Outdated Show resolved Hide resolved
specs/src/specs/namespace.md Show resolved Hide resolved
The namespace ID is a 28 byte identifier that uniquely identifies a namespace. The ID is encoded as a byte slice of length 28.
The namespace ID is a 28 byte identifier that uniquely identifies a namespace.
The ID is encoded as a byte slice of length 28.
<!-- It may be useful to indicate the endianness of the encoding) -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] is endianess applicable for a byte slice that doesn't inherently represent some value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that doesn't inherently represent some value?

Can you provide further clarification on this specific section?
The interpretation of the namespace impacts in various contexts, particularly in relation to ordering in NMTs. Consequently, the encoding and decoding of the namespace play a crucial role when transmitting data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK https://en.wikipedia.org/wiki/Endianness is important to clarify for types like a int64 but I don't understand how it applies to this []byte

Screenshot 2023-07-19 at 3 05 51 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can use the encoding/binary packages in the Go std to read or write multi-byte values from byte slices either in big endian or little endian format.

Exactly, as soon as we want to read and use that byte slice, then the endianness becomes important. Imagine a namespace query with a namespace encoded in little-endian order (in its Protobuf format), while the receiver expects namespaces in big-endian order, so the query won't be resolved correctly, as there won't be any match in the tree for that namespace due to the mismatch endianness.

specs/src/specs/namespace.md Outdated Show resolved Hide resolved
specs/src/specs/namespace.md Outdated Show resolved Hide resolved
specs/src/specs/namespace.md Outdated Show resolved Hide resolved
specs/src/specs/namespace.md Outdated Show resolved Hide resolved
@MSevey MSevey requested a review from a team July 19, 2023 17:03
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Thanks for improving the namespace specs! :shipit:

@staheri14 staheri14 merged commit b77ac63 into main Jul 19, 2023
21 checks passed
@staheri14 staheri14 deleted the sanaz/namespace-specs-audit-update branch July 19, 2023 20:24
@evan-forbes evan-forbes removed this from the Mainnet milestone Aug 24, 2023
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