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

staheri14 · 2023-07-18T17:55:49Z

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 · 2023-07-18T17:56:09Z

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 · 2023-07-18T18:16:06Z

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

…te' into sanaz/namespace-specs-audit-update

specs/src/specs/namespace.md

staheri14 · 2023-07-18T22:23:11Z

Adding some issues relevant to this specifications

cmwaters · 2023-07-19T12:58:33Z

specs/src/specs/namespace.md

+## Protobuf Definition
+
+<!-- TODO: Add protobuf definition for namespace -->


Does namespace actually have protobuf definitions?

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

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

cmwaters · 2023-07-19T12:59:07Z

specs/src/specs/namespace.md


 ## 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 -->


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

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?

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

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

Yes, it would be rejected in ProcessProposal

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

Yes.

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

specs/src/specs/fraud_proofs.md

specs/src/specs/namespace.md

rootulp

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

rootulp · 2023-07-19T13:50:34Z

specs/src/specs/namespace.md

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


[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?

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.

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.

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

rootulp · 2023-07-19T13:58:46Z

specs/src/specs/namespace.md

-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) -->


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

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.

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

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

cmwaters

LGTM

rootulp

Thanks for improving the namespace specs!

staheri14 added 5 commits July 17, 2023 15:44

expands on the reserved namespaces section

e445aac

moves version 0 under its own subsection

8de047a

reorganizes the version subsection and expands on the version 0

29888be

explains the usage of the version value 255

421c441

conforms to semantic line breaks

8549b30

adds one item under considerations

39afd72

staheri14 and others added 3 commits July 18, 2023 11:18

Merge branch 'main' into sanaz/namespace-specs-audit-update

b2e343a

adds a link to the reserved namespaces

1f46ff5

Merge remote-tracking branch 'origin/sanaz/namespace-specs-audit-upda…

405a603

…te' into sanaz/namespace-specs-audit-update

rootulp assigned staheri14 Jul 18, 2023

staheri14 changed the title ~~doc: revises the namespace specifications (inline with the internal audit)~~ doc: revises the namespace specifications Jul 18, 2023

staheri14 added 4 commits July 18, 2023 12:45

revises the reserved namespaces section

96e2140

replaces MUST with SHOULD

ac657d1

replaces block with the proper term of data square

713a160

removes excess line

27dd594

staheri14 changed the title ~~doc: revises the namespace specifications~~ doc: revises the namespace specifications and includes some clarifications Jul 18, 2023

staheri14 added the namespace audit label Jul 18, 2023

staheri14 commented Jul 18, 2023

View reviewed changes

specs/src/specs/namespace.md Outdated Show resolved Hide resolved

staheri14 commented Jul 18, 2023

View reviewed changes

specs/src/specs/namespace.md Outdated Show resolved Hide resolved

staheri14 commented Jul 18, 2023

View reviewed changes

specs/src/specs/namespace.md Outdated Show resolved Hide resolved

staheri14 commented Jul 18, 2023

View reviewed changes

specs/src/specs/namespace.md Outdated Show resolved Hide resolved

staheri14 added 4 commits July 18, 2023 13:13

fixes linter issues

f25dee0

removes a comment

912b941

adds todo for protobuf

c49fd2f

fixes a dead link

c901967

staheri14 added this to the Mainnet milestone Jul 18, 2023

a minor rewording

05c587d

staheri14 mentioned this pull request Jul 18, 2023

chore: following semantic line breaks in documentations #2123

Closed

staheri14 marked this pull request as ready for review July 18, 2023 22:29

staheri14 requested review from liamsi, adlerjohn, evan-forbes, rootulp and cmwaters as code owners July 18, 2023 22:29

staheri14 requested review from rootulp and removed request for rootulp July 18, 2023 22:29

MSevey requested a review from a team July 18, 2023 22:37

cmwaters reviewed Jul 19, 2023

View reviewed changes

rootulp reviewed Jul 19, 2023

View reviewed changes

removes a comment about endianness of version value

f48db89

MSevey requested a review from a team July 19, 2023 17:03

staheri14 added 2 commits July 19, 2023 10:24

revises the reserved namespaces intro paragraph

09ed3bb

replaces SHOULD with MUST

f171508

staheri14 requested review from rootulp and cmwaters July 19, 2023 17:46

removes a stale comment

d39223b

cmwaters approved these changes Jul 19, 2023

View reviewed changes

rootulp approved these changes Jul 19, 2023

View reviewed changes

staheri14 merged commit b77ac63 into main Jul 19, 2023
21 checks passed

staheri14 deleted the sanaz/namespace-specs-audit-update branch July 19, 2023 20:24

evan-forbes removed this from the Mainnet milestone Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

staheri14 commented Jul 18, 2023 •

edited

Loading

github-actions bot commented Jul 18, 2023 •

edited

Loading

codecov-commenter commented Jul 18, 2023 •

edited

Loading

staheri14 commented Jul 18, 2023

cmwaters Jul 19, 2023

staheri14 Jul 19, 2023

cmwaters Jul 19, 2023

cmwaters Jul 19, 2023

staheri14 Jul 19, 2023

staheri14 Jul 19, 2023

staheri14 Jul 19, 2023

cmwaters Jul 19, 2023

rootulp Jul 19, 2023

staheri14 Jul 19, 2023 •

edited

Loading

rootulp left a comment

rootulp Jul 19, 2023

staheri14 Jul 19, 2023

rootulp Jul 19, 2023

staheri14 Jul 19, 2023

rootulp Jul 19, 2023

staheri14 Jul 19, 2023

rootulp Jul 19, 2023

staheri14 Jul 19, 2023

cmwaters left a comment

rootulp left a comment

		## Protobuf Definition

		<!-- TODO: Add protobuf definition for namespace -->

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

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

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

Conversation

staheri14 commented Jul 18, 2023 • edited Loading

Overview

Checklist

github-actions bot commented Jul 18, 2023 • edited Loading

codecov-commenter commented Jul 18, 2023 • edited Loading

Codecov Report

staheri14 commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

staheri14 commented Jul 18, 2023 •

edited

Loading

github-actions bot commented Jul 18, 2023 •

edited

Loading

codecov-commenter commented Jul 18, 2023 •

edited

Loading

staheri14 Jul 19, 2023 •

edited

Loading