[Feature Request]: Unify the representation of jsonProof and pb.Proof

[zvolin]

Implementation ideas

Hi,
I think it'd be nice if the jsonProof and pb.Proof had the same representations. My use case is that I'm generating most of the celestia types from protobuf definitions and that would allow me to re-use the pb-generated proof type in node rpc api calls without translating between types.

Currently those two look like so
jsonProof

type jsonProof struct {
	Start                   int      `json:"start"`
	End                     int      `json:"end"`
	Nodes                   [][]byte `json:"nodes"`
	LeafHash                []byte   `json:"leaf_hash"`
	IsMaxNamespaceIDIgnored bool     `json:"is_max_namespace_id_ignored"`
}

pb.Proof

message Proof {
  int64 start = 1;
  int64 end = 2;
  repeated bytes nodes = 3;
  bytes hashleaf = 4;
}

Eg the protobuf definition could be changed like so to allow for seamless transitions:

message Proof {
  int64 start = 1;
  int64 end = 2;
  repeated bytes nodes = 3;
  bytes leaf_hash = 4;
  bool is_max_namespace_id_ignored = 5;
}

I know this would be a breaking change tho, so curious to hear your opinion

[zvolin]

Also it looks like in eds/byzanine the proof has leaf_hash field, not hashleaf.
Could there be just one proto definition for proof?

[vgonkivs]

Hello @zvolin. Thanks for creating an issue. Yes, I think it makes sense to keep them consistent. Will extend protocol struct with one more field and fix naming

[Wondertan]

I am also thinking about removing is-max-namespace-ignore field in JSON instead. This is a system wide param, rather than per proof, so we can simplify things by removing it.

@vgonkivs, also consider unifying the BEFP proof proto definition if possible

[vgonkivs]

@Wondertan it's just 1 byte and I don't think it will take much place. Also, please not that json representation is from nmt repo. Anyway, I've created an issue in the nmt and proposing to move all photo-proof code there - closer to the implementation.
celestiaorg/nmt#219

[zvolin]

it's just 1 byte

iirc bools are encoded as 4 bytes on wire

they are encoded as i32, which takes up to 4 bytes, so they probably just take 1 or 2 🙏

[vgonkivs]

Yes, they Are encoded as i32 but protobuf also uses a form of variable-length encoding called "variant" encoding. That means that smaller values can be encoded using fewer bytes. So, it's possible that true/false can be represented in a single byte.

[zvolin]

@Wondertan I don't think that the recently linked pull request resolves the problem mentioned. It is only related in a sense that it fixes the json representation of something, but it doesn't touch the Proof at all.

Ideally, I'd like to have it completed once

• there is a single protobuf definition for Proof (currently I've noticed at least 2, which have different field namings, from share/p2p and share/eds)
• their json Proof representation is consistent with the Proof from the nmt module (or at least they have the same field names)

I'm happy to discuss those, but please reopen as this PR wasn't really related

[zvolin]

I think the problem is that in the linked commit there is:

Partly fixes #2427

Github only cares about the part fixes https://github.com/celestiaorg/celestia-node/issues/2427. If you want to link a PR to the issue, just use mentions instead of fixes as this is a keyword in github's understanding

[Wondertan]

Thanks for noting @zvolin. This issue is definitely not resolved. It was closed due to the “fixes” keyword in celestiaorg/go-header#82 after the merge.

EDIT: as you mentioned in the subsequent comment, which I read after writing the comment

[Wondertan]

Also, @vgonkivs meant to link #2423 there, not #2327(current one)

[vgonkivs]

ooooops, yes, my fault. Sorry.
Regarding the current issue: I've created an issue in the nmt repo, so we can have a single representation of the pb.Proof and in case nmt.Proof will be modified, json/pb representations will be also modified.

[vgonkivs]

Hello @zvolin, we moved a pb definition of proof to the nmt library. You can find it here. The last thing is to rename IsMaxNamespaceIDIgnored to IsMaxNamespaceIgnored which I believe will be done in the scope of celestiaorg/nmt#206