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.

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

ValidateBlobNamespace: cleaning up and future-proofing #2143

Closed
ivan-gavran opened this issue Jul 21, 2023 · 0 comments · Fixed by #2145
Closed

ValidateBlobNamespace: cleaning up and future-proofing #2143

ivan-gavran opened this issue Jul 21, 2023 · 0 comments · Fixed by #2145
Assignees
Labels
audit item is from an audit

Comments

@ivan-gavran
Copy link

There seems to be some confusion about blob namespace validation:

  • there are two functions called ValidateBlobNamespace: one (A) is a member function of the Namespace structure, and the other (B) takes Namespace as an argument. The two functions are almost identical, the only difference being that B checks if the namespace version is NamespaceVersionZero.
  • Interestingly, both functions are used in a similar context: A is called from within ValidateBlobTx (which is called both from ProcessProposal and CheckTx). B is called from within ValidateBasic of MsgPayForBlobs, which is also called from ValidateBlobTx.
  • My guess is that calling both of these functions is unnecessary and that B should be the used one.

Finally, a question of future-proving: would it make sense to replace the explicit check for version 0,

if ns.Version != appns.NamespaceVersionZero {
		return ErrInvalidNamespaceVersion
	}

with a check for whether the ns.Version is among the supported user-namespace versions? (Along the lines of how it was done for share versions, here.)

There is another function which validates namespace versions, ValidateVersion. In this validation, both zero-namespace and max-namespace are allowed (so, it is checking for any possible versions, and not just user-created blobs). It would probably make sense to change the name of that function into something more concrete, e.g., ValidateVersionSupported.

relevant commit: v1.0.0-rc6

@ivan-gavran ivan-gavran changed the title ValidateBlobNamespace: cleaning up and future-proving ValidateBlobNamespace: cleaning up and future-proofing Jul 21, 2023
@rootulp rootulp added audit item is from an audit and removed external labels Jul 21, 2023
@rootulp rootulp self-assigned this Jul 21, 2023
@rootulp rootulp added this to the Post-mainnet milestone Jul 21, 2023
rootulp added a commit that referenced this issue Jul 27, 2023
Closes #2143

De-duplicate `ValidateBlobNamespace` and export it from `x/blob/types`
package. The namespace package needs to define its own `isBlobNamespace`
because it can't import `ValidateBlobNamespace` from `x/blob/types` as
that would introduce a circular dependency.
rootulp added a commit that referenced this issue Aug 14, 2023
Closes #2143

De-duplicate `ValidateBlobNamespace` and export it from `x/blob/types`
package. The namespace package needs to define its own `isBlobNamespace`
because it can't import `ValidateBlobNamespace` from `x/blob/types` as
that would introduce a circular dependency.
cmwaters pushed a commit to celestiaorg/go-square that referenced this issue Dec 14, 2023
Closes celestiaorg/celestia-app#2143

De-duplicate `ValidateBlobNamespace` and export it from `x/blob/types`
package. The namespace package needs to define its own `isBlobNamespace`
because it can't import `ValidateBlobNamespace` from `x/blob/types` as
that would introduce a circular dependency.
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
Closes celestiaorg/celestia-app#2143

De-duplicate `ValidateBlobNamespace` and export it from `x/blob/types`
package. The namespace package needs to define its own `isBlobNamespace`
because it can't import `ValidateBlobNamespace` from `x/blob/types` as
that would introduce a circular dependency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit item is from an audit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants