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!: write PFB txs to their own namespace #1228

Merged
merged 14 commits into from
Jan 18, 2023
Merged

feat!: write PFB txs to their own namespace #1228

merged 14 commits into from
Jan 18, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jan 12, 2023 •
edited

Closes #1173, #1237
Opens #1243

@rootulp rootulp self-assigned this Jan 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2023 •
edited

Codecov Report

Merging #1228 (fcdffb1) into main (a8c8849) will increase coverage by 0.06%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main    #1228      +/-   ##
==========================================
+ Coverage   48.77%   48.84%   +0.06%     
==========================================
  Files          75       75              
  Lines        4340     4369      +29     
==========================================
+ Hits         2117     2134      +17     
- Misses       2043     2055      +12     
  Partials      180      180
Impacted Files Coverage Δ
app/process_proposal.go 0.00% <0.00%> (ø)
app/estimate_square_size.go 89.83% <100.00%> (+1.36%) ⬆️
app/parse_txs.go 57.14% <100.00%> (+5.52%) ⬆️
pkg/prove/proof.go 75.94% <100.00%> (+0.10%) ⬆️
pkg/shares/share_splitting.go 73.25% <100.00%> (+1.65%) ⬆️

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

@evan-forbes evan-forbes added consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules warn:api breaking item will be break an API and require a major bump labels Jan 12, 2023
@evan-forbes evan-forbes added this to the Incentivized Testnet milestone Jan 12, 2023
@rootulp rootulp mentioned this pull request Jan 16, 2023
Closed
@rootulp rootulp marked this pull request as ready for review January 16, 2023 17:00
@MSevey MSevey requested review from a team and staheri14 and removed request for a team January 16, 2023 20:50
@MSevey MSevey requested a review from a team January 16, 2023 20:55
evan-forbes
evan-forbes previously approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice, LGTM! :shipit: everything makes sense to me and is about exactly what I expected change wise. lets let the relevant outside teams know of this change sometime after we merge

just to clarify, we won't actually be able to parse the square back into a block until #1230, correct? I know no one does that atm, but just want to clarify

app/estimate_square_size_test.go Outdated Show resolved Hide resolved
pkg/shares/share_splitting.go
@@ -63,9 +63,10 @@ func Split(data coretypes.Data, useShareIndexes bool) ([]Share, error) {
currentShareCount += len(blobShares)
tailShares := TailPaddingShares(wantShareCount - currentShareCount)
shares := make([]Share, 0, data.SquareSize*data.SquareSize)
shares = append(append(append(append(
shares = append(append(append(append(append(
Copy link
Member

Choose a reason for hiding this comment

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

note to self that we should create a generic helper like appendAll or something to get rid of alllllll the giant appends we have

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea or alternatively:

	shares := make([]Share, 0, data.SquareSize*data.SquareSize)
	shares = append(shares, txShares...)
	shares = append(shares, pfbTxShares...)
	shares = append(shares, padding...)
	shares = append(shares, blobShares...)
	shares = append(shares, tailShares...)

I don't have a strong preference. Part of me thinks that since Go doesn't have a built-in appendAll it may be more idiomatic to use multiple append() invocations

Copy link
Member

Choose a reason for hiding this comment

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

sure that works too

@MSevey MSevey requested a review from a team January 17, 2023 16:40
@rootulp
Copy link
Collaborator Author

rootulp commented Jan 17, 2023

just to clarify, we won't actually be able to parse the square back into a block until #1230, correct? I know no one does that atm, but just want to clarify

Correct. It seems like a desirable property to go from data square => block so I think we should explore that issue after this is merged.

@rootulp
Copy link
Collaborator Author

rootulp commented Jan 17, 2023

I was a little :suspect: when I saw unit tests pass but I'm glad the integ test failed

--- FAIL: TestIntegrationTestSuite (77.94s)
    integration_test.go:61: setting up integration test suite
    network.go:218: acquiring test network lock
    network.go:228: preparing test network with chain-id "chain-lmhytD"
    network.go:511: starting test network...
    network.go:517: started validator 0
    network.go:525: started test network at height: 35
    --- FAIL: TestIntegrationTestSuite/TestShareInclusionProof (20.04s)
        integration_test.go:353:
            	Error Trace:	/Users/rootulp/git/rootulp/celestia/celestia-app/app/test/integration_test.go:353
            	Error:      	Should be true
            	Test:       	TestIntegrationTestSuite/TestShareInclusionProof
    integration_test.go:83: tearing down integration test suite
    network.go:614: cleaning up test network...
    network.go:641: finished cleaning up test network
    network.go:611: released test network lock

@rootulp
Revert "wip: extract transactions package"
e424cef 
This reverts commit 34f8fc4.
@rootulp
test: remove tx proof and tx share position
e372b11 
TxSharePosition is not expected to work prior to this PR and definitely
won't work after this PR. Remove the tx share position test from the
integ test to unblock this PR.
@evan-forbes evan-forbes mentioned this pull request Jan 18, 2023
Open
@rootulp rootulp merged commit b6e2651 into celestiaorg:main Jan 18, 2023
@rootulp rootulp deleted the rp/pay-for-blob-namespace branch January 18, 2023 20:32
@rootulp rootulp mentioned this pull request Jan 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

@staheri14 staheri14 Awaiting requested review from staheri14 staheri14 is a code owner automatically assigned from celestiaorg/celestia-core

Assignees

@rootulp rootulp

Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules warn:api breaking item will be break an API and require a major bump
Projects
None yet
Milestone
Incentivized Testnet
Development

Successfully merging this pull request may close these issues.

Put all PFBs in a reserved namespace
3 participants
@rootulp @codecov-commenter @evan-forbes