feat: Introduce `--namespace-version` CLI flag #1585

amityadav0 · 2023-03-31T13:25:20Z

Overview

Introduce --namespace-version CLI flag

Closes Introduce --namespace-version CLI flag #1528

I have introduced namespace flag. Currently this supports namespace version zero only.
I think data structure for storing namespace versions can be updated. Maybe a map with version details.
For example:

Key: Version[0], Value: { NamespaceVersionPrefixSize: ... , NamespaceVersionIDSize: ... }
Key: Version[1], Value: { NamespaceVersionPrefixSize: ... , NamespaceVersionIDSize: ... }

instead of constants

	NamespaceVersionZero = uint8(0)
	NamespaceVersionZeroPrefixSize = 22
	NamespaceVersionZeroIDSize = NamespaceIDSize - NamespaceVersionZeroPrefixSize

Then it would be possible to switch versions easily from cli. same for share version.

Please let me know if this makes sense or am I missing something.

rootulp

Sorry for the delay in reviewing @amityadav0 but thanks for working on this!

x/blob/client/cli/payforblob.go

Co-authored-by: Rootul P <rootulp@gmail.com>

amityadav0 · 2023-04-05T08:54:54Z

Thanks for the review @rootulp. I have updated the PR.

codecov-commenter · 2023-04-05T09:07:30Z

Codecov Report

Merging #1585 (8cb9afe) into main (e905143) will decrease coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1585      +/-   ##
==========================================
- Coverage   49.17%   49.04%   -0.13%     
==========================================
  Files          84       85       +1     
  Lines        4948     4963      +15     
==========================================
+ Hits         2433     2434       +1     
- Misses       2277     2290      +13     
- Partials      238      239       +1

see 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rootulp · 2023-04-05T20:35:18Z

Thanks for updating the PR! This overall LGTM. I had two optional suggestions that I applied on top of this PR. I made the changes myself b/c I forgot to include them in the previous round of PR feedback and I didn't want to block the PR on addressing them.

evan-forbes

Left a few minor blocking questions/change, thanks @amityadav0 !!

x/blob/client/cli/payforblob.go

x/blob/client/testutil/integration_test.go

evan-forbes · 2023-04-07T22:14:00Z

x/blob/client/cli/payforblob.go

@@ -57,10 +60,24 @@ func CmdPayForBlob() *cobra.Command {
 	}

 	flags.AddTxFlagsToCmd(cmd)
+	cmd.PersistentFlags().String(FlagNamespaceVersion, "0", "Specify the namespace version (default is 0)")


we shouldn't write defaults, since they're filled in for us by cobra

Suggested change

cmd.PersistentFlags().String(FlagNamespaceVersion, "0", "Specify the namespace version (default is 0)")

cmd.PersistentFlags().String(FlagNamespaceVersion, "0", "Specify the namespace version")

@evan-forbes where are they filled in? After this suggestion the help doc looks like:

$ ./build/celestia-appd tx blob PayForBlobs --help ... --namespace-version uint8 Specify the namespace version

hmmm I'm not sure that's a good question. the rest get filled in for example https://github.com/celestiaorg/cosmos-sdk/blob/f19e20459b8bcf195167ad256de5a6ca6edc41f5/server/start.go#L159-L161

--address string Listen address (default "tcp://0.0.0.0:26658") --home string The application home directory (default "/home/evan/.celestia-app")

Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>

rootulp

2 small nits based on the last change to uint8 flag

x/blob/client/cli/payforblob.go

Co-authored-by: Rootul P <rootulp@gmail.com>

evan-forbes

Missed this first time around! sorry about that @amityadav0 thanks again for your patience

evan-forbes · 2023-04-12T00:42:41Z

x/blob/client/cli/payforblob.go

