zoneconcierge: enriching SubmissionData to store BTCSpvProof

[SebastianElvis]

Partially fixes BM-304

This PR enriches the SubmissionData in BTCCheckpoint module to support ZoneConcierge's timestamps. Specifically, this PR

• replaces the BTC txs with the full BTCSpvProofs in SubmissionData
• adds the BTCSpvProofs in the best SubmissionData to the response of FinalizedChainInfo API. This allows one to verify whether the checkpoint is indeed on BTC or not.

In addition, this PR also moves BTCSpvProof from tx.proto to btccheckpoint.proto since it's strange to make btccheckpoint.proto to depend on tx.proto.

I marked @KonradStaniec as reviewer since this PR mostly touches the BTCCheckpoint module. Thanks Konrad!

[KonradStaniec]

There are two problems I see with sticking BTCSpvProof verbatim into SubmissionData.

1. It creates coupling between stuff we receive from outside world (as BTCSpvProof is part of message MsgInsertBTCSpvProof) and between stuff we have already validated and saved into database. Now if we want to change BTCSpvProof due to some message requirements we will also modify it in db or if we want to optimize db layout due to performance we will touch message.
   In general we should have:

• separate structure for the things module receive from untrusted sources (form the network or rpc, in general in transaction)
• separate structure for the things which is validated and trusted and saved in module database
• separate structure for the things in which module communicate with other modules i.e interfaces

2. Some of the fields in BTCSpvProof are really not necessary (like confirming_btc_header as this can be taken from btc light client, or btc_transaction_index as this is part of submission key).

Solution to both problems is creating separate struct like message TransactionInfo which will hold all necessary transactions data necessary to prove its existence maybe like:

message TransactionInfo {
  bytes transaction
  bytes proof // maybe it could use here better format as we alrready processed and valideated the proof ? something to consider taking into account who an when will verify this proofs
  TransactionKey transactionKey // Although we already have it as part of SubmissionKey, maybe it is worth having it here to not relay on fact that TransactionInfo will be ordered in the same order as TransactionKeys in SubmissionKey
}

[KonradStaniec]

@SebastianElvis could you remind me who and when will validate those bitcoin inclusion proofs ?

[SebastianElvis]

@SebastianElvis could you remind me who and when will validate those bitcoin inclusion proofs ?

These proofs will be a part of a timestamp of a CZ header. Upon a timestamp, the CZ will verify these proofs in order to ensure an the checkpoint of the epoch that includes this CZ header is indeed included in BTC.

[SebastianElvis]

Hey @KonradStaniec, thanks for the comments. Yeah I totally agree your point that we need to separate data that is pre- and post-verification. I have adopted your proposal of the new struct TransactionInfo with some slight changes, added some constructors for it, and revised the PR according to this model. Please feel free to have a look again :)

[KonradStaniec]

Doesn't we need to have standard go work around for the loop variable semantics as txKey is a pointer
i.e

for i, txKey := range submissionKey.Key {
  var key = txKey
  txs[i] = NewTransactionInfo(key, btcSpvProofs[i].BtcTransaction, btcSpvProofs[i].MerkleNodes)
}

Otherwise we will run into golang/go#56010 ? It would be nice to have even simple unit test showing that each TransactionInfo realy have expected key.

Also If we make this function public, we should handle case when length of submissionKey.Key is different than btcSpvProofs otherwise we risk having panic ( I am aware that there is comment, but this is not perfect precaution as people often do not read them 😅 )

I would either return error in that case and panic in msg processing if this happen (as this mean there is some bug ) or make this a private helper in keeper so that it will not be used outside of module.

[SebastianElvis]

Otherwise we will run into golang/go#56010 ? It would be nice to have even simple unit test showing that each TransactionInfo realy have expected key.

Thanks for the pointer, didn't know this pitfall in Go before! I have replaced txKey from a per-loop variable in the for loop with a per-iteration variable, and added more assertions for the TxsInfo in the unit test TestSubmitValidNewCheckpoint.

Also If we make this function public, we should handle case when length of submissionKey.Key is different than btcSpvProofs otherwise we risk having panic ( I am aware that there is comment, but this is not perfect precaution as people often do not read them 😅 )

Totally agree. Since this function is short and is only used here once, I removed this function and merged it into InsertBTCSpvProof.

Please feel free to have a look again.

[KonradStaniec]

In general I think I would prefer to think about SubmissionData as internal database type for btccheckpoint module, and have here some other type with only necessary data (like maybe SubmissionInfo or something like that) that way if SubmissionData will gain new fields those will not necessary affect QueryFinalizedChainInfoResponse.

But yea it is not blocking for me, as we can always refactor it later. (as there are probably more such dependencies between modules)

[SebastianElvis]

That's a good point! You are right that proof_epoch_submitted does not need everything in SubmissionData. I have replaced it with the two TransactionInfo, which are necessary and sufficient for verifying the inclusion of the epoch's checkpoint. Specifically, in TransactionInfo:

• TransactionKey and Proof will be used for verifying inclusion of two BTC txs, with the assistance of a BTC light client.
• Transaction will be used for reconstructing the raw checkpoint and comparing the raw checkpoint with the one in QueryFinalizedChainInfoResponse. We will also compare the hash of this tx with that in TransactionKey.