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!: decrease namespace from 33 to 29 bytes #1771

Merged
merged 8 commits into from
May 18, 2023
Merged

feat!: decrease namespace from 33 to 29 bytes #1771

merged 8 commits into from
May 18, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented May 15, 2023 •
edited
Loading

Closes #1753

@rootulp rootulp added consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules warn:api breaking item will be break an API and require a major bump labels May 15, 2023
@rootulp rootulp added this to the Mainnet milestone May 15, 2023
@rootulp rootulp self-assigned this May 15, 2023
@rootulp rootulp marked this pull request as ready for review May 17, 2023 19:54
evan-forbes
evan-forbes approved these changes May 18, 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.

:shipit: :shipit:

@evan-forbes
Copy link
Member

I did two :shipit:, which I rarely do, so interpret that as you will

rach-id
rach-id approved these changes May 18, 2023
View reviewed changes
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

Deferring to other reviewers for final approval

pkg/appconsts/appconsts.go
@@ -12,7 +12,7 @@ const (
NamespaceVersionSize = 1

// NamespaceIDSize is the size of a namespace ID in bytes.
NamespaceIDSize = 32
NamespaceIDSize = 28
Copy link
Member

Choose a reason for hiding this comment

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

[non blocking]
This const is also defined in core, wouldn't it make sense to use it here instead of rehardcoding it here?

pkg/da/data_availability_header_test.go
@@ -26,7 +26,7 @@ func TestNilDataAvailabilityHeaderHashDoesntCrash(t *testing.T) {

func TestMinDataAvailabilityHeader(t *testing.T) {
dah := MinDataAvailabilityHeader()
expectedHash := []byte{0xe9, 0x5, 0x28, 0x49, 0xf, 0x1d, 0x51, 0x67, 0x29, 0x2c, 0x1f, 0x1b, 0x83, 0xe1, 0x74, 0x2a, 0x27, 0x48, 0x17, 0x34, 0x12, 0xc9, 0x1d, 0xf7, 0xdd, 0x1, 0x96, 0x78, 0xa4, 0x62, 0xb9, 0x77}
expectedHash := []byte{0x3d, 0x96, 0xb7, 0xd2, 0x38, 0xe7, 0xe0, 0x45, 0x6f, 0x6a, 0xf8, 0xe7, 0xcd, 0xf0, 0xa6, 0x7b, 0xd6, 0xcf, 0x9c, 0x20, 0x89, 0xec, 0xb5, 0x59, 0xc6, 0x59, 0xdc, 0xaa, 0x1f, 0x88, 0x3, 0x53}
Copy link
Member

Choose a reason for hiding this comment

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

[Non Blocking][question]
I am wondering how these expected hashes are generated?

Copy link
Member

Choose a reason for hiding this comment

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

good follow up to document this imo.

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 am wondering how these expected hashes are generated?

I generate them by running the test after a share encoding change and copying the actual output to the expected output. Related doc:

celestia-app/x/blob/types/payforblob_test.go

Lines 64 to 66 in 207ec1a

// TestCreateCommitment will fail if a change is made to share encoding or how
// the commitment is calculated. If this is the case, the expected commitment
// bytes will need to be updated.

Copy link
Member

Choose a reason for hiding this comment

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

Cool 👍

@rootulp rootulp mentioned this pull request May 18, 2023
Open
@MSevey MSevey requested a review from a team May 18, 2023 16:04
staheri14
staheri14 approved these changes May 18, 2023
View reviewed changes
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

LGTM! added some suggestions and comments all non-blocking.

pkg/da/data_availability_header_test.go
@@ -42,13 +42,13 @@ func TestNewDataAvailabilityHeader(t *testing.T) {
tests := []test{
{
name: "typical",
expectedHash: []byte{0x5b, 0x27, 0x3e, 0x3a, 0x5d, 0x9e, 0x90, 0x25, 0x58, 0x21, 0xb7, 0xe0, 0x4d, 0x4b, 0xaa, 0xde, 0x37, 0xa6, 0x6f, 0xcc, 0xd, 0x16, 0x6f, 0x9e, 0xe0, 0x7f, 0xbe, 0x8, 0xb4, 0x41, 0xc8, 0xa6},
expectedHash: []byte{0xb5, 0x6e, 0x4d, 0x25, 0x1a, 0xc2, 0x66, 0xf4, 0xb9, 0x1c, 0xc5, 0x46, 0x4b, 0x3f, 0xc7, 0xef, 0xcb, 0xdc, 0x88, 0x80, 0x64, 0x64, 0x74, 0x96, 0xd1, 0x31, 0x33, 0xf0, 0xdc, 0x65, 0xac, 0x25},
squareSize: 2,
shares: generateShares(4),
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Can we use 2*2 instead of 4 in the generateShares(4), it is more clear how 4 is calculated and why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! #1788

pkg/shares/share_sequence_test.go Show resolved Hide resolved
pkg/shares/share_splitting_test.go Show resolved Hide resolved
pkg/shares/share_splitting_test.go
@@ -156,10 +156,10 @@ func TestSplitTxs_forTxShares(t *testing.T) {
appns.TxNamespace.Bytes(),
[]byte{
0x0, // info byte
0x0, 0x0, 0x0, 0x52, // reserved bytes
0x0, 0x0, 0x0, 0x4a, // reserved bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

[Suggestion, non-blocking] Could we include visual representations illustrating how the data is organized within their respective shares? by visual representation I mean something similar to the ones in this example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#1789

staheri14
staheri14 approved these changes May 18, 2023
View reviewed changes
Copy link
Contributor

@staheri14 staheri14 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! added some suggestions, questions, and comments all non-blocking.

@rootulp rootulp merged commit 5615205 into celestiaorg:main May 18, 2023
@rootulp rootulp mentioned this pull request May 18, 2023
Closed
@rootulp rootulp mentioned this pull request May 18, 2023
Open
@rootulp rootulp deleted the rp/29-byte-namespaces branch May 18, 2023 18:45
@rootulp rootulp mentioned this pull request Jun 5, 2023
Closed
@rootulp rootulp mentioned this pull request Jun 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rach-id rach-id rach-id approved these changes

@MSevey MSevey MSevey approved these changes

@staheri14 staheri14 staheri14 approved these changes

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

@liamsi liamsi Awaiting requested review from liamsi liamsi is a code owner

@adlerjohn adlerjohn Awaiting requested review from adlerjohn

@cmwaters cmwaters Awaiting requested review from cmwaters

Assignees

@rootulp rootulp

Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules 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.

Decrease the namespace size from 33 => 29 bytes
5 participants
@rootulp @evan-forbes @MSevey @staheri14 @rach-id