type-safe(r) GenTxid constructors #28658
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, 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. |
instagibbs
force-pushed
the
2023-10-typesafe-gentxid-ctor
branch
from
8a1a8c0
to
7acc86e
Compare
| @@ -430,8 +430,8 @@ class GenTxid | |||
| GenTxid(bool is_wtxid, const uint256& hash) : m_is_wtxid(is_wtxid), m_hash(hash) {} | |||
|
|
|||
| public: | |||
| static GenTxid Txid(const uint256& hash) { return GenTxid{false, hash}; } | |||
| static GenTxid Wtxid(const uint256& hash) { return GenTxid{true, hash}; } | |||
There was a problem hiding this comment.
Should GenTxid just become a std::variant<Txid,Wtxid> at this point? cf #28107 (comment)
In either case, these should be explicit GenTxid(const Txid& txid) constructors, afaics.
There was a problem hiding this comment.
Could DRY it as
template<bool has_witness>
explicit GenTxid(const transaction_identifier<has_witness>& id) m_is_wtxid{has_witness}, m_hash{id.ToUint256()} {}There was a problem hiding this comment.
We could go the "whole way", would just change quite a bit of code. If that's what we want to do, sure.
There was a problem hiding this comment.
I don't think explicit GenTxid(...) requires changing a bunch of code. It is simply a constructor that can be used by new code, and old code can (for now) use the old methods to create the object.
Should be fine to add and use it, maybe as part of one of the follow-ups to convert more code?
| @@ -705,7 +705,7 @@ class CTxMemPool | |||
| LOCK(cs); | |||
| // Sanity check the transaction is in the mempool & insert into | |||
| // unbroadcast set. | |||
| if (exists(GenTxid::Txid(txid))) m_unbroadcast_txids.insert(txid); | |||
| if (exists(GenTxid::Txid(Txid::FromUint256(txid)))) m_unbroadcast_txids.insert(txid); | |||
There was a problem hiding this comment.
Most of these changes just makes the code more verbose without making it any clearer or much safer, IMO -- this one would at least be clearer if it was changing the function to AddUnbroadcastTx(const Txid& txid)
There was a problem hiding this comment.
Most of this GenTxid::Txid(Txid::FromUint256()) chaining can be removed by further work pushing the types "up"
Can do that if that's the feedback.
|
|
up for grabs, opening #28997 |
…d::Wtxid not GenTxid::Txid 38816ff fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid (Greg Sanders) Pull request description: Fixes the bugs in the fuzz test with no more changes as an alternative to bitcoin/bitcoin#28658 ACKs for top commit: naumenkogs: ACK 38816ff dergoegge: ACK 38816ff Tree-SHA512: 5e46a83f2b2a2ac0672a63eb6200b019e01089ab1aa80c4ab869b6fcf27ccf2e84a064e96397f1a1869ccfa43b0c9638cbae681a27c4ca3c96ac71f41262601e
Builds on #28107
Tiny(<30 loc) set of changes that detects issues with fuzz targets in master, and longer term should make things safer.
Most of this
GenTxid::Txid(Txid::FromUint256())chaining can be removed by further work pushing the types "up", and maybe other helper functions that make them directly out ofCOutPoints or similar since that appears to be a common pattern.