Add SPV inclusion check that returns block heights#140
Conversation
frolvanya
left a comment
frolvanya
left a comment
There was a problem hiding this comment.
Let's add a couple of tests to test_basics.rs
| >= args.confirmations, | ||
| "Not enough blocks confirmed" | ||
| ); | ||
| match self.verify_transaction_inclusion_with_heights(proof) { |
There was a problem hiding this comment.
Why do we need to change v1 if it's deprecated?
There was a problem hiding this comment.
It is just to reduce the amount of copy-paste code. But may be you are right in the point of view, to always check the coinbase tx in verify_transaction_inclusion_with_heights...
There was a problem hiding this comment.
Return back verify_tx_inclusion. Now removed Some in coinbase_tx_id in the arguments of verify_tx_inclusion_with_heights. Decided to add a check for now that these values are defaults.
After some time we'll remove this part and will always perform the check.
added: 7ecc6da |
frolvanya
left a comment
frolvanya
left a comment
There was a problem hiding this comment.
FYI, we have a flaky test, it's better to fix it later
2 participants
Summary
verify_transaction_inclusion_with_heights(TxInclusionProof) -> Option<TxInclusionInfo>returning the tx's block height and the current mainchain tip height in one call.verify_transaction_inclusion(bool, withconfirmations) preserved — now delegates to the new method and applies the confirmations check on top.btc-types::contract_args:TxInclusionProof,TxInclusionInfo,TxBlockMeta. ExistingProofArgsis unchanged, no protocol break.Motivation
btc-connector needs to aggregate transferred amounts per block, so it needs the tx's block height alongside the inclusion proof. It also wants to apply the confirmation-depth policy adaptively on its side rather than committing to a single threshold at call time. The new method returns both heights in a single call and avoids a follow-up query just to get the height.