Decouple PartSetHeader from BlockID by evan-forbes · Pull Request #441 · celestiaorg/celestia-core

evan-forbes · 2021-06-29T02:40:25Z

Description

This PR is strictly moving the PartSetHeader outside of the BlockID. Afterwords, it should be easier to decide exactly what to do with the PartSetHeader in each scenario.

Add PartSetHeader to structs that have the BlockID
- debug at each step
Remove the PartSetHeader from BlockID
Decouple by relying on the new PartSetHeader
Fix hardcoded tests
figure out why full02 is absent during the e2e tests

Part 1/? to help remove the PartSetHeader #418
Closes: #XXX

evan-forbes · 2021-07-02T01:01:02Z

Besides the hardcoded tests, only the e2e test is failing. The logs show that all the nodes are working fine except full02, which doesn't show up on the e2e manifest. I'm still investigating why exactly, but the only difference between full01, which works fine, and full02 is that full02 only connects to the seed node while full01 has multiple persistent peers.

edit: resolved 4f064db

I just missed passing the partsetheader there was all

evan-forbes

I tried to be careful to not actually change any functionality, and strictly decouple partsetheader from the blockID.

The main breaking change is that the Key method for BlockID no longer returns a string that includes the PartSetHeader's hash and total.
https://github.com/celestiaorg/lazyledger-core/blob/f639bee4e2219ad11ee3f7efc15d77325cbb71a8/types/block.go#L1616-L1619

Ideally, most of the additions of the PartSetHeader introduced in this PR will be undone. I think this is a great starting point to begin removing the PartSetHeader, as we can now test smaller removals individually. We could also get rid of the BlockID struct, and just use []byte or tmbytes.HexBytes, but I felt like there were enough changes in this PR as is.

types/vote_set_test.go

proto/tendermint/consensus/types.proto

Wondertan

Left some nits and refs to todos not be missed. Mostly, I have nothing valuable to add as the change is fairly simple, even though it affects the entire codebase. Another step further, thanks @evan-forbes!

consensus/reactor.go

state/helpers_test.go

state/tx_filter_test.go

store/store.go

types/events.go

types/vote.go

types/vote_set_test.go

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

liamsi · 2021-07-11T11:43:08Z

consensus/common_test.go

+		BlockID:          types.BlockID{Hash: hash},
+		PartSetHeader:    header,
 	}
 	v := vote.ToProto()
 	err = vs.PrivValidator.SignVote(config.ChainID(), v)


Question: Wasn't the conclusion that at the voting stage we don't need to sign over the PartsSetHeader anymore? It's probably a good idea to leave it as is in this PR and focus on the decoupling but it seems important to notice as we at least want the commits not be over the PartSetHeader (otherwise verifying a commit sign still means verifying two commitments to the data). We should track this in an issue too.
cc @adlerjohn

Right, you don't want any signatures that end up in the Commit to be over the PartSetHeader (or its hash), otherwise you'll have this dangling hash that can't be validated. There's an open issue for specifying what's in the commit signatures: celestiaorg/celestia-specs#92, which is now relevant.

definitely, I can open an issue for it. Due to the large number of changes necessary to decouple the PSH from the BlockID, I purposefully left this in. The idea being that we can make important changes in a more targeted PRs.

codecov-commenter · 2021-07-11T15:10:44Z

Codecov Report

Merging #441 (daf469a) into master (8da1644) will decrease coverage by 0.11%.
The diff coverage is 74.91%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
- Coverage   61.76%   61.65%   -0.12%     
==========================================
  Files         263      263              
  Lines       23003    23119     +116     
==========================================
+ Hits        14207    14253      +46     
- Misses       7292     7343      +51     
- Partials     1504     1523      +19

Impacted Files	Coverage Δ
consensus/types/height_vote_set.go	`31.85% <0.00%> (ø)`
consensus/types/round_state.go	`0.00% <0.00%> (ø)`
light/verifier.go	`69.23% <ø> (ø)`
statesync/stateprovider.go	`0.00% <0.00%> (ø)`
store/store.go	`65.08% <ø> (ø)`
types/events.go	`100.00% <ø> (ø)`
types/protobuf.go	`50.94% <0.00%> (-0.98%)`	⬇️
types/validator_set.go	`88.70% <25.00%> (-1.30%)`	⬇️
types/block_meta.go	`62.74% <47.36%> (-3.93%)`	⬇️
types/test_util.go	`39.53% <50.00%> (+0.51%)`	⬆️
... and 35 more

liamsi

Dope! Would you be up to summarize the rationale behind this (just really briefly) in an ADR? I'm also wondering if we should try to upstream this to tendermint (in combination with some modified version of #457 without the DAHeader).

cc @marbar3778

liamsi · 2021-07-18T19:26:56Z

types/proposal.go

+	DAHeader      *DataAvailabilityHeader `json:"da_header"`
+	PartSetHeader PartSetHeader           `json:"part_set_header"`


Can we add a comment ontop of both fields why both are needed here (at this stage)?

This reverts commit 9d4265d.

* Revert "Remove the PartSetHeader from VoteSetMaj23, VoteSetBits, and state.State (#479)" This reverts commit 3ba47c2. * Revert "Stop Signing over the `PartSetHeader` for Votes and remove it from the `Header` (#457)" This reverts commit 818be04. * Revert "Decouple PartSetHeader from BlockID (#441)" This reverts commit 9d4265d.

