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 of COutPoints or similar since that appears to be a common pattern.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--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.
429 | @@ -430,8 +430,8 @@ class GenTxid 430 | GenTxid(bool is_wtxid, const uint256& hash) : m_is_wtxid(is_wtxid), m_hash(hash) {} 431 | 432 | public: 433 | - static GenTxid Txid(const uint256& hash) { return GenTxid{false, hash}; } 434 | - static GenTxid Wtxid(const uint256& hash) { return GenTxid{true, hash}; }
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.
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()} {}
We could go the "whole way", would just change quite a bit of code. If that's what we want to do, sure.
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?
704 | @@ -705,7 +705,7 @@ class CTxMemPool 705 | LOCK(cs); 706 | // Sanity check the transaction is in the mempool & insert into 707 | // unbroadcast set. 708 | - if (exists(GenTxid::Txid(txid))) m_unbroadcast_txids.insert(txid); 709 | + if (exists(GenTxid::Txid(Txid::FromUint256(txid)))) m_unbroadcast_txids.insert(txid);
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)
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.
validation.cpp: In lambda function:
validation.cpp:365:70: error: ‘transaction_identifier<has_witness>::transaction_identifier(const uint256&) [with bool has_witness = false]’ is private within this context
365 | if (m_mempool->exists(GenTxid::Txid(txin.prevout.hash))) continue;
| ^
In file included from ./primitives/transaction.h:14,
from ./primitives/block.h:9,
from ./chain.h:13,
from ./validation.h:15,
from validation.cpp:6:
./util/transaction_identifier.h:16:5: note: declared private here
16 | transaction_identifier(const uint256& wrapped) : m_wrapped{wrapped} {}
| ^~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [Makefile:11211: libbitcoin_node_a-validation.o] Error 1
make[2]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
make[1]: *** [Makefile:20226: install-recursive] Error 1
make[1]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
make: *** [Makefile:812: install-recursive] Error 1
Exit status: 2������������������������
up for grabs, opening #28997
