type-safe(r) GenTxid constructors

instagibbs commented at 6:29 pm on October 16, 2023: member

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.

[net processing] Use HasWitness over comparing (w)txids cdb14d79e8

Introduce types for txids & wtxids ed70e65016

Use type-safe txid types in orphanage 940a49978c

DrahtBot commented at 6:29 pm on October 16, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28970 ([WIP] p2p: opportunistically accept 1-parent-1-child packages by glozow)

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.

fanquake requested review from dergoegge on Oct 16, 2023

DrahtBot added the label CI failed on Oct 16, 2023

Remove uint256-typed GenTxid::{Wt,T}xid constructors 7acc86e1f1

instagibbs force-pushed on Oct 17, 2023

DrahtBot removed the label CI failed on Oct 17, 2023

in src/primitives/transaction.h:434 in 7acc86e1f1

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}; }

ajtowns commented at 6:23 am on October 19, 2023:

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.

ajtowns commented at 7:49 am on October 19, 2023:

Could DRY it as

0template<bool has_witness>
1explicit GenTxid(const transaction_identifier<has_witness>& id) m_is_wtxid{has_witness}, m_hash{id.ToUint256()} {}

instagibbs commented at 1:32 pm on October 19, 2023:

We could go the “whole way”, would just change quite a bit of code. If that’s what we want to do, sure.

maflcko commented at 11:21 am on December 5, 2023:

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?

in src/txmempool.h:708 in 7acc86e1f1

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);

ajtowns commented at 6:42 am on October 19, 2023:

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)

instagibbs commented at 1:33 pm on October 19, 2023:

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.

glozow added the label Refactoring on Oct 20, 2023

DrahtBot added the label CI failed on Nov 15, 2023

DrahtBot commented at 11:42 am on November 16, 2023: contributor

 0validation.cpp: In lambda function:
 1validation.cpp:365:70: error: ‘transaction_identifier<has_witness>::transaction_identifier(const uint256&) [with bool has_witness = false]’ is private within this context
 2  365 |                 if (m_mempool->exists(GenTxid::Txid(txin.prevout.hash))) continue;
 3      |                                                                      ^
 4In file included from ./primitives/transaction.h:14,
 5                 from ./primitives/block.h:9,
 6                 from ./chain.h:13,
 7                 from ./validation.h:15,
 8                 from validation.cpp:6:
 9./util/transaction_identifier.h:16:5: note: declared private here
10   16 |     transaction_identifier(const uint256& wrapped) : m_wrapped{wrapped} {}
11      |     ^~~~~~~~~~~~~~~~~~~~~~
12make[2]: *** [Makefile:11211: libbitcoin_node_a-validation.o] Error 1
13make[2]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
14make[1]: *** [Makefile:20226: install-recursive] Error 1
15make[1]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
16make: *** [Makefile:812: install-recursive] Error 1
17
18Exit status: 2������������������������

DrahtBot removed the label CI failed on Nov 29, 2023

instagibbs commented at 7:46 pm on December 4, 2023: member

up for grabs, opening #28997

instagibbs closed this on Dec 4, 2023

fanquake referenced this in commit b3ab0c3819 on Dec 5, 2023

type-safe(r) GenTxid constructors #28658

Code Coverage

Reviews

Conflicts