feat!: multiple blobs per PFB #1154

evan-forbes · 2022-12-23T06:44:47Z

Overview

This PR implements the ability to add an arbitrary number of blobs to a single PFB. Leaving as a draft until we merge the blocking PR in core, and probably tidy up a bit, rebase, or add a few more unit tests.

In part thanks to us planning ahead earlier this year, there's actually not that many required changes to get multiple blobs per PFB. Mostly just adding tests, using a new mechanism to calculate the share commitments, making a few things a slice instead of a single thing, and minor adjustments to make square estimation/square layout work with multiple blobs.

closes #388

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

codecov-commenter · 2022-12-23T07:00:20Z

Codecov Report

Merging #1154 (99df848) into main (c57a0bf) will increase coverage by 0.31%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main    #1154      +/-   ##
==========================================
+ Coverage   48.20%   48.52%   +0.31%     
==========================================
  Files          72       72              
  Lines        4070     4134      +64     
==========================================
+ Hits         1962     2006      +44     
- Misses       1936     1951      +15     
- Partials      172      177       +5

Impacted Files	Coverage Δ
app/check_tx.go	`0.00% <0.00%> (ø)`
app/prepare_proposal.go	`0.00% <ø> (ø)`
app/process_proposal.go	`0.00% <0.00%> (ø)`
x/blob/payforblob.go	`0.00% <0.00%> (ø)`
x/blob/types/blob_tx.go	`18.57% <0.00%> (-5.51%)`	⬇️
x/blob/types/events.go	`0.00% <0.00%> (ø)`
testutil/testnode/node_interaction_api.go	`56.41% <50.00%> (-1.45%)`	⬇️
pkg/shares/share_splitting.go	`71.60% <66.66%> (ø)`
x/blob/types/payforblob.go	`81.09% <73.07%> (-0.73%)`	⬇️
app/estimate_square_size.go	`88.46% <73.33%> (-6.78%)`	⬇️
... and 6 more

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

evan-forbes · 2022-12-23T15:35:40Z

we should be able to split one or two minor refactors out of this PR

rootulp

Overall looks really good to me! Hyped for this feature

rootulp · 2022-12-23T15:36:22Z

app/estimate_square_size_test.go

+			func() [][]int { return [][]int{{4}} },
+			2, 1,


[question] if the square size is 2, the blob size is 4, and the expected starting index is 1, how does the blob fit in the square? I would expect something like:

C = compact share B = blob share |C|B| |B|B|

with one B share leftover

ahh I see, this threw me off after you mentioned it too. This only takes a single share as its blobsize, not the number of shares that are used. so we have single single share tx, and therefore

|C|B| |_|_|

ohh I see, blobSizes is the # of bytes in the blob. [No change needed] then I expect this to behave similarly and indeed it does

Suggested change

func() [][]int { return [][]int{{4}} },

2, 1,

func() [][]int { return [][]int{{1}} },

2, 1,

app/estimate_square_size_test.go

app/test/block_size_test.go

pkg/inclusion/get_commit.go

proto/blob/event.proto

proto/blob/tx.proto

testutil/blobfactory/payforblob_factory.go

x/blob/client/cli/wirepayfordata.go

x/blob/types/payforblob.go

cmwaters

Just some high level comments first

cmwaters · 2023-01-03T10:39:07Z

proto/blob/tx.proto

  // with this message should use when included in a block. The share_version
  // specified must match the share_version used to generate the
  // share_commitment in this message.
-  uint32 share_version = 8;
+  repeated uint32 share_versions = 8;


Question: would we ever want a message to refer to blobs that have different share_versions? Can't we enforce that all blobs must have the same share_version

there could maybe be a scenario with multiple share_versions, but we could enforce that if we wanted to. Its probably a good trade-off

Ok perhaps a better question: What changes would cause us to change the share version?

One use case is to add the sender of the PFB, or other metadata, before the blob in the square.

Other usecases would be to allow posters to not use varints or some other encoding should it be useful.

I'm not sure if people will ever want to mix those tho. Even if they do, then only in that specific case do we have more linear increase in state transitions per different share version.

With the benefit of removing a lot of redundant bytes. Which sounds like a really good tradeoff.

Yeah I don't think that constraining the share versions for blobs within a single PFB is going to be a problem for users. Worst case they just have two PFBs (for each version) and pay the extra 2 cents in gas.

left an issue in #1223

x/blob/client/cli/wirepayfordata.go

cmwaters · 2023-01-03T10:48:16Z

proto/blob/tx.proto

