RelayTransaction
function to replace uint256
with the Txid
and Wtxid
types, improving type safety and helping with the gradual transition to using these types throughout the codebase.
RelayTransaction
#31001
RelayTransaction
function to replace uint256
with the Txid
and Wtxid
types, improving type safety and helping with the gradual transition to using these types throughout the codebase.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
Concept ACK | jonatack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
$N
places with unsafe types and this fixes a good chunk ($nn%
) of them. If the standalone chunks are too small and this is split-up too much, the review overhead may be higher than needed to make meaningful progress here?
is this in view of some follow-on work or a one-off?
I noticed it while doing a bit of review on Erlay (https://github.com/bitcoin/bitcoin/pull/30116). I was planning on continuing to address places in the code where this switch is needed.
If the standalone chunks are too small and this is split-up too much, the review overhead may be higher than needed to make meaningful progress here?
Agreed that it may be too small a change to warrant a PR. I was trying to keep the scope within RelayTransaction
only. But I can do some more investigation and see if it makes sense to expand a bit. Or at least add context as you said.
Easier to swallow these useful pills if the code is going to have churn anyways
I understand where this is coming from but if we only do these refactors if we’re already touching the code, then I don’t think we’ll ever finish the migration (there is simply too much code to be touched). I’d prefer for us to move to the new types as soon as possible and remove the const uint256&
conversion function, so that we end up with actual type-safety.
I suggested to Marco to work on this, so that the txid type-safety project makes some progress. He has to get started somewhere and I don’t think it’s unreasonable to pick a small self-contained change to start with and then increase the size/scope for the next PRs.
@dergoegge that’s an argument for bigger bites then?
Not against these or more changes.
1742@@ -1743,7 +1743,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
1743 CTransactionRef tx = m_mempool.get(txid);
1744
1745 if (tx != nullptr) {
1746- RelayTransaction(txid, tx->GetWitnessHash());
1747+ RelayTransaction(tx->GetHash(), tx->GetWitnessHash());
tx
only needed inside the conditional
0- CTransactionRef tx = m_mempool.get(txid);
1-
2- if (tx != nullptr) {
3+ if (CTransactionRef tx = m_mempool.get(txid)) {
or
0+ if (CTransactionRef tx = m_mempool.get(txid); tx != nullptr) {