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

fix: setting data root in CreateProposalBlock #1271

Merged
merged 12 commits into from
Apr 23, 2024
Merged

fix: setting data root in CreateProposalBlock #1271

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e4bfdb8
fix: setting data root in CreateProposalBlock
cmwaters Mar 20, 2024
0caaeeb
make the fix more succinct
cmwaters Mar 20, 2024
dbfa70b
fix e2e tests take 1
cmwaters Mar 20, 2024
9be6eb2
remove unused square and failing line in test
cmwaters Mar 20, 2024
bc381f8
update data go doc
cmwaters Mar 20, 2024
4e50b0c
remove the hash from the data proto type
cmwaters Mar 25, 2024
7bfa1c4
fix failing test
cmwaters Apr 17, 2024
2a36b9b
Merge branch 'main' into cal/fix-setting-data-root
cmwaters Apr 17, 2024
0c4666e
Bring back the square size
cmwaters Apr 18, 2024
df3328b
Apply suggestions from code review
cmwaters Apr 19, 2024
2d21196
Merge branch 'main' into cal/fix-setting-data-root
cmwaters Apr 23, 2024
4ff558d
Merge branch 'main' into cal/fix-setting-data-root
cmwaters Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions abci/types/application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package types
import (
"context"

"github.com/cometbft/cometbft/crypto/tmhash"
"github.com/cometbft/cometbft/crypto/merkle"
)

// Application is an interface that enables any finite, deterministic state machine
Expand Down Expand Up @@ -96,7 +96,7 @@ func (BaseApplication) ApplySnapshotChunk(req RequestApplySnapshotChunk) Respons

func (BaseApplication) PrepareProposal(req RequestPrepareProposal) ResponsePrepareProposal {
// we use placeholder values for the hash
rootulp marked this conversation as resolved.
Show resolved Hide resolved
req.Txs = append(req.Txs, tmhash.Sum(nil))
req.Txs = append(req.Txs, merkle.HashFromByteSlices(req.Txs))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return ResponsePrepareProposal{Txs: req.Txs}
}

Expand Down
8 changes: 3 additions & 5 deletions state/execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/cometbft/cometbft/libs/log"
mempl "github.com/cometbft/cometbft/mempool"
cmtstate "github.com/cometbft/cometbft/proto/tendermint/state"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/cometbft/cometbft/proxy"
"github.com/cometbft/cometbft/types"
)
Expand Down Expand Up @@ -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],
Copy link
Contributor

Choose a reason for hiding this comment

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

The Hash is left unset, is it intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

})
block.Data.Txs = types.ToTxs(rpp.Txs[:len(rpp.Txs)-1])
// update the data hash with the one passed back by celestia-app
block.DataHash = rpp.Txs[len(rpp.Txs)-1]

var blockDataSize int
for _, tx := range block.Txs {
Expand Down
Loading