RelayTransaction expects to pass a TxId and Wtxid as arguments, but any uint256 type is really accepted. When calling RelayTransaction, most times we are already passing tx->GetHash() and tx->GetWitnessHash() to it, and their value are downcasted.
Updates RelayTransaction and make sure we are consistent with its arguments
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32104.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process. A summary of reviews will appear here.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
4240 | } else {
4241 | LogPrintf("Force relaying tx %s (wtxid=%s) from peer=%d\n",
4242 | - tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), pfrom.GetId());
4243 | - RelayTransaction(tx.GetHash(), tx.GetWitnessHash());
4244 | + txid.ToString(), wtxid.ToString(), pfrom.GetId());
4245 | + RelayTransaction(txid, wtxid);
not sure. This seems like a pessimization that makes it harder to use the type-safe Txid and Wtxid in RelayTransaction
It would be better to make this code type-safe, to avoid having to touch it again in the future.
If that is the case, shouldn't we be enforcing RelayTransaction to take Txid and Wtxid instead of uint256?
Yes. Alternatively, at a minimum the txid and wtxid types should be adjusted here. Otherwise, this refactor may cause a silent merge conflict with the type-safety refactor.
I've updated the approach to enforce RelayTransaction types, and extended it to all other calls to the method
RelayTransaction expects to pass a TxId and Wtxid as arguments, but
any uint256 type is really accepted. When calling RelayTransaction, most times
we are already passing tx->GetHash() and tx->GetWitnessHash() to it, and their
value are downcasted.
Updates RelayTransaction and make sure we are consistent with its arguments
cc @marcofleon & #31001.
Oh, I didn't realize Marco was already working on this. I saw this when updating the Erlay PR and thought it may be good to standalone fix. I'm happy to close it if you're planning to get it addressed @marcofleon
Lgtm as a standalone change. It's the same change I implement in fa8400494e3c411a55375e71abaaa2637a22e216 as part of the complete refactor. I actually missed all the tx.GetHash() and tx.GetWitnessHash() in ProcessMessage so good catch there.
My final refactor does end up altering RelayTransaction in a more involved way, but that includes changes (switching GenTxid to a variant) that haven't been discussed yet. I'll open a draft PR of that branch this week.