celestiaorg / celestiaorg/nmt

namespace: PrefixedData/PrefixedData8 are easy to misuse

Open
#71 1 comment 4 reactions 0 assignees View on GitHub
API enhancement
Dominant language
Go
Stars
122
Forks
53
Avg merge
1d 15h
Merged PRs (30d)
5

Description

The `PrefixedData` and `PrefixedData8` are both byte slices:

```go
// PrefixedData simply represents a slice of bytes which consists of
// a namespace.ID and raw data.
// The user has to guarantee that the bytes are valid namespace prefixed data.
// Go's type system does not allow enforcing the structure we want:
// [namespaceID, rawData ...], especially as this type does not expect any
// particular size for the namespace.
type PrefixedData []byte

// PrefixedData8 like PrefixedData is just a slice of bytes.
// It assumes that the slice it represents is at least 8 bytes.
// This assumption is not enforced by the type system though.
type PrefixedData8 []byte
```

The definitions are problematic, because (1) casting byte slices may violate the type invariants (as pointed out in the comments) (2) they're mutable (3) PrefixedData has no way to indicate the namespace identifier length. For an example where mutability is a problem, see

https://github.com/celestiaorg/nmt/blob/62742433f65a37daf889ee563b0ee117c83e9664/nmt.go#L253-L257

where it is not clear to the caller that the parameter `namespacedData` is retained by the `Push` method.

The missing namespace length is problematic for `validateAndExtractNamespace`,

https://github.com/celestiaorg/nmt/blob/62742433f65a37daf889ee563b0ee117c83e9664/nmt.go#L336-L340

because it can only validate that the namespace+data total length is at least as long as the expected namespace length.

Perhaps the definitions of `PrefixedData` and `PrefixedData8` are caused by wanting to support zero-copy and zero-allocation use, where the user generates `PrefixedData` values by slicing existing byte slices. If so, the behaviour of `Push` should document that argument values are retained.

Otherwise, I recommend changing the definition to hide the internal structure, and add validating constructors. There are several possibilites:

```go
type PrefixedData struct {
// namespace and data can be sliced from the same
// underlying []byte allocation.
namespace []byte
data []byte
}

func NewPrefixedData(namespace, data []byte) PrefixedData {
bytes := make([]byte, len(namespace)+len(data))
d := PrefixedData{
namespace: bytes[:len(namespace)],
data: bytes[len(namespace)],
}
copy(d.namespace, namespace)
copy(d.data, data)
return d
}
```

or equivalently

```go
type PrefixedData struct {
namespaceAndData []byte
namespaceLen int
}
```

@odeke-em

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.