x/ibc/core/23-commitment/types: invalid code in MerkleProof uses impossible conversions and burns unnecessary allocations and CPU cycles

[odeke-em]

Summary of Bug

While auditing and testing out code for correctness and bugs, I noticed this code:

The code

cosmos-sdk/x/ibc/core/23-commitment/types/merkle.go

Lines 273 to 275 in c6b8e5f

	func (proof MerkleProof) Empty() bool {
	return proto.Equal(&proof, nil) || proto.Equal(&proof, &MerkleProof{}) || proto.Equal(&proof, &tmcrypto.ProofOps{})
	}

tries to compare the address taken from a value with nil

Problems

• This code comparison is invalid and impossible to happen. Taking an address from a value can NEVER result in nil
• On every comparison, this code allocates (56B on 64-bit systems):
  • &MerkleProof{} (12B on 32-bit, 24B on 64-bit)
  • &tmcrypto.ProofOps{} (12B on 32-bit, 24B on 64-bit)
  • A pointer &MerkleProof -- which can never be zero
  • Burns extra CPU cycles on code that'll never match

We are expending bytes and CPU time on code unconditionally that's meant to be a cheap operation

Remedy

Given the operations attempted in here, we should really be using a pointer.
We could perhaps make this

var blankMerkleProof = &MerkleProof{}
var blankProofOps = &tmcrypto.ProofOps{}
func (proof *MerkleProof) Empty() bool { 
 	return proof == nil || proto.Equal(proof, blankMerkleProof) || proto.Equal(proof, blankProofOps) 
} 

which produces better results in every dimension

$ benchstat before.txt after.txt 
name     old time/op    new time/op    delta
Empty-8     311ns ± 5%     232ns ± 5%  -25.49%  (p=0.000 n=20+19)

name     old alloc/op   new alloc/op   delta
Empty-8     56.0B ± 0%      8.0B ± 0%  -85.71%  (p=0.000 n=20+20)

name     old allocs/op  new allocs/op  delta
Empty-8      3.00 ± 0%      1.00 ± 0%  -66.67%  (p=0.000 n=20+20)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned