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

Copilot · 2025-08-20T17:55:35Z

This PR addresses an inconsistency in the merkle proof API where functions return pointers but callers immediately dereference them to values, creating unnecessary indirection and potential performance overhead.

Problem

The following functions were returning pointers that callers consistently dereferenced:

ProofsFromByteSlices returned []*Proof
ProofFromProto returned *Proof
ProofsFromLeafHashes returned []*Proof

Additionally, several struct fields and internal functions maintained pointer semantics when value semantics would be more efficient and clearer:

CompactBlock.proofsCache was []*merkle.Proof
RecoveryPart.Proof was *merkle.Proof
Various internal functions returned []*merkle.Proof

Call sites throughout the codebase immediately converted these to values:

// Before: unnecessary pointer allocation and dereference
_, proofs := merkle.ProofsFromByteSlices(chunks)
part := &Part{
    Proof: *proofs[index], // dereferencing pointer to get value
}

proof, err := merkle.ProofFromProto(&pb.Proof, false)
txProof := TxProof{
    Proof: *proof, // dereferencing pointer to get value
}

Solution

Changed the functions and fields to use values directly:

ProofsFromByteSlices now returns []Proof
ProofFromProto now returns Proof
ProofsFromLeafHashes now returns []Proof
CompactBlock.proofsCache is now []merkle.Proof
RecoveryPart.Proof is now merkle.Proof
Internal functions updated to return value slices

This eliminates the unnecessary pointer allocations and dereferences:

// After: direct value assignment
_, proofs := merkle.ProofsFromByteSlices(chunks)
part := &Part{
    Proof: proofs[index], // direct value assignment
}

proof, err := merkle.ProofFromProto(&pb.Proof, false)
txProof := TxProof{
    Proof: proof, // direct value assignment
}

Performance Optimization

Additionally optimized slice allocation in CompactBlock.Proofs() to eliminate potential reallocations by pre-allocating with exact size and using direct index assignment instead of append operations.

Benefits

Performance: Eliminates unnecessary heap allocations for *Proof objects that are immediately copied
Clarity: Removes confusing mixed pointer/value semantics throughout the codebase
Consistency: The Proof struct contains only simple fields (int64, []byte, [][]byte), making value copies efficient (shallow copy of slice headers)
Cache locality: Better memory access patterns without extra pointer indirection
Efficiency: Optimized slice operations to prevent unnecessary reallocations

Changes

Updated function signatures in crypto/merkle/proof.go
Removed dereference operations across call sites in types/, rpc/, and consensus/ packages
Updated struct fields to use value types consistently
Changed internal cache and proof handling to use values
Added address-of operations where external APIs still require pointers
Updated validation logic to check for zero values instead of nil
Updated tests to work with new value semantics
Optimized slice allocation patterns for better performance
Resolved merge conflicts with buffer-based PartSet refactor in main branch

All existing functionality is preserved - this is purely an API improvement that eliminates unnecessary indirection while adding performance optimizations.

Fixes #2357.

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

- Change ProofsFromByteSlices to return []Proof instead of []*Proof - Change ProofFromProto to return Proof instead of *Proof - Change ProofsFromLeafHashes to return []Proof instead of []*Proof - Update all call sites to remove pointer dereferences - Convert RowProof.Proofs field to []merkle.Proof for consistency - Update helper functions to provide pointers where needed for external APIs Co-authored-by: tzdybal <[email protected]>

tzdybal

Some more *Proofs can be easily changed to Proof.

@copilot please address review comments.

tzdybal · 2025-08-20T18:50:35Z

consensus/propagation/types/types.go

 		return nil, errors.New("invalid number of partset hashes")
 	}

 	c.proofsCache = make([]*merkle.Proof, 0, len(c.PartsHashes))


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

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

tzdybal · 2025-08-20T18:51:26Z

consensus/propagation/types/types.go

 		Index:  r.Index,
 		Data:   r.Data,
-		Proof:  proof,
+		Proof:  &proof,


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

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.

tzdybal · 2025-08-20T18:55:00Z