* Revert "Remove the PartSetHeader from VoteSetMaj23, VoteSetBits, and state.State (#479)" This reverts commit 3ba47c2. * Revert "Stop Signing over the `PartSetHeader` for Votes and remove it from the `Header` (#457)" This reverts commit 818be04. * Revert "Decouple PartSetHeader from BlockID (#441)" This reverts commit 9d4265d. * Revert "Save and load block data using IPFS (#374)" This reverts commit 8da1644. * fix merge error * Revert "Add the ipfs dag api object in Blockstore (#356)" and get rid of embedded ipfs objects This reverts commit 40acb17. * remove ipfs node provider from new node * stop init ipfs repos * remove IPFS node config * clean up remaining ipfs stuff

* Update go-built-in.md (#438) (cherry picked from commit 06eeaaa) # Conflicts: # docs/tutorials/go-built-in.md * Revert "Update go-built-in.md (#438)" This reverts commit b197c0883dad94b03d80aba709798e6a80ba103f. * Update go-built-in.md (#438) --------- Co-authored-by: Aliasgar Merchant <44069404+alijnmerchant21@users.noreply.github.com> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Adi Seredinschi <adizere@gmail.com> Co-authored-by: lasarojc <lasaro@informal.systems>

evan-forbes self-assigned this Jun 29, 2021

evan-forbes added 7 commits June 29, 2021 14:10

add partsetheader to block meta fields

aa91af7

add the partsetheader to the proposal

a6da1c5

add partsetheader to Commit

8fc605d

add the partsetheader to vote

8673502

add the partsetheader to Header

6d2178e

add the partsetheader to the state

f218d7d

begin relying on decoupled partsetheader

d3c1e7d

evan-forbes force-pushed the evan/ismail/explore_blockid_partsetheader branch from 6c2ba43 to d3c1e7d Compare June 30, 2021 01:57

This comment has been minimized.

Sign in to view

evan-forbes added 6 commits June 30, 2021 13:58

fix TestHeaderHash by updating expected hash

934077a

fix all tests in the types package

afc874b

fix all tests in state

ce27a00

fix prival tests

574c63d

fix p2p tests

528b336

get evidence, light and consensus to compile

0edea27

This comment has been minimized.

Sign in to view

liamsi mentioned this pull request Jul 1, 2021

Add the Data Availability Header to BlockID #312

Closed

5 tasks

evan-forbes added 2 commits July 1, 2021 15:18

add partset header to and from proto for consensus messages

185eeef

revert blockid.Key requiring the partsetheader

c15f05e

evan-forbes added 4 commits July 2, 2021 11:19

fix e2e by including missed lastpartsetheader to state

4f064db

fix more hardcoded tests

10b9644

linter

f639bee

clean up todos

a63c633

evan-forbes commented Jul 2, 2021

View reviewed changes

types/vote_set_test.go Outdated Show resolved Hide resolved

proto/tendermint/consensus/types.proto Show resolved Hide resolved

evan-forbes marked this pull request as ready for review July 2, 2021 18:14

evan-forbes requested review from Wondertan and liamsi as code owners July 2, 2021 18:14

celestiaorg deleted a comment from lgtm-com bot Jul 2, 2021

Wondertan reviewed Jul 9, 2021

View reviewed changes

fix PartSetHeader field in EventDAtaCompleteProposal

72e985a

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

liamsi reviewed Jul 11, 2021

View reviewed changes

This was referenced Jul 11, 2021

Don't sign over the PartSetHeader or include it in the Header #456

Closed

Remove the PartSetHeader where possible #418

Closed

remove unused tests

daf469a

evan-forbes mentioned this pull request Jul 14, 2021

Stop Signing over the PartSetHeader for Votes and remove it from the Header #457

Merged

liamsi approved these changes Jul 19, 2021

View reviewed changes

evan-forbes merged commit 9d4265d into master Jul 25, 2021

evan-forbes deleted the evan/ismail/explore_blockid_partsetheader branch July 25, 2021 16:43

evan-forbes mentioned this pull request Aug 2, 2021

Document already made Decisions to Decouple Blockid and PartSetHeader in ADR 005 #482

Merged

evan-forbes added a commit that referenced this pull request Aug 23, 2021

Revert "Decouple PartSetHeader from BlockID (#441)"

dbf5418

This reverts commit 9d4265d.

This was referenced Aug 23, 2021

Revert changes to the BlockID and PartSetHeader #509

Closed

Revert changes to the PartSetHeader and BlockID #510

Merged

Proposal: Remove the PartSetHeader from the Header and don't Sign over it when Voting #511

Closed

		DAHeader *DataAvailabilityHeader `json:"da_header"`
		PartSetHeader PartSetHeader `json:"part_set_header"`

Conversation

evan-forbes commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

This comment has been minimized.

This comment has been minimized.

evan-forbes commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evan-forbes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Wondertan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liamsi Jul 11, 2021

Choose a reason for hiding this comment

Uh oh!

adlerjohn Jul 11, 2021

Choose a reason for hiding this comment

Uh oh!

evan-forbes Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evan-forbes Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

liamsi Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

evan-forbes commented Jun 29, 2021 •

edited

Loading

evan-forbes commented Jul 2, 2021 •

edited

Loading

evan-forbes left a comment •

edited

Loading

evan-forbes Jul 11, 2021 •

edited

Loading

codecov-commenter commented Jul 11, 2021 •

edited

Loading