@@ -26,16 +26,16 @@ message ShareCommitAndSignature {
 // MsgPayForBlob pays for the inclusion of a blob in the block.
 message MsgPayForBlob {


Suggested change

message MsgPayForBlob {

message MsgPayForBlobs {

?

Definitely!! started to change this in this PR, but since this PR is already large, decided to spin it off as a follow up. #1221

cmwaters · 2023-01-03T10:49:43Z

proto/blob/tx.proto

@@ -26,16 +26,16 @@ message ShareCommitAndSignature {
 // MsgPayForBlob pays for the inclusion of a blob in the block.
 message MsgPayForBlob {
  string signer = 1;
-  bytes namespace_id = 2;
-  uint64 blob_size = 3;
+  repeated bytes namespace_ids = 2;


Thinking out loud: Most cases of submitting multiple blobs would all probably be for the same namespace. Could we add a rule that if there is a single namespace_id yet multiple blob_sizes that the same namespace is used for all blobs

I actually think there will be a lot of use cases where people post PFBs with many different namespaces, but better yet, we have #1115

The only problem with not encoding namespace is it means we have events without namespaces, which could be annoying for indexers. Say I want to query the most popular namespaces - this now becomes super difficult.

left a comment in #1115 to link to this comment as its a good point

proto/blob/event.proto

x/blob/types/blob_tx.go

x/blob/client/cli/wirepayfordata.go

app/process_proposal.go

cmwaters · 2023-01-03T12:17:42Z

app/process_proposal.go

+		if len(msgs) != 1 {
+			continue
+		}


If it's not possible for a correct node to generate this in PrepareProposal then we should outright reject the entire proposal. Same for line 96

good point, I agree and don't see why we shouldn't change this now. e9a8014

I tried to test this, but it would take a ton of code to hit this test, as we'd basically have to write a custom share splitting method just to create an other wise valid block.

evan-forbes

addressed some feedback here, but I still plan on breaking off one refactor in a meager attempt to reduce the size of the PR.

x/blob/types/payforblob.go

app/process_proposal.go

app/estimate_square_size_test.go

evan-forbes · 2023-01-04T06:29:47Z

app/process_proposal.go

+		if len(msgs) != 1 {
+			continue
+		}


good point, I agree and don't see why we shouldn't change this now. e9a8014

I tried to test this, but it would take a ton of code to hit this test, as we'd basically have to write a custom share splitting method just to create an other wise valid block.

pkg/inclusion/get_commit.go

evan-forbes · 2023-01-04T06:36:59Z

proto/blob/tx.proto

@@ -26,16 +26,16 @@ message ShareCommitAndSignature {
 // MsgPayForBlob pays for the inclusion of a blob in the block.
 message MsgPayForBlob {
  string signer = 1;
-  bytes namespace_id = 2;
-  uint64 blob_size = 3;
+  repeated bytes namespace_ids = 2;


I actually think there will be a lot of use cases where people post PFBs with many different namespaces, but better yet, we have #1115

evan-forbes · 2023-01-04T06:40:20Z

proto/blob/tx.proto

  // with this message should use when included in a block. The share_version
  // specified must match the share_version used to generate the
  // share_commitment in this message.
-  uint32 share_version = 8;
+  repeated uint32 share_versions = 8;


there could maybe be a scenario with multiple share_versions, but we could enforce that if we wanted to. Its probably a good trade-off

x/blob/types/blob_tx.go

cmwaters · 2023-01-04T10:31:10Z

pkg/inclusion/get_commit.go

+		}
+		commitments[i] = commitment
+	}
+	return merkle.HashFromByteSlices(commitments), nil


What's the advantage of merkelizing all the commitments over plain hashing

good question, so if we have many blobs being paid for by a PFB, then there might be some scenario where you want to prove that someone paid for a specific blob without downloading the entirely blob. other than that, I'm not really sure. Using a hash would probably work fine

want to prove that someone paid for a specific blob without downloading the entirely blob

Won't you still need the blob or at least the commitment for that blob?

(I'm generally a bit hesitant about merkelizing something without a strong use case. For example, tendermint merkelizes the header which I always felt was pointless. The headers is like 400 bytes. Just download the entire thing)

another good point.

it would be good to get @nashqueue 's point of view on this as well for inclusion fraud proofs. We can discuss synchronously if necessary, as we should probably make a decision soon. I'm definitely fine with changing this to simply include all of the share commitments in the PFB instead of using a secondary commitment over all of them.

I'm fairly certain that we would have to end up downloading all of the share commitments for a single PFB if we're attempting to prove inclusion for a single one in both of the other options, those being taking a hash over all of the commitments, or including all commitments in the PFB.

If there is ever a scenario where a single PFB pays for 1000 blobs, then I could see the secondary commitment being useful, until then, probably not.

I think you make a tradeoff between fraud-proof size and DA bytes. In a way, you can see it if you have multiple commitments, you only need to show that one of them is broken. If you have only one, you need all blobs to compute the merkle hash and compare that it is wrong. If there is no limit to PFBs per Tx then the worst case fraud proof size for a single commitment fraud-proof would be O(n), with n being the number of shares in a square. This is the case if you have 1 share-sized blob per Tx. So, therefore, I would say saving all commitments is the better option here.

sounds good, that's what we'll do then

left an issue to handle after this is merged #1231

…1196) ## Overview part of #388, and split apart from #1154 this PR uses a new mechanism to generate the share commit that can create a commitment over multiple blobs. We don't yet support multiple blobs per PFB, so this is currently effectively hashing the normal share commitment. ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords

## Overview part of #388, split apart from #1154 This PR refactors enforcement of only allowing a single PFB per sdk.Tx and, per feedback in #1154, **adds a new block validity rule** where all transactions must be decodable! ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords Co-authored-by: Callum Waters <cmwaters19@gmail.com> Co-authored-by: Rootul P <rootulp@gmail.com>

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

…tions

## Overview bumps core to v1.13.0-tm-v0.34.23 blocking #1154 and #1042 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords

evan-forbes · 2023-01-14T00:33:07Z

there are still a lot of follow ups to this PR, but if there is no more feedback, I'll try to merge this PR monday

rootulp

LGTM w/ the exception of one test name

x/blob/types/payforblob_test.go

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

evan-forbes added app ABCI modifies an ABCI method consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules x/blob item is directly relevant to the blob module labels Dec 23, 2022

evan-forbes added this to the Incentivized Testnet milestone Dec 23, 2022

evan-forbes self-assigned this Dec 23, 2022

evan-forbes mentioned this pull request Dec 23, 2022

feat!: Multiple indexes per IndexWrapper celestiaorg/celestia-core#922

Merged

evan-forbes mentioned this pull request Dec 23, 2022

feat: implement non-interactive default rules for reduced padding #1152

Merged

rootulp reviewed Dec 23, 2022

View reviewed changes

cmwaters reviewed Jan 3, 2023

View reviewed changes

evan-forbes mentioned this pull request Jan 4, 2023

Add the namespace to EventPayForBlob #1169

Closed

evan-forbes commented Jan 4, 2023

View reviewed changes

cmwaters reviewed Jan 4, 2023

View reviewed changes

This was referenced Jan 9, 2023

feat!: enforce single pfb per blobtx #1195

Merged

feat!: use MultiShareCommitments instead of a single ShareCommitment #1196

Merged

chore!: refactor NewMsgPayForBlob to accept blobs #1197

Merged

evan-forbes added 8 commits January 10, 2023 23:03

fleshing out pfb refactor

722d74b

rebase: flesh out changes

bd5838b

rebase: get things to compile

bce6578

fix: tests

0f4b6b1

chore: update square estimation

cc49a74

chore: add some test cases for multiple blobs

978a4c0

fix: use shares used instead of data used

d532f36

fix: tests

d1dec5e

evan-forbes and others added 3 commits January 12, 2023 16:55

chore: spelling

b967055

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

fix: remove accidental addition

3986ea2

refactor: change validate blobs to be more consistent with other func…

64e3a39

…tions

evan-forbes mentioned this pull request Jan 13, 2023

Revert MultiShareCommitment and include each ShareCommitment in the PFB #1231

Closed

Merge branch 'main' into evan/multi-blob-PFBs

f93b941

evan-forbes mentioned this pull request Jan 13, 2023

chore: bump core to v1.13.0-tm-v0.34.23 #1232

Merged

5 tasks

MSevey removed the request for review from a team January 13, 2023 21:35

evan-forbes added 2 commits January 13, 2023 16:37

Merge branch 'main' into evan/multi-blob-PFBs

9acb5c5

fix: test

cab96e6

MSevey requested a review from a team January 13, 2023 22:39

evan-forbes mentioned this pull request Jan 14, 2023

ADR 013: Non-Interactive Default Rules for Zero Padding #1161

Merged

6 tasks

evan-forbes enabled auto-merge (squash) January 14, 2023 00:31

evan-forbes disabled auto-merge January 14, 2023 00:33

evan-forbes requested review from rootulp and cmwaters January 14, 2023 00:33

rootulp reviewed Jan 16, 2023

View reviewed changes

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

chore: testname

950554b

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

MSevey requested a review from a team January 16, 2023 17:29

merge main

96505a6

rootulp previously approved these changes Jan 16, 2023

View reviewed changes

Merge branch 'main' into evan/multi-blob-PFBs

99df848

evan-forbes dismissed rootulp’s stale review via 99df848 January 16, 2023 17:53

MSevey requested a review from a team January 16, 2023 17:53

evan-forbes enabled auto-merge (squash) January 16, 2023 18:04

evan-forbes requested a review from rootulp January 16, 2023 18:04

rootulp approved these changes Jan 16, 2023

View reviewed changes

evan-forbes merged commit 222bb1e into main Jan 16, 2023

evan-forbes deleted the evan/multi-blob-PFBs branch January 16, 2023 18:12

		@@ -26,16 +26,16 @@ message ShareCommitAndSignature {
		// MsgPayForBlob pays for the inclusion of a blob in the block.
		message MsgPayForBlob {

feat!: multiple blobs per PFB #1154

feat!: multiple blobs per PFB #1154

Conversation

evan-forbes commented Dec 23, 2022 • edited

Overview

Checklist

codecov-commenter commented Dec 23, 2022 • edited

Codecov Report

evan-forbes commented Dec 23, 2022

rootulp 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

cmwaters 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters Jan 3, 2023 • edited

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

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

nashqueue Jan 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Jan 14, 2023

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes commented Dec 23, 2022 •

edited

codecov-commenter commented Dec 23, 2022 •

edited

cmwaters Jan 3, 2023 •

edited

nashqueue Jan 12, 2023 •

edited