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.

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

fix(abci): use correct hash in abci block.LastCommit.BlockID #1320

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion block/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func (e *Executor) CreateBlock(
}

block.Header.SetDymHeader(types.MakeDymHeader(block.Data.ConsensusMessages))
copy(block.Header.LastCommitHash[:], types.GetLastCommitHash(lastCommit, &block.Header))
copy(block.Header.LastCommitHash[:], types.GetLastCommitHash(lastCommit))
copy(block.Header.DataHash[:], types.GetDataHash(block))
copy(block.Header.SequencerHash[:], state.GetProposerHash())
copy(block.Header.NextSequencersHash[:], nextSeqHash[:])
Expand Down
2 changes: 1 addition & 1 deletion rpc/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ func (c *Client) Commit(ctx context.Context, height *int64) (*ctypes.ResultCommi
if err != nil {
return nil, err
}
commit := types.ToABCICommit(com, &b.Header)
commit := types.ToABCICommit(com)
block, err := types.ToABCIBlock(b)
if err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions testutil/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func GenerateBlocks(startHeight uint64, num uint64, proposerKey crypto.PrivKey,
block := generateBlock(i+startHeight, proposerHash, lastHeaderHash)
copy(block.Header.DataHash[:], types.GetDataHash(block))
if i > 0 {
copy(block.Header.LastCommitHash[:], types.GetLastCommitHash(&blocks[i-1].LastCommit, &block.Header))
copy(block.Header.LastCommitHash[:], types.GetLastCommitHash(&blocks[i-1].LastCommit))
}

signature, err := generateSignature(proposerKey, &block.Header)
Expand Down Expand Up @@ -268,7 +268,7 @@ func GenerateLastBlocks(startHeight uint64, num uint64, proposerKey crypto.PrivK

copy(block.Header.DataHash[:], types.GetDataHash(block))
if i > 0 {
copy(block.Header.LastCommitHash[:], types.GetLastCommitHash(&blocks[i-1].LastCommit, &block.Header))
copy(block.Header.LastCommitHash[:], types.GetLastCommitHash(&blocks[i-1].LastCommit))
}

signature, err := generateSignature(proposerKey, &block.Header)
Expand Down
5 changes: 3 additions & 2 deletions types/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,9 @@ type IntermediateStateRoots struct {
RawRootsList [][]byte
}

func GetLastCommitHash(lastCommit *Commit, header *Header) []byte {
lastABCICommit := ToABCICommit(lastCommit, header)
func GetLastCommitHash(lastCommit *Commit) []byte {
lastABCICommit := ToABCICommit(lastCommit)
// Note: hash only depends on the signature
return lastABCICommit.Hash()
}

Expand Down
36 changes: 8 additions & 28 deletions types/conv.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,13 @@ func ToABCIHeader(header *Header) tmtypes.Header {
// Returned block should pass `ValidateBasic`.
func ToABCIBlock(block *Block) (*tmtypes.Block, error) {
abciHeader := ToABCIHeader(&block.Header)
abciCommit := ToABCICommit(&block.LastCommit, &block.Header)
// This assumes that we have only one signature
if len(abciCommit.Signatures) == 1 {
abciCommit.Signatures[0].ValidatorAddress = block.Header.ProposerAddress
}
abciLastCommit := ToABCICommit(&block.LastCommit)
abciBlock := tmtypes.Block{
Header: abciHeader,
Evidence: tmtypes.EvidenceData{
Evidence: nil,
},
LastCommit: abciCommit,
LastCommit: abciLastCommit,
}
abciBlock.Data.Txs = ToABCIBlockDataTxs(&block.Data)
abciBlock.Header.DataHash = block.Header.DataHash[:]
Expand Down Expand Up @@ -96,36 +92,20 @@ func ToABCIBlockMeta(block *Block) (*tmtypes.BlockMeta, error) {
// ToABCICommit converts Dymint commit into commit format defined by ABCI.
// This function only converts fields that are available in Dymint commit.
// Other fields (especially ValidatorAddress and Timestamp of Signature) has to be filled by caller.
func ToABCICommit(commit *Commit, header *Header) *tmtypes.Commit {
headerHash := header.Hash()
func ToABCICommit(commit *Commit) *tmtypes.Commit {
tmCommit := tmtypes.Commit{
Height: int64(commit.Height), //nolint:gosec // height is non-negative and falls in int64
Round: 0,
BlockID: tmtypes.BlockID{
Hash: headerHash[:],
Hash: commit.HeaderHash[:],
PartSetHeader: tmtypes.PartSetHeader{
Total: 1,
Hash: headerHash[:],
Hash: commit.HeaderHash[:],
},
},
}
// Check if TMSignature exists. if not use the previous dymint signature for backwards compatibility.
if len(commit.TMSignature.Signature) == 0 {
for _, sig := range commit.Signatures {
commitSig := tmtypes.CommitSig{
BlockIDFlag: tmtypes.BlockIDFlagCommit,
Signature: sig,
}
tmCommit.Signatures = append(tmCommit.Signatures, commitSig)
}
// This assumes that we have only one signature
if len(commit.Signatures) == 1 {
tmCommit.Signatures[0].ValidatorAddress = header.ProposerAddress
tmCommit.Signatures[0].Timestamp = header.GetTimestamp()
}
} else {
tmCommit.Signatures = append(tmCommit.Signatures, commit.TMSignature)
}

// Very old commits may not have a TM signature already. Should run a prior version of
// dymint to handle them.
tmCommit.Signatures = append(tmCommit.Signatures, commit.TMSignature)
return &tmCommit
Comment on lines +107 to 110
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to remove this, since

  • I guess the only blocks without the signatures are very old? Are there even any on mainnet?
  • Nowadays you need to be running the right dymint to process old blocks anyway

}
Loading