feat!: remove namespace and share version from `MsgPayForBlobs` #2930

rootulp · 2023-12-13T01:54:18Z

Notes for reviewers

a few tests had to be updated because the removal of these fields means that the MsgPayForBlobs encoded in the PFB namespace are slightly smaller. Tests that hard-coded share indices had to be adjusted slightly.
proto-check-breaking is expected to fail

coderabbitai · 2023-12-13T05:57:04Z

Walkthrough

The overarching change involves the removal of namespaces and share_versions fields from various parts of the codebase, particularly within messages and events related to blob transactions. This reflects a shift in the validation logic from namespace and share version errors to invalid share commitment errors, streamlining the data structure and potentially improving the efficiency of the validation process.

Changes

File(s)	Summary of Changes
`app/test/check_tx_test.go`	Changed the expected ABCI code value in the `TestCheckTx` function to reflect the alteration from `ErrNamespaceMismatch.ABCICode()` to `ErrInvalidShareCommitment.ABCICode()`.
`app/test/process_proposal_test.go`	Removed import statement for `user` package, removed function invocation related to user signing and address retrieval, and removed a test case related to an invalid namespace in an index wrapper transaction.
`pkg/proof/proof_test.go`	Adjusted the `startingShare` and `endingShare` values within the test cases of the `TestNewShareInclusionProof` function.
`pkg/square/builder_test.go`	Modified test cases for the `TestBuilderRejectsBlobTransactions` and `TestSquareBlobPostions` functions, impacting the control flow and expected outcomes of these tests.
`proto/celestia/blob/v1/event.proto`	Modified the `EventPayForBlobs` message by replacing the `namespaces` field with a `reserved` field.
`proto/celestia/blob/v1/tx.proto`	Altered the `MsgPayForBlobs` message by replacing `namespaces` and `share_versions` fields with `reserved` fields.
`test/util/blobfactory/payforblob_factory.go`	Removed the import of the package "bytes" and the function "IndexWrappedTxWithInvalidNamespace" from the file.
`x/blob/keeper/keeper.go`	Modified the `PayForBlobs` method within the `Keeper` type by altering the `EmitTypedEvent` function call to remove the `msg.Namespaces` argument.
`x/blob/keeper/keeper_test.go`	Removed the `namespaces` variable and its related assertion from the `TestPayForBlobs` function.
`x/blob/types/blob_tx.go`	Removed a block of code responsible for validating namespaces within the `ValidateBlobTx` function.
`x/blob/types/blob_tx_test.go`	Updated the expected error in the `TestValidateBlobTx` function from `types.ErrNamespaceMismatch` to `types.ErrInvalidShareCommitment`.
`x/blob/types/events.go`	Removed the `namespaces` parameter from the `NewPayForBlobsEvent` function signature and its corresponding assignment within the function body.
`x/blob/types/payforblob.go`	Refactored the `NewMsgPayForBlobs` function to remove the processing of namespaces and share versions, removed the `namespacesToBytes` function, and introduced the `GetBlobSizes` function to extract blob sizes.
`x/blob/types/payforblob_test.go`	Modified the `TestValidateBasic` function extensively, removing several declarations and test cases associated with removed declarations, and removing assertions related to namespace comparisons in the `TestNewMsgPayForBlobs` function.

Assessment against linked issues