crypto/merkle/proof_test.go

 		{"empty proof", &Proof{}, false},
 		{"failure nil", nil, false},
-		{"success", proofs[0], true},
+		{"success", &proofs[0], true},


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

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.

tzdybal · 2025-08-20T18:59:45Z

rpc/core/blocks.go

 	_, proofs := merkle.ProofsFromByteSlices(dataRootEncodedTuples)
 	//nolint:gosec
-	return proofs[height-int64(tuples[0].height)], nil
+	return &proofs[height-int64(tuples[0].height)], nil


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

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.

tzdybal · 2025-08-20T19:03:00Z

@copilot please address the review comments.

Co-authored-by: tzdybal <[email protected]>

Copilot · 2025-08-20T19:24:02Z

@copilot please address the review comments.

All review comments have been addressed in 47f748c. The changes eliminate additional pointer indirection while maintaining API compatibility and passing all tests.

tzdybal · 2025-09-01T17:35:45Z

consensus/propagation/types/types.go

 	}

-	c.proofsCache = make([]*merkle.Proof, 0, len(c.PartsHashes))
+	c.proofsCache = make([]merkle.Proof, 0, len(c.PartsHashes))


@copilot, is the size of slice optimal? Elements are added in 2 loops, it seems reallocation will always happen, but we can avoid it. Please verify and fix if needed.

Fixed in 86ce837. Optimized slice allocation by pre-allocating with exact size make([]merkle.Proof, len(c.PartsHashes)) and using direct index assignment instead of append to eliminate any reallocation.

tzdybal · 2025-09-01T17:38:01Z

@copilot rebase changes on current main.

Co-authored-by: tzdybal <[email protected]>

Copilot · 2025-09-01T17:51:28Z

@copilot rebase changes on current main.

Completed in 86ce837. Successfully rebased all changes on current main branch (37feaa0), resolved merge conflicts from the buffer-based PartSet refactor, and updated all code to maintain compatibility while preserving the value-based proof semantics.

evan-forbes · 2025-10-13T04:00:49Z

is this still a draft? if so, then we should close due to staleness imo

Initial plan

2d1e1f8

Copilot AI assigned Copilot and tzdybal Aug 20, 2025

Copilot started work on behalf of tzdybal August 20, 2025 17:55 View session

Copilot AI changed the title ~~[WIP] Proofs are created as pointers but callers immediately dereference to values~~ fix: change proof functions to return values instead of pointers Aug 20, 2025

Copilot AI requested a review from tzdybal August 20, 2025 18:16

Copilot finished work on behalf of tzdybal August 20, 2025 18:16

tzdybal requested changes Aug 20, 2025

View reviewed changes

Copilot started work on behalf of tzdybal August 20, 2025 19:03 View session

fix: address review comments - change more proof types to values

47f748c

Co-authored-by: tzdybal <[email protected]>

Copilot AI requested a review from tzdybal August 20, 2025 19:25

Copilot finished work on behalf of tzdybal August 20, 2025 19:25

tzdybal reviewed Sep 1, 2025

View reviewed changes

Copilot started work on behalf of tzdybal September 1, 2025 17:38 View session

fix: optimize slice allocation to avoid reallocation

d0cb4b4

Co-authored-by: tzdybal <[email protected]>

Copilot AI requested a review from tzdybal September 1, 2025 17:52

Copilot finished work on behalf of tzdybal September 1, 2025 17:52

rach-id added the backport-to-v0.39.x label Sep 6, 2025

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

Copilot AI commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Performance Optimization

Benefits

Changes

Uh oh!

tzdybal 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!

tzdybal Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

tzdybal Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

tzdybal Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

tzdybal Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

tzdybal commented Aug 20, 2025

Uh oh!

Copilot AI commented Aug 20, 2025

Uh oh!

tzdybal Sep 1, 2025 • 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!

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

tzdybal commented Sep 1, 2025

Uh oh!

Copilot AI commented Sep 1, 2025

Uh oh!

evan-forbes commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Aug 20, 2025 •

edited

Loading

tzdybal left a comment •

edited

Loading

tzdybal Sep 1, 2025 •

edited

Loading