fix: setting data root in CreateProposalBlock #1271

cmwaters · 2024-03-20T10:57:31Z

Currently main fails when using celestia-app because the data root that is returned to it wasn't being correctly updated in the header.

This PR fixes this

abci/types/application.go

types/block.go

rootulp

Can a unit test verify this in celestia-core or is it only easily test-able from celestia-app?

cmwaters · 2024-03-21T10:48:45Z

Can a unit test verify this in celestia-core or is it only easily test-able from celestia-app?

I think we could have a unit test on this. At least to catch future regressions

staheri14 · 2024-03-21T23:54:19Z

state/execution.go

@@ -145,10 +144,9 @@ func (blockExec *BlockExecutor) CreateProposalBlock(
 	}

 	// update the block with the response from PrepareProposal
-	block.Data, _ = types.DataFromProto(&cmtproto.Data{
-		Txs:  rpp.Txs[:len(rpp.Txs)-1],
-		Hash: rpp.Txs[len(rpp.Txs)-1],


The Hash is left unset, is it intentional?

Yes. We could probably remove it from the Data struct if we wanted to

So, wouldn't that be a breaking behaviour, namely it has been previously set and with the new change it is not?

Yes that would be breaking, but it just breaks celestia-app, not downstream users

Actually going to revert this because I don't think it makes sense to break app just yet

rootulp

LGTM. As a follow up, should we consider adding to the specs the ABCI change that moved the data root from a field in the prepare / process proposal response to the last transaction in the list of txs? It looks like we don't currently have specs for our flavor of ABCI in https://celestiaorg.github.io/celestia-app/ so I'm not sure where this addition belongs.

Related: celestiaorg/celestia-app#1066

evan-forbes

[blocking question]

does this break the PartSetHeader's hash? if so, is this going to be backported / included on v0.34.x? or by main, did you mean celestia-core's main? If it is going to backported, and it does break the PartSetHeader's hash, what effect will that have on the upgrade? perhaps a better question is simply, when are we planning on celestia-app using celestia-core main?

cmwaters · 2024-04-08T08:49:04Z

does this break the PartSetHeader's hash?

I don't think so

perhaps a better question is simply, when are we planning on celestia-app using celestia-core main

Most likely for v3

cmwaters · 2024-04-08T09:48:25Z

TestByzantineConflictingProposalsWithPartition is failing in a way that I have no idea how it's connected to the changes

cmwaters · 2024-04-08T11:55:17Z

What seems to be happening is the following:

We have 4 validators, validator 0 is byzaninte, the remaining are divided into two partitions: Validator 1 in partition A and validator 2 and 3 in partition B.

Byzantine validator proposes two different proposals: Block A to partition A and Block B to partition B. It also sends prevotes to both.
Partition B reaches 2/3 and eventually produces a block
Network is healed (i.e. validator 1 can talk directly to validators 2 and 3)
Validator 1 receives 2/3 commits for B and tries to commit B. It sees that the proposal block parts it has (A) doesn't match what the 2/3 committed for (B) and panics

In the tests on main, it for some reason has nil for it's proposal block part so it prints something like:

commit is for a block we do not know about; set ProposalBlock=nil module=consensus validator=1 height=1 commit_round=0 proposal=343D64D40989F3C8A1CCB73D52AE0DAE8EEA1D51EB0443058DA8D9F84701D004 commit=C40D8D242C29FDB14E7B97A9D7C19751D980FF482AF13939BAC305906AF29553

It then sets it's block part header to B and eventually receives B and commits it.

evan-forbes · 2024-04-08T12:08:31Z

does this break the PartSetHeader's hash?

I could be wrong, but it appears to since it removes the square size, which we're still including atm

cmwaters · 2024-04-17T15:15:01Z

does this break the PartSetHeader's hash?

I could be wrong, but it appears to since it removes the square size, which we're still including atm

I'm actually not sure that square size is populated in data (and in the block)

celestia-core/types/block.go

Line 1104 in bf68e4f

func DataFromProto(dp *cmtproto.Data) (Data, error) {

cmwaters · 2024-04-17T17:07:09Z

abci/types/application.go

@@ -96,7 +96,7 @@ func (BaseApplication) ApplySnapshotChunk(req RequestApplySnapshotChunk) Respons

 func (BaseApplication) PrepareProposal(req RequestPrepareProposal) ResponsePrepareProposal {
 	// we use placeholder values for the hash
-	req.Txs = append(req.Txs, tmhash.Sum(nil))
+	req.Txs = append(req.Txs, merkle.HashFromByteSlices(req.Txs))


This is what was causing the test to fail. We were using idential hashes so the blockID.Hash was the same but blockID.PartSetHeader was different

evan-forbes · 2024-04-17T17:12:18Z

I'm actually not sure that square size is populated in data (and in the block)

unfortunately for us, we are on v0.34.x-celestia 😭 this was actually the reason why the compact blocks implementation didn't work in testground but did work in the comet e2e tests here

celestia-core/types/block.go

Lines 1097 to 1122 in d3ff8e7

    
           	tp.SquareSize = data.SquareSize 
        
           	tp.Hash = data.hash 
        
           	return *tp 
        
           } 
        
           // DataFromProto takes a protobuf representation of Data & 
        
           // returns the native type. 
        
           func DataFromProto(dp *cmtproto.Data) (Data, error) { 
        
           	if dp == nil { 
        
           		return Data{}, errors.New("nil data") 
        
           	} 
        
           	data := new(Data) 
        
           	if len(dp.Txs) > 0 { 
        
           		txBzs := make(Txs, len(dp.Txs)) 
        
           		for i := range dp.Txs { 
        
           			txBzs[i] = Tx(dp.Txs[i]) 
        
           		} 
        
           		data.Txs = txBzs 
        
           	} else { 
        
           		data.Txs = Txs{} 
        
           	} 
        
           	data.SquareSize = dp.SquareSize

we forgot to remove the square size, which makes upgrading to not using it more complicated than it needed to

cmwaters · 2024-04-17T17:33:32Z

Ah you're right. I was looking at main but I thought I was looking at v1.x. We can revert it and keep it around a bit longer.

rootulp

LGTM. I still think a unit test to verify the fix / prevent regressions would help but can add afterwards

consensus/byzantine_test.go

state/execution.go

evan-forbes

do we need to update

celestia-core/version/version.go

Lines 20 to 21 in d76e844

    
           	BlockProtocol uint64 = 11 
        
           )

?

cmwaters · 2024-04-19T09:24:08Z

do we need to update

celestia-core/version/version.go

Lines 20 to 21 in d76e844

BlockProtocol uint64 = 11

)

?

I don't think we have any block format changes since I reverted the protos to match v0.34.x-celestia

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

fix: setting data root in CreateProposalBlock

e4bfdb8

cmwaters requested review from evan-forbes, staheri14, rach-id, ninabarbakadze and rootulp as code owners March 20, 2024 10:57

cmwaters self-assigned this Mar 20, 2024

make the fix more succinct

0caaeeb

rootulp previously approved these changes Mar 20, 2024

View reviewed changes

fix e2e tests take 1

dbfa70b

cmwaters dismissed rootulp’s stale review via dbfa70b March 20, 2024 14:37

remove unused square and failing line in test

9be6eb2

rootulp previously approved these changes Mar 20, 2024

View reviewed changes

abci/types/application.go Show resolved Hide resolved

types/block.go Show resolved Hide resolved

update data go doc

bc381f8

cmwaters dismissed rootulp’s stale review via bc381f8 March 20, 2024 15:12

rootulp previously approved these changes Mar 20, 2024

View reviewed changes

staheri14 reviewed Mar 21, 2024

View reviewed changes

remove the hash from the data proto type

4e50b0c

cmwaters dismissed rootulp’s stale review via 4e50b0c March 25, 2024 13:08

rootulp reviewed Mar 25, 2024

View reviewed changes

rootulp previously approved these changes Mar 25, 2024

View reviewed changes

evan-forbes reviewed Mar 26, 2024

View reviewed changes

cmwaters marked this pull request as draft April 8, 2024 16:43

fix failing test

7bfa1c4

cmwaters dismissed rootulp’s stale review via 7bfa1c4 April 17, 2024 17:04

Merge branch 'main' into cal/fix-setting-data-root

2a36b9b

cmwaters marked this pull request as ready for review April 17, 2024 17:05

cmwaters requested a review from a team as a code owner April 17, 2024 17:05

cmwaters commented Apr 17, 2024

View reviewed changes

cmwaters requested review from staheri14, evan-forbes and rootulp April 17, 2024 17:07

Bring back the square size

0c4666e

rootulp previously approved these changes Apr 18, 2024

View reviewed changes

consensus/byzantine_test.go Outdated Show resolved Hide resolved

state/execution.go Outdated Show resolved Hide resolved

evan-forbes previously approved these changes Apr 18, 2024

View reviewed changes

Apply suggestions from code review

df3328b

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

cmwaters dismissed stale reviews from evan-forbes and rootulp via df3328b April 19, 2024 09:25

cmwaters requested review from rootulp and evan-forbes April 19, 2024 09:26

evan-forbes approved these changes Apr 20, 2024

View reviewed changes

rootulp approved these changes Apr 22, 2024

View reviewed changes

Merge branch 'main' into cal/fix-setting-data-root

2d21196

cmwaters enabled auto-merge (squash) April 23, 2024 12:45

Merge branch 'main' into cal/fix-setting-data-root

4ff558d

cmwaters merged commit 9ba3fac into main Apr 23, 2024
19 checks passed

cmwaters deleted the cal/fix-setting-data-root branch April 23, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: setting data root in CreateProposalBlock #1271

fix: setting data root in CreateProposalBlock #1271

cmwaters commented Mar 20, 2024

rootulp left a comment

cmwaters commented Mar 21, 2024

staheri14 Mar 21, 2024

cmwaters Mar 25, 2024

staheri14 Mar 26, 2024

cmwaters Apr 8, 2024

cmwaters Apr 18, 2024

rootulp left a comment

evan-forbes left a comment •

edited

Loading

cmwaters commented Apr 8, 2024

cmwaters commented Apr 8, 2024 •

edited

Loading

cmwaters commented Apr 8, 2024

evan-forbes commented Apr 8, 2024 •

edited

Loading

cmwaters commented Apr 17, 2024 •

edited

Loading

cmwaters Apr 17, 2024

evan-forbes commented Apr 17, 2024

cmwaters commented Apr 17, 2024

rootulp left a comment

evan-forbes left a comment

cmwaters commented Apr 19, 2024

fix: setting data root in CreateProposalBlock #1271

fix: setting data root in CreateProposalBlock #1271

Conversation

cmwaters commented Mar 20, 2024

rootulp left a comment

Choose a reason for hiding this comment

cmwaters commented Mar 21, 2024

staheri14 Mar 21, 2024

Choose a reason for hiding this comment

cmwaters Mar 25, 2024

Choose a reason for hiding this comment

staheri14 Mar 26, 2024

Choose a reason for hiding this comment

cmwaters Apr 8, 2024

Choose a reason for hiding this comment

cmwaters Apr 18, 2024

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

cmwaters commented Apr 8, 2024

cmwaters commented Apr 8, 2024 • edited Loading

cmwaters commented Apr 8, 2024

evan-forbes commented Apr 8, 2024 • edited Loading

cmwaters commented Apr 17, 2024 • edited Loading

cmwaters Apr 17, 2024

Choose a reason for hiding this comment

evan-forbes commented Apr 17, 2024

cmwaters commented Apr 17, 2024

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

cmwaters commented Apr 19, 2024

evan-forbes left a comment •

edited

Loading

cmwaters commented Apr 8, 2024 •

edited

Loading

evan-forbes commented Apr 8, 2024 •

edited

Loading

cmwaters commented Apr 17, 2024 •

edited

Loading