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: Introduce --namespace-version CLI flag #1585

Merged
merged 17 commits into from
Apr 19, 2023
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ac5d444
add namespace flag
amityadav0 Mar 31, 2023
75e9c50
fix
amityadav0 Mar 31, 2023
be7a4a9
Update x/blob/client/cli/payforblob.go
amityadav0 Apr 5, 2023
e61f256
few changes
amityadav0 Apr 5, 2023
7ffca88
add comment
amityadav0 Apr 5, 2023
c1dadfd
chore: extract getNamespace
rootulp Apr 5, 2023
ce48149
test: unit test invalid namespace ID and version
rootulp Apr 5, 2023
380f958
Update x/blob/client/cli/payforblob.go
amityadav0 Apr 8, 2023
a61fb39
change flag to uint8
amityadav0 Apr 8, 2023
1d3d2a1
Update x/blob/client/cli/payforblob.go
amityadav0 Apr 10, 2023
736ef26
Update x/blob/client/cli/payforblob.go
amityadav0 Apr 10, 2023
1b7c0f9
Merge branch 'main' into namespace-version
amityadav0 Apr 11, 2023
9838fcb
lint
amityadav0 Apr 11, 2023
db18cc2
append changes
amityadav0 Apr 13, 2023
316e50a
update
amityadav0 Apr 13, 2023
a3d22c1
refactor: use appropriate size when initializaing capacity of ns
evan-forbes Apr 17, 2023
8cb9afe
chore: check for err & use NamespaceIDSize
rootulp Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions x/blob/client/cli/payforblob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ import (
coretypes "github.com/tendermint/tendermint/types"
)

// FlagNamespaceVersion allows the user to override the namespace version when
// submitting a PayForBlob.
const FlagNamespaceVersion = "namespace-version"

func CmdPayForBlob() *cobra.Command {
cmd := &cobra.Command{
Use: "PayForBlobs [hexNamespaceID] [hexBlob]",
Expand All @@ -33,12 +37,10 @@ func CmdPayForBlob() *cobra.Command {
if err != nil {
return fmt.Errorf("failure to decode hex namespace ID: %w", err)
}

// TODO: allow the user to override the namespace version via a new flag
// See https://github.com/celestiaorg/celestia-app/issues/1528
namespace, err := appns.New(appns.NamespaceVersionZero, append(appns.NamespaceVersionZeroPrefix, namespaceID...))
namespaceVersion, _ := cmd.Flags().GetUint8(FlagNamespaceVersion)
rootulp marked this conversation as resolved.
Show resolved Hide resolved
namespace, err := getNamespace(namespaceID, namespaceVersion)
if err != nil {
return fmt.Errorf("failure to create namespace: %w", err)
return err
}

rawblob, err := hex.DecodeString(args[1])
Expand All @@ -57,10 +59,20 @@ func CmdPayForBlob() *cobra.Command {
}

flags.AddTxFlagsToCmd(cmd)
cmd.PersistentFlags().Uint8(FlagNamespaceVersion, 0, "Specify the namespace version")

return cmd
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

default:
return appns.Namespace{}, fmt.Errorf("namespace version %d is not supported", namespaceVersion)
}
}

// broadcastPFB creates the new PFB message type that will later be broadcast to tendermint nodes
// this private func is used in CmdPayForBlob and CmdTestRandBlob
func broadcastPFB(cmd *cobra.Command, blob *types.Blob) error {
Expand Down
33 changes: 32 additions & 1 deletion x/blob/client/testutil/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package testutil

import (
"bytes"
"encoding/hex"
"fmt"
"strconv"
Expand All @@ -20,6 +21,7 @@ import (
appns "github.com/celestiaorg/celestia-app/pkg/namespace"
"github.com/celestiaorg/celestia-app/testutil/network"
"github.com/celestiaorg/celestia-app/testutil/testfactory"
"github.com/celestiaorg/celestia-app/x/blob/client/cli"
paycli "github.com/celestiaorg/celestia-app/x/blob/client/cli"
abci "github.com/tendermint/tendermint/abci/types"
)
Expand Down Expand Up @@ -62,8 +64,8 @@ func (s *IntegrationTestSuite) TestSubmitPayForBlob() {
require := s.Require()
val := s.network.Validators[0]
hexNamespace := hex.EncodeToString(appns.RandomBlobNamespaceID())
invalidNamespaceID := hex.EncodeToString(bytes.Repeat([]byte{0}, 8)) // invalid because ID is expected to be 10 bytes
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved

// some hex blob
hexBlob := "0204033704032c0b162109000908094d425837422c2116"

testCases := []struct {
Expand All @@ -87,6 +89,35 @@ func (s *IntegrationTestSuite) TestSubmitPayForBlob() {
expectedCode: 0,
respType: &sdk.TxResponse{},
},
{
name: "invalid namespace ID",
args: []string{
invalidNamespaceID,
hexBlob,
fmt.Sprintf("--from=%s", username),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(2))).String()),
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
},
expectErr: true,
expectedCode: 0,
respType: &sdk.TxResponse{},
},
{
name: "invalid namespace version",
args: []string{
hexNamespace,
hexBlob,
fmt.Sprintf("--from=%s", username),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(2))).String()),
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
fmt.Sprintf("--%s=1", cli.FlagNamespaceVersion),
},
expectErr: true,
expectedCode: 0,
respType: &sdk.TxResponse{},
},
}

for _, tc := range testCases {
Expand Down