+func getNamespace(namespaceID []byte, namespaceVersion uint8) (appns.Namespace, error) {
+	switch namespaceVersion {
+	case appns.NamespaceVersionZero:
+		return appns.New(namespaceVersion, append(appns.NamespaceVersionZeroPrefix, namespaceID...))


we should never append directly to a global, as it could cause tons of bugs

we need to first make an empty slice of appropriate capacity, and then append

I don't think we are appending to global value. Append function will return a value here which is passed to appns.New()
z = append(x, y)
This line won't change x or y.

the issue is that as a rule of thumb, we should never do
z = append(x, y)
only ever
z = append(z, x, y)

this can, and has, caused quite a few really tricky bugs in the past
https://www.calhoun.io/why-are-slices-sometimes-altered-when-passed-by-value-in-go/

Got it. Will read this in depth and update the PR. Thanks for the answer.

My takeaway after reading that article is that

z = append(x, y)

is safe because the length or capacity of x will not be impacted by the above statement. After execution the global x remains unmodified. My understanding is that the copy is only necessary if a function is invoked on the global x that modifies elements of the original slice. If append is dangerous in this usage, I would expect there to be a linter to warn us about incorrect usage but I couldn't find one on https://golangci-lint.run/usage/linters/

@evan-forbes how do we enforce that append isn't misused? Additionally are there examples of just the append causing bugs:

we should never append directly to a global, as it could cause tons of bugs

I think we have some linter, but otherwise this is just good practice imo since it always fixes the issue and eliminates the reader having to think "is this an issue". Also, I still think its an issue but don't have the brain capacity at the moment to figure it out lol 😆

examples celestiaorg/celestia-core#336, celestiaorg/celestia-core#250, celestiaorg/nmt#30, there was also one in node that fixed during the barcelona onsite but I couldn't find it in a cursory search

all were caused by
z = append(x, y)

so now we should never ever ever do that no matter what because evan is too scarred

I have updated the code. Have created a copy of global and then used append on that copy

evan-forbes · 2023-04-13T13:11:04Z

x/blob/client/cli/payforblob.go

+		namespaceVersionZeroPrefix := appns.NamespaceVersionZeroPrefix
+		return appns.New(namespaceVersion, append(namespaceVersionZeroPrefix, namespaceID...))


this doesn't actually avoid the issue, since slices can be treated similar to pointers. Its true that we're passing by value here, but we're passing a pointer by value, which unfortunatley still won't solve our problem

in order to avoid the underlying issue, we need to make an explicit copy of the slice
similar to

ns := make([]byte, 0, appconsts.NamespaceSize) ns = append(ns, append(prefix, namespaceID)...)

I guess now I get it.
with above example prefix may still be changed

nsp := make([]byte, 0, appns.NamespaceVersionZeroPrefixSize) nsp = append(nsp, appns.NamespaceVersionZeroPrefix...) return appns.New(namespaceVersion, append(nsp, namespaceID...))

I have updated the code to this

evan-forbes

made super minor change to push this over the line

thanks again @amityadav0 !! apologies this took so long

x/blob/client/cli/payforblob.go

rootulp · 2023-04-17T19:37:00Z

x/blob/client/cli/payforblob.go

+func getNamespace(namespaceID []byte, namespaceVersion uint8) (appns.Namespace, error) {
+	switch namespaceVersion {
+	case appns.NamespaceVersionZero:
+		nsp := make([]byte, 0, appns.NamespaceSize)


[nit]

Suggested change

nsp := make([]byte, 0, appns.NamespaceSize)

nsp := make([]byte, 0, appns.NamespaceIDSize)

why not namespace size here? aren't we using the full 33?

The ID is 32 bytes. The version is 1 byte. The total namespace is 33 bytes.

Since this variable is just the ID, only 32 bytes are necessary.

merged to clear out his PR, but we should still fix this imo if need be,

I'm confused a bit

so we should not be appending the version at all here since its added when we call Bytes()? therefore we're adding it twice?

amityadav0 added 2 commits March 31, 2023 18:09

add namespace flag

ac5d444

fix

75e9c50

amityadav0 requested a review from evan-forbes as a code owner March 31, 2023 13:25

MSevey requested review from a team and cmwaters and removed request for a team March 31, 2023 13:25

github-actions bot added the external label Mar 31, 2023

rootulp changed the title ~~Introduce --namespace-version CLI flag~~ Introduce --namespace-version CLI flag Mar 31, 2023

rootulp assigned amityadav0 Mar 31, 2023

rootulp changed the title ~~Introduce --namespace-version CLI flag~~ feat: Introduce --namespace-version CLI flag Mar 31, 2023

rootulp reviewed Apr 4, 2023

View reviewed changes

x/blob/client/cli/payforblob.go Outdated Show resolved Hide resolved

x/blob/client/cli/payforblob.go Outdated Show resolved Hide resolved

Update x/blob/client/cli/payforblob.go

be7a4a9

Co-authored-by: Rootul P <rootulp@gmail.com>

MSevey requested a review from a team April 5, 2023 08:46

few changes

e61f256

add comment

7ffca88

rootulp added 2 commits April 5, 2023 16:34

chore: extract getNamespace

c1dadfd

test: unit test invalid namespace ID and version

ce48149

rootulp previously approved these changes Apr 5, 2023

View reviewed changes

evan-forbes requested changes Apr 7, 2023

View reviewed changes

Update x/blob/client/cli/payforblob.go

380f958

Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>

amityadav0 dismissed rootulp’s stale review via 380f958 April 8, 2023 07:06

MSevey requested a review from a team April 8, 2023 07:06

change flag to uint8

a61fb39

rootulp requested changes Apr 10, 2023

View reviewed changes

x/blob/client/cli/payforblob.go Outdated Show resolved Hide resolved

x/blob/client/cli/payforblob.go Outdated Show resolved Hide resolved

amityadav0 and others added 2 commits April 10, 2023 21:19

Update x/blob/client/cli/payforblob.go

1d3d2a1

Co-authored-by: Rootul P <rootulp@gmail.com>

Update x/blob/client/cli/payforblob.go

736ef26

Co-authored-by: Rootul P <rootulp@gmail.com>

MSevey requested a review from a team April 10, 2023 15:49

rootulp previously approved these changes Apr 10, 2023

View reviewed changes

rootulp requested a review from evan-forbes April 10, 2023 16:48

Merge branch 'main' into namespace-version

1b7c0f9

amityadav0 dismissed rootulp’s stale review via 1b7c0f9 April 11, 2023 16:00

MSevey requested a review from a team April 11, 2023 16:00

lint

9838fcb

evan-forbes requested changes Apr 12, 2023

View reviewed changes

append changes

db18cc2

MSevey requested a review from a team April 13, 2023 08:55

evan-forbes reviewed Apr 13, 2023

View reviewed changes

update

316e50a

MSevey requested a review from a team April 13, 2023 15:19

amityadav0 requested a review from evan-forbes April 14, 2023 14:58

refactor: use appropriate size when initializaing capacity of ns

a3d22c1

evan-forbes previously approved these changes Apr 17, 2023

View reviewed changes

evan-forbes enabled auto-merge (squash) April 17, 2023 12:49

evan-forbes requested a review from rootulp April 17, 2023 12:49

chore: check for err & use NamespaceIDSize

8cb9afe

rootulp reviewed Apr 17, 2023

View reviewed changes

rootulp dismissed evan-forbes’s stale review via 8cb9afe April 17, 2023 19:37

MSevey requested a review from a team April 17, 2023 19:37

rootulp approved these changes Apr 17, 2023

View reviewed changes

rootulp requested review from evan-forbes and removed request for cmwaters April 17, 2023 19:37

rootulp added this to the Mainnet milestone Apr 17, 2023

evan-forbes approved these changes Apr 19, 2023

View reviewed changes

evan-forbes merged commit d88adf7 into celestiaorg:main Apr 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce `--namespace-version` CLI flag #1585

feat: Introduce `--namespace-version` CLI flag #1585

amityadav0 commented Mar 31, 2023 •

edited

Loading

rootulp left a comment

amityadav0 commented Apr 5, 2023

codecov-commenter commented Apr 5, 2023 •

edited

Loading

rootulp commented Apr 5, 2023

evan-forbes left a comment •

edited

Loading

evan-forbes Apr 7, 2023

rootulp Apr 10, 2023

evan-forbes Apr 12, 2023

rootulp left a comment

evan-forbes left a comment

evan-forbes Apr 12, 2023

amityadav0 Apr 12, 2023

evan-forbes Apr 12, 2023

amityadav0 Apr 12, 2023

rootulp Apr 12, 2023

evan-forbes Apr 12, 2023

amityadav0 Apr 13, 2023

evan-forbes Apr 13, 2023

amityadav0 Apr 13, 2023

evan-forbes left a comment

rootulp Apr 17, 2023

evan-forbes Apr 18, 2023

rootulp Apr 18, 2023

evan-forbes Apr 19, 2023

	cmd.PersistentFlags().String(FlagNamespaceVersion, "0", "Specify the namespace version (default is 0)")
	cmd.PersistentFlags().String(FlagNamespaceVersion, "0", "Specify the namespace version")

		namespaceVersionZeroPrefix := appns.NamespaceVersionZeroPrefix
		return appns.New(namespaceVersion, append(namespaceVersionZeroPrefix, namespaceID...))

	nsp := make([]byte, 0, appns.NamespaceSize)
	nsp := make([]byte, 0, appns.NamespaceIDSize)

feat: Introduce --namespace-version CLI flag #1585

feat: Introduce --namespace-version CLI flag #1585

Conversation

amityadav0 commented Mar 31, 2023 • edited Loading

Overview

rootulp left a comment

Choose a reason for hiding this comment

amityadav0 commented Apr 5, 2023

codecov-commenter commented Apr 5, 2023 • edited Loading

Codecov Report

rootulp commented Apr 5, 2023

evan-forbes left a comment • edited Loading

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

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes 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

Choose a reason for hiding this comment

evan-forbes 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

feat: Introduce `--namespace-version` CLI flag #1585

feat: Introduce `--namespace-version` CLI flag #1585

amityadav0 commented Mar 31, 2023 •

edited

Loading

codecov-commenter commented Apr 5, 2023 •

edited

Loading

evan-forbes left a comment •

edited

Loading