Skip to content

fix: change proof functions to return values instead of pointers #2358

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

fix: change proof functions to return values instead of pointers #2358

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d1e1f8
Initial plan
Copilot Aug 20, 2025
a1e2e2c
fix: change proof functions to return values instead of pointers
Copilot Aug 20, 2025
47f748c
fix: address review comments - change more proof types to values
Copilot Aug 20, 2025
d0cb4b4
fix: optimize slice allocation to avoid reallocation
Copilot Sep 1, 2025
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
10 changes: 7 additions & 3 deletions consensus/propagation/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,14 +209,18 @@ func (c *CompactBlock) Proofs() ([]*merkle.Proof, error) {
c.proofsCache = make([]*merkle.Proof, 0, len(c.PartsHashes))
Copy link
Member

@tzdybal tzdybal Aug 20, 2025

Choose a reason for hiding this comment

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

As you're changing everything to reduce pointers, you might as well change proofsCache to []merkle.Proof instead of changing loops below.

Copy link
Contributor Author

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

Fixed in 47f748c. Changed proofsCache field from []*merkle.Proof to []merkle.Proof and updated the loops to directly assign values instead of taking addresses.


root, proofs := merkle.ProofsFromLeafHashes(c.PartsHashes[:total])
c.proofsCache = append(c.proofsCache, proofs...)
for i := range proofs {
c.proofsCache = append(c.proofsCache, &proofs[i])
}

if !bytes.Equal(root, c.Proposal.BlockID.PartSetHeader.Hash) {
return c.proofsCache, fmt.Errorf("incorrect PartsHash: original root")
}

parityRoot, eproofs := merkle.ProofsFromLeafHashes(c.PartsHashes[total:])
c.proofsCache = append(c.proofsCache, eproofs...)
for i := range eproofs {
c.proofsCache = append(c.proofsCache, &eproofs[i])
}

if !bytes.Equal(c.BpHash, parityRoot) {
return c.proofsCache, fmt.Errorf("incorrect PartsHash: parity root")
Expand Down Expand Up @@ -483,7 +487,7 @@ func RecoveryPartFromProto(r *protoprop.RecoveryPart) (*RecoveryPart, error) {
Round: r.Round,
Index: r.Index,
Data: r.Data,
Proof: proof,
Proof: &proof,
Copy link
Member

@tzdybal tzdybal Aug 20, 2025

Choose a reason for hiding this comment

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

Can RecoveryPart.Proof also be of type Proof and not *Proof? analyze usages to see if it's viable option.

Copy link
Contributor Author

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

Fixed in 47f748c. Changed RecoveryPart.Proof from *merkle.Proof to merkle.Proof. Analyzed all usages and updated the validation logic to check for zero value instead of nil, and updated all call sites including tests.

}
return rp, rp.ValidateBasic()
}
Expand Down
30 changes: 15 additions & 15 deletions crypto/merkle/proof.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ type Proof struct {

// ProofsFromByteSlices computes inclusion proof for given items.
// proofs[0] is the proof for items[0].
func ProofsFromByteSlices(items [][]byte) (rootHash []byte, proofs []*Proof) {
func ProofsFromByteSlices(items [][]byte) (rootHash []byte, proofs []Proof) {
trails, rootSPN := trailsFromByteSlices(items)
rootHash = rootSPN.Hash
proofs = make([]*Proof, len(items))
proofs = make([]Proof, len(items))
for i, trail := range trails {
proofs[i] = &Proof{
proofs[i] = Proof{
Total: int64(len(items)),
Index: int64(i),
LeafHash: trail.Hash,
Expand Down Expand Up @@ -145,20 +145,20 @@ func (sp *Proof) ToProto() *cmtcrypto.Proof {
return pb
}

func ProofFromProto(pb *cmtcrypto.Proof, optional bool) (*Proof, error) {
func ProofFromProto(pb *cmtcrypto.Proof, optional bool) (Proof, error) {
if pb == nil || (len(pb.LeafHash) == 0 && len(pb.Aunts) == 0) {
if optional {
return nil, nil
return Proof{}, nil
}
return nil, errors.New("nil proof")
return Proof{}, errors.New("nil proof")
}

sp := new(Proof)

sp.Total = pb.Total
sp.Index = pb.Index
sp.LeafHash = pb.LeafHash
sp.Aunts = pb.Aunts
sp := Proof{
Total: pb.Total,
Index: pb.Index,
LeafHash: pb.LeafHash,
Aunts: pb.Aunts,
}

return sp, sp.ValidateBasic()
}
Expand Down Expand Up @@ -275,12 +275,12 @@ func trailsFromLeafHashes(leafHashes [][]byte) (trails []*ProofNode, root *Proof
}
}

func ProofsFromLeafHashes(leafHashes [][]byte) (rootHash []byte, proofs []*Proof) {
func ProofsFromLeafHashes(leafHashes [][]byte) (rootHash []byte, proofs []Proof) {
trails, rootNode := trailsFromLeafHashes(leafHashes)
rootHash = rootNode.Hash
proofs = make([]*Proof, len(leafHashes))
proofs = make([]Proof, len(leafHashes))
for i, trail := range trails {
proofs[i] = &Proof{
proofs[i] = Proof{
Total: int64(len(leafHashes)),
Index: int64(i),
LeafHash: trail.Hash,
Expand Down
6 changes: 3 additions & 3 deletions crypto/merkle/proof_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ func TestProofValidateBasic(t *testing.T) {
[]byte("watermelon"),
[]byte("kiwi"),
})
tc.malleateProof(proofs[0])
tc.malleateProof(&proofs[0])
err := proofs[0].ValidateBasic()
if tc.errStr != "" {
assert.Contains(t, err.Error(), tc.errStr)
Expand All @@ -186,15 +186,15 @@ func TestVoteProtobuf(t *testing.T) {
}{
{"empty proof", &Proof{}, false},
{"failure nil", nil, false},
{"success", proofs[0], true},
{"success", &proofs[0], true},
Copy link
Member

@tzdybal tzdybal Aug 20, 2025

Choose a reason for hiding this comment

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

Change v1 in testCases struct to Proof instead of referencing here and dereferencing in line 197.

Copy link
Contributor Author

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

Fixed in 47f748c. Updated the test case to handle the nil case separately and removed the dereference in the comparison, while maintaining the same test coverage.

}
for _, tc := range testCases {
pb := tc.v1.ToProto()

v, err := ProofFromProto(pb, false)
if tc.expPass {
require.NoError(t, err)
require.Equal(t, tc.v1, v, tc.testName)
require.Equal(t, *tc.v1, v, tc.testName)
} else {
require.Error(t, err)
}
Expand Down
2 changes: 1 addition & 1 deletion crypto/merkle/proof_value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func ValueOpDecoder(pop cmtcrypto.ProofOp) (ProofOperator, error) {
if err != nil {
return nil, err
}
return NewValueOp(pop.Key, sp), nil
return NewValueOp(pop.Key, &sp), nil
}

func (op ValueOp) ProofOp() cmtcrypto.ProofOp {
Expand Down
2 changes: 1 addition & 1 deletion rpc/core/blocks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ func (env *Environment) proveDataRootTuples(tuples []DataRootTuple, height int64
}
_, proofs := merkle.ProofsFromByteSlices(dataRootEncodedTuples)
//nolint:gosec
return proofs[height-int64(tuples[0].height)], nil
return &proofs[height-int64(tuples[0].height)], nil
Copy link
Member

@tzdybal tzdybal Aug 20, 2025

Choose a reason for hiding this comment

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

Is this needed? Can't we return merkle.Proof from this function?

Copy link
Contributor Author

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

Fixed in 47f748c. Changed proveDataRootTuples to return merkle.Proof directly instead of *merkle.Proof. The caller now takes the address when needed to maintain API compatibility with the external interface that still expects a pointer.

}

// fetchDataRootTuples takes an end exclusive range of heights and fetches its
Expand Down
10 changes: 5 additions & 5 deletions types/part_set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func PartFromProto(pb *cmtproto.Part) (*Part, error) {
}
part.Index = pb.Index
part.Bytes = pb.Bytes
part.Proof = *proof
part.Proof = proof

return part, part.ValidateBasic()
}
Expand Down Expand Up @@ -238,7 +238,7 @@ func NewPartSetFromData(data []byte, partSize uint32) (ops *PartSet, err error)
added, err := ops.AddPart(&Part{
Index: uint32(index),
Bytes: chunk,
Proof: *proofs[index],
Proof: proofs[index],
})
if err != nil {
return nil, err
Expand Down Expand Up @@ -308,7 +308,7 @@ func Encode(ops *PartSet, partSize uint32) (*PartSet, int, error) {
added, err := eps.AddPart(&Part{
Index: i,
Bytes: chunks[i],
Proof: *eproofs[i],
Proof: eproofs[i],
})
if err != nil {
return nil, 0, err
Expand Down Expand Up @@ -390,7 +390,7 @@ func Decode(ops, eps *PartSet, lastPartLen int) (*PartSet, *PartSet, error) {
added, err := ops.AddPart(&Part{
Index: uint32(i),
Bytes: d,
Proof: *proofs[i],
Proof: proofs[i],
})
if err != nil {
return nil, nil, err
Expand All @@ -414,7 +414,7 @@ func Decode(ops, eps *PartSet, lastPartLen int) (*PartSet, *PartSet, error) {
added, err := eps.AddPart(&Part{
Index: uint32(i),
Bytes: data[int(ops.Total())+i],
Proof: *eproofs[i],
Proof: eproofs[i],
})
if err != nil {
return nil, nil, err
Expand Down
2 changes: 1 addition & 1 deletion types/results.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func (a ABCIResults) Hash() []byte {
// ProveResult returns a merkle proof of one result from the set
func (a ABCIResults) ProveResult(i int) merkle.Proof {
_, proofs := merkle.ProofsFromByteSlices(a.toByteSlices())
return *proofs[i]
return proofs[i]
}

func (a ABCIResults) toByteSlices() [][]byte {
Expand Down
6 changes: 3 additions & 3 deletions types/row_proof.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type RowProof struct {
RowRoots []tmbytes.HexBytes `json:"row_roots"`
// Proofs is a list of Merkle proofs where each proof proves that a row
// exists in a Merkle tree with a given data root.
Proofs []*merkle.Proof `json:"proofs"`
Proofs []merkle.Proof `json:"proofs"`
// StartRow the index of the start row.
// Note: currently, StartRow is not validated as part of the proof verification.
// If this field is used downstream, Validate(root) should be called along with
Expand Down Expand Up @@ -66,10 +66,10 @@ func RowProofFromProto(p *tmproto.RowProof) RowProof {
return RowProof{}
}
rowRoots := make([]tmbytes.HexBytes, len(p.RowRoots))
rowProofs := make([]*merkle.Proof, len(p.Proofs))
rowProofs := make([]merkle.Proof, len(p.Proofs))
for i := range p.Proofs {
rowRoots[i] = p.RowRoots[i]
rowProofs[i] = &merkle.Proof{
rowProofs[i] = merkle.Proof{
Total: p.Proofs[i].Total,
Index: p.Proofs[i].Index,
LeafHash: p.Proofs[i].LeafHash,
Expand Down
4 changes: 2 additions & 2 deletions types/row_proof_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ var incorrectRoot = bytes.Repeat([]byte{0}, 32)
func validRowProof() RowProof {
return RowProof{
RowRoots: tmbytes.FromBytes([]byte{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x9d, 0xe6, 0x38, 0x91, 0xc1, 0x6, 0xaf, 0x81, 0x75, 0x5a, 0x36, 0xf5, 0xb2, 0x62, 0x1e, 0xfa, 0xb9, 0xb8, 0x73, 0x87, 0xef, 0xe3, 0x6b, 0x33, 0xd8, 0xbf, 0xc9, 0x87, 0x1b, 0x8d, 0xfa, 0x8a}),
Proofs: []*merkle.Proof{
Proofs: []merkle.Proof{
{
Total: 128,
Index: 0,
Expand All @@ -105,7 +105,7 @@ func mismatchedRowRoots() RowProof {

func mismatchedProofs() RowProof {
rp := validRowProof()
rp.Proofs = []*merkle.Proof{}
rp.Proofs = []merkle.Proof{}
return rp
}

Expand Down
4 changes: 2 additions & 2 deletions types/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (txs Txs) Proof(i int) TxProof {
return TxProof{
RootHash: root,
Data: txs[i],
Proof: *proofs[i],
Proof: proofs[i],
}
}

Expand Down Expand Up @@ -272,7 +272,7 @@ func TxProofFromProto(pb cmtproto.TxProof) (TxProof, error) {
pbtp := TxProof{
RootHash: pb.RootHash,
Data: pb.Data,
Proof: *pbProof,
Proof: pbProof,
}

return pbtp, nil
Expand Down
Loading