Objective	Addressed	Explanation
Consider removing extraneous fields in `MsgPayForBlobs` (#2932)	✅
Evaluate the necessity of `namespaces` and `share_versions` fields (#2932)	✅
Propose the removal of `namespaces` and `share_versions` based on redundancy (#2932)	✅
Highlight the impact of removing fields on validation methods (#2932)	✅

The code changes align with the objectives of the linked issue, as they remove the namespaces and share_versions fields from MsgPayForBlobs and related structures, and update the validation logic accordingly.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

Note: Auto-reply has been disabled for this repository by the repository owner. The CodeRabbit bot will not respond to your comments unless it is explicitly tagged.

You can tag CodeRabbit on specific lines of code or entire files in the PR by tagging @coderabbitai in a comment. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
You can tag @coderabbitai in a PR comment and ask questions about the PR and the codebase. Examples:
- @coderabbitai generate interesting stats about this repository from git and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid.
- @coderabbitai read the files in the src/scheduler package and generate README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

rootulp · 2023-12-13T05:57:30Z

Ready for review but I don't plan on merging until @evan-forbes is back and has time to review.

x/blob/types/payforblob.go

evan-forbes

yikes, big-oof on us leaving these in. 🙈

left a comment below, but I think we will also need some if statements for versioning

evan-forbes · 2024-01-02T11:19:52Z

proto/celestia/blob/v1/tx.proto

  string signer = 1;
-  // namespaces is a list of namespaces that the blobs are associated with. A
-  // namespace is a byte slice of length 29 where the first byte is the
-  // namespaceVersion and the subsequent 28 bytes are the namespaceId.
-  repeated bytes namespaces = 2;
+  // 2 was previously used for namespaces.
+  reserved 2;
  // blob_sizes is a list of blob sizes (one per blob). Each size is in bytes.
  repeated uint32 blob_sizes = 3;
  // share_commitments is a list of share commitments (one per blob).
  repeated bytes share_commitments = 4;
-  // share_versions are the versions of the share format that the blobs
-  // associated with this message should use when included in a block. The
-  // share_versions specified must match the share_versions used to generate the
-  // share_commitment in this message.
-  repeated uint32 share_versions = 8;
+  // 8 was previously used for share_versions;
+  reserved 8;
 }


blocking

since this is breaking, do we need to create a v2 of the proto msg?

I think #2932 (comment) makes sense, and we might have to add an intermediate type to convert between v1 and v2 so that we don't have to have an extra copy of the logic to execute the msg.

rootulp · 2024-01-02T21:55:32Z

Yea good point. Going to close this PR and leave the issue open b/c I think it's prob safe to introduce a new type like MsgPayForBlobsV2 without these fields.

feat!: remove namespace and share version from MsgPayForBlobs

9d960fe

rootulp self-assigned this Dec 13, 2023

rootulp added 2 commits December 12, 2023 21:02

feat!: remove namespaces from event

2676147

fix: lint

b63ea72

rootulp added the breaking label Dec 13, 2023

rootulp marked this pull request as ready for review December 13, 2023 05:56

rootulp requested review from evan-forbes and cmwaters as code owners December 13, 2023 05:56

coderabbitai bot reviewed Dec 13, 2023

View reviewed changes

x/blob/types/payforblob.go Outdated Show resolved Hide resolved

fix: error params

14c605b

celestia-bot requested a review from a team December 13, 2023 06:13

refactor: un-export

0661370

coderabbitai bot reviewed Dec 13, 2023

View reviewed changes

x/blob/types/payforblob.go Outdated Show resolved Hide resolved

Merge branch 'main' into rp/remove-namespace-and-share-version

6fc623f

evan-forbes reviewed Jan 2, 2024

View reviewed changes

rootulp marked this pull request as draft January 2, 2024 21:54

rootulp closed this Jan 2, 2024

rootulp added consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules and removed breaking labels Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: remove namespace and share version from `MsgPayForBlobs` #2930

feat!: remove namespace and share version from `MsgPayForBlobs` #2930

rootulp commented Dec 13, 2023 •

edited

Loading

coderabbitai bot commented Dec 13, 2023 •

edited

Loading

Walkthrough

Changes

Assessment against linked issues

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

rootulp commented Dec 13, 2023

evan-forbes left a comment •

edited

Loading

evan-forbes Jan 2, 2024

rootulp commented Jan 2, 2024

feat!: remove namespace and share version from MsgPayForBlobs #2930

feat!: remove namespace and share version from MsgPayForBlobs #2930

Conversation

rootulp commented Dec 13, 2023 • edited Loading

Notes for reviewers

coderabbitai bot commented Dec 13, 2023 • edited Loading

Walkthrough

Changes

Assessment against linked issues

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

rootulp commented Dec 13, 2023

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

evan-forbes Jan 2, 2024

Choose a reason for hiding this comment

rootulp commented Jan 2, 2024

feat!: remove namespace and share version from `MsgPayForBlobs` #2930

feat!: remove namespace and share version from `MsgPayForBlobs` #2930

rootulp commented Dec 13, 2023 •

edited

Loading

coderabbitai bot commented Dec 13, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

evan-forbes left a comment •

edited

Loading