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.

Sign up for GitHub

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

Jump to bottom

feat!: reserve the last 256 namespaces for protocol use #2257

Merged
merged 7 commits into from
Aug 14, 2023
Merged

feat!: reserve the last 256 namespaces for protocol use #2257

merged 7 commits into from
Aug 14, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Aug 11, 2023

Closes #2253

@rootulp rootulp added consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules specs directly relevant to the specs warn:api breaking item will be break an API and require a major bump backport:v1.x PR will be backported automatically to the v1.x branch upon merging labels Aug 11, 2023
@rootulp rootulp added this to the Mainnet milestone Aug 11, 2023
@rootulp rootulp self-assigned this Aug 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2023 •
edited
Loading

Codecov Report

Merging #2257 (9f75450) into main (86c06b5) will increase coverage by 0.12%.
The diff coverage is 91.30%.

@@            Coverage Diff             @@
##             main    #2257      +/-   ##
==========================================
+ Coverage   21.55%   21.67%   +0.12%     
==========================================
  Files         128      129       +1     
  Lines       14346    14357      +11     
==========================================
+ Hits         3092     3112      +20     
+ Misses      10949    10940       -9     
  Partials      305      305
Files Changed Coverage Δ
pkg/namespace/namespace.go 73.14% <71.42%> (+8.66%) ⬆️
pkg/namespace/consts.go 100.00% <100.00%> (ø)
pkg/shares/padding.go 52.08% <100.00%> (ø)
pkg/shares/shares.go 69.00% <100.00%> (ø)

@rootulp rootulp marked this pull request as ready for review August 11, 2023 19:03
@rootulp rootulp requested review from staheri14 and removed request for adlerjohn and liamsi August 11, 2023 19:04
rootulp
rootulp commented Aug 11, 2023
View reviewed changes
specs/src/specs/namespace.md
@@ -57,30 +57,32 @@ The ID is encoded as a byte slice of length 28.

## Reserved Namespaces
Copy link
Collaborator Author

@rootulp rootulp Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I rephrased this section a bit. Please LMK if you think I should revert anything @staheri14

rach-id
rach-id reviewed Aug 11, 2023
View reviewed changes
pkg/namespace/consts.go Show resolved Hide resolved
pkg/namespace/consts.go Show resolved Hide resolved
Wondertan
Wondertan reviewed Aug 12, 2023
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Does node needs to be updated with this?

@rootulp
Copy link
Collaborator Author

rootulp commented Aug 12, 2023

Does node needs to be updated with this?

Yes. From a cursory investigation:

  1. Rename MaxReservedNamespace to MaxPrimaryReservedNamespace
  2. Rename ReservedPaddingNamespace to PrimaryReservedPaddingNamespace
  3. Add MinSecondaryReservedNamespace constant to this list
  4. ValidateForBlob should account for the secondary reserved namespace range.
  5. Add unit tests

@rootulp rootulp mentioned this pull request Aug 12, 2023
Closed
@MSevey MSevey requested a review from a team August 12, 2023 16:19
@rootulp rootulp mentioned this pull request Aug 12, 2023
Closed
cmwaters
cmwaters previously approved these changes Aug 14, 2023
View reviewed changes
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

specs/src/specs/namespace.md Show resolved Hide resolved
@MSevey MSevey requested a review from a team August 14, 2023 13:56
evan-forbes
evan-forbes approved these changes Aug 14, 2023
View reviewed changes
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.

nice! this is even simpler than the old implementation!

@rootulp rootulp merged commit cb8247e into celestiaorg:main Aug 14, 2023
19 of 21 checks passed
mergify bot pushed a commit that referenced this pull request Aug 14, 2023
@rootulp @mergify
feat!: reserve the last 256 namespaces for protocol use (#2257)
25b2df7 
Closes #2253

(cherry picked from commit cb8247e)

# Conflicts:
#	pkg/namespace/consts.go
#	pkg/namespace/namespace.go
#	pkg/namespace/namespace_test.go
#	pkg/shares/padding.go
#	specs/src/specs/namespace.md
@mergify mergify bot mentioned this pull request Aug 14, 2023
Merged
rootulp added a commit that referenced this pull request Aug 14, 2023
@rootulp
Revert "feat!: reserve the last 256 namespaces for protocol use (#2257)"
b76d74c 
This reverts commit 25b2df7.
rootulp added a commit that referenced this pull request Aug 14, 2023
@rootulp
feat!: reserve the last 256 namespaces for protocol use (#2257)
8bd17d3 
Closes #2253
rootulp added a commit that referenced this pull request Aug 15, 2023
@rootulp
feat!: reserve the last 256 namespaces for protocol use (backport #2257…
53b5dbf 
…) (#2261)
@rootulp rootulp mentioned this pull request Aug 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan left review comments

@cmwaters cmwaters cmwaters approved these changes

@evan-forbes evan-forbes evan-forbes approved these changes

@staheri14 staheri14 Awaiting requested review from staheri14

@rach-id rach-id Awaiting requested review from rach-id

Assignees

@rootulp rootulp

Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules specs directly relevant to the specs warn:api breaking item will be break an API and require a major bump
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reserve the last 256 namespaces for the protocol
6 participants
@rootulp @codecov-commenter @Wondertan @cmwaters @rach-id @evan-forbes