util: Type-safe transaction identifiers by dergoegge · Pull Request #28107 · bitcoin/bitcoin

dergoegge · 2023-07-19T14:10:55Z

We currently have two different identifiers for transactions: txid (refering to the hash of a transaction without witness data) and wtxid (referring to the hash of a transaction including witness data). Both are typed as uint256 which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.

This PR introduces explicit Txid and Wtxid types that (if used) would cause compilation errors for such type confusion bugs.

(Only the orphanage is converted to use these types in this PR)

DrahtBot · 2023-07-19T14:10:59Z

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.

Type	Reviewers
ACK	stickies-v, hebasto, instagibbs, BrandonOdiwuor, glozow, achow101
Concept ACK	TheCharlatan, luke-jr, ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28690 (build: Introduce internal kernel library by TheCharlatan)
#28481 (txorphanage: add size accounting, use wtxids, support multiple announcers by glozow)
#28438 (Use serialization parameters for CTransaction by ajtowns)

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.

maflcko · 2023-07-19T14:17:49Z

How is this different from GenTxid?

dergoegge · 2023-07-19T14:28:17Z

How is this different from GenTxid?

I think mostly compile-time vs. runtime checks? GenTxid can hold both txid types but doesn't enforce type correctness at compile time. Any type checks would happen at runtime, e.g. assert(gtxid.IsWtxid()). Also nothing prevents passing the wrong txid type when constructing a GenTxid.

(I guess with this PR we could consider making GenTxid a std::variant<Txid, Wtxid>)

stickies-v

Concept ACK.

We don't consistently have clear variable naming (e.g. the ambiguous hash; or txid being used to refer to a wtxid), sometimes requiring quite a bit of developer time to figure out the expected ~type. I have experienced this quite a bit myself.

Introducing types makes this explicit at relatively low overhead, and I think the approach of subclassing uint256 makes sense.

We'll have to be careful for this not to lead to unnecessary refactoring code churn, but that shouldn't be a reason not to make this change imo.

glozow · 2023-07-20T12:28:52Z

I've definitely written/seen bugs that could be prevented if Txid and Wtxid were different types. When I review code it's something that I look for and it comes up surprisingly often. Interfaces taking uint256 often implicitly expect them to be a txid, eg #23325.

Agree that checking at compile time would be nice and probably prevent bugs / make review easier. Concept ACK assuming we're ok with the amount of code change.

dergoegge · 2023-07-20T12:29:33Z

We'll have to be careful for this not to lead to unnecessary refactoring code churn, but that shouldn't be a reason not to make this change imo.

In terms of refactoring, this should be similar to the time type-safety PRs in that we can gradually convert our code.

instagibbs · 2023-07-20T15:24:08Z

concept ACK, it comes up pretty often

to work around this I'm writing Assume's everywhere to make things clearer for me :)

jonatack · 2023-07-20T16:11:45Z

If helpful, a somewhat similar pros/cons discussion took place in March 2021 in #21328.

dergoegge · 2023-07-21T15:31:51Z

Ready for review

sedited · 2023-07-21T16:30:45Z

Concept ACK

luke-jr · 2023-07-25T18:26:37Z

Concept ACK, but I'm not too sure about the Wtxid type. It's literally just a hash of the transaction, not really an identifier per se.

dergoegge · 2023-07-26T12:51:03Z

Concept ACK, but I'm not too sure about the Wtxid type.

Could you clarify your objections about the Wtxid type? This PR aims to catch type confusion bugs (confusion between txids and wtxids) at compile-time and I don't see how to do that without these explicit types.

src/util/transaction_identifier.h

instagibbs

ACK 76fe2c304201646d63b1b01b397e18a99252a2d9

played around with some extensions to see where it could go, I think longer term we could use GenTxid for situations where both are acceptable, and Wtxid/Txid when only one type is, and slowly migrate towards that. f.e. GenTxid could have a GetTypedHash to convert to the other.

src/txorphanage.cpp

dergoegge · 2023-08-07T13:40:47Z

@MarcoFalke could you give this another look? You've done similar work on our time type-safety, so I'd appreciate your feedback.

maflcko

left some quick comments

src/util/transaction_identifier.h

hebasto · 2023-10-12T11:12:59Z

src/util/transaction_identifier.h

+    constexpr int Compare(const Other& other) const
+    {
+        static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
+        return 0;


I wonder should we use the [[noreturn]] attribute instead?

stickies-v

ACK 940a499

hebasto · 2023-10-12T12:39:41Z

A type safety implies that the following code:

CMutableTransaction tx;
uint256 hash = tx.GetHash();

must fail to compile. But it does.

If it is expected, maybe document such behaviour?

dergoegge · 2023-10-12T12:41:14Z

If it is expected, maybe document such behaviour?

It is documented here:

bitcoin/src/util/transaction_identifier.h

Lines 52 to 59 in 940a499

    
               /** Conversion function to `uint256`. 
        
                * 
        
                * Note: new code should use `ToUint256`. 
        
                * 
        
                * TODO: This should be removed once the majority of the code has switched 
        
                * to using the Txid and Wtxid types. Until then it makes for a smoother 
        
                * transition to allow this conversion. */ 
        
               operator uint256() const { return m_wrapped; }

hebasto

ACK 940a499, I have reviewed the code and it looks OK.

~~nit in 940a499: using auto instead of explicit iterator types might make the code more concise, readable and maintainable.~~

EDIT: I saw #28107 (comment)

instagibbs

re-ACK 940a499

BrandonOdiwuor

re-ACK 940a499

glozow

reACK 940a499
range-diff from last review

export Txid and Wtxid #28107 (comment)
no reassignment of inv #28107 (comment)
comment telling people to use FromUint256 #28107 (comment)
overloaded instead of templated Compare #28107 (comment)
removing unnecessary autoing #28107 (comment)
includes
dropping unnecessary #28107 (comment)
using std::byte instead of unsigned char + needed casts #28107 (comment)

glozow · 2023-10-12T15:04:19Z

Assuming we should wait until branch off to merge?

fanquake · 2023-10-12T15:09:37Z

Assuming we should wait until branch off to merge?

Any particular reason? If these start getting used more widely, it's just going to make doing backports more complicated if so.

glozow · 2023-10-12T15:19:58Z

Assuming we should wait until branch off to merge?

Any particular reason? If these start getting used more widely, it's just going to make doing backports more complicated if so.

Idc that much, was asked why I didn't merge yet. Conversions are still possible so it shouldn't be that complicated, but whatever you prefer. Could use this time to look at what it looks like to change the rest of the codebase and see if we run into any bumps.

ajtowns · 2023-10-19T06:09:37Z

src/net_processing.cpp

        for (const auto& ptx : pblock->vtx) {
            m_recent_confirmed_transactions.insert(ptx->GetHash());
-            if (ptx->GetHash() != ptx->GetWitnessHash()) {
+            if (ptx->HasWitness()) {


Is this an improvement? Comparing two uint256s is O(1) time and fairly cache friendly, whereas checking an arbitrary number of inputs' witnesses might not be either of those.

Comparing Txid and Wtxid is no longer allowed. We could use ToUint256 or add a cache for HasWitness?

I like using HasWitness because it is immediately obvious what we are checking.

Can CTransaction::HasWitness be changed to check whether hash != m_witness_hash (after the unavoidable first time when you need to iterate through the inputs to look for witnesses)?

Could move the "iterate through" part into ComputeWitnessHash, and have HasWitness just return hash.ToUint256() == m_witness_hash.ToUint256(). Note that that's an example of where ToUint256() not doing a copy would make sense.

src/util/transaction_identifier.h

ajtowns · 2023-10-19T06:11:21Z

src/util/transaction_identifier.h

+    uint256 m_wrapped;
+
+    // Note: Use FromUint256 externally instead.
+    transaction_identifier(const uint256& wrapped) : m_wrapped{wrapped} {}


What's the advantage of FromUint256 over just declaring this as explicit and making it public?

That might be better but would allow something like Txid{Wtxid{}} right now (because of the conversion function). We could make it a template like we do for Compare...

But (a) that would presumably be silly to write, and (b) the conversion function is going away, which will ensure any code like that also goes away.

src/util/transaction_identifier.h

achow101 · 2023-10-26T18:09:19Z

ACK 940a499

dergoegge · 2023-10-26T18:32:33Z

I'll open a follow up PR for @ajtowns's suggestions soon.

stickies-v reviewed Jul 20, 2023

View reviewed changes

dergoegge force-pushed the 2023-07-typesafe-txids branch from eb26626 to 369e790 Compare July 21, 2023 15:05

dergoegge changed the title ~~rfc: Type-safe transaction identifiers~~ util: Type-safe transaction identifiers Jul 21, 2023

DrahtBot added the Utils/log/libs label Jul 21, 2023

dergoegge force-pushed the 2023-07-typesafe-txids branch from 369e790 to 1aecdbc Compare July 21, 2023 15:23

DrahtBot added the CI failed label Jul 21, 2023

dergoegge force-pushed the 2023-07-typesafe-txids branch from 1aecdbc to c7338d8 Compare July 21, 2023 15:28

dergoegge marked this pull request as ready for review July 21, 2023 15:31

dergoegge force-pushed the 2023-07-typesafe-txids branch from c7338d8 to 76fe2c3 Compare July 21, 2023 15:33

DrahtBot removed the CI failed label Jul 21, 2023

vasild reviewed Jul 31, 2023

View reviewed changes

src/util/transaction_identifier.h Outdated Show resolved Hide resolved

instagibbs approved these changes Aug 3, 2023

View reviewed changes

src/txorphanage.cpp Outdated Show resolved Hide resolved

maflcko reviewed Aug 7, 2023

View reviewed changes

dergoegge force-pushed the 2023-07-typesafe-txids branch from 76fe2c3 to c1f9c3b Compare August 8, 2023 13:28

hebasto reviewed Oct 12, 2023

View reviewed changes

stickies-v approved these changes Oct 12, 2023

View reviewed changes

DrahtBot requested review from BrandonOdiwuor, glozow and hebasto and removed request for BrandonOdiwuor October 12, 2023 12:12

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor October 12, 2023 12:39

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor October 12, 2023 12:41

hebasto approved these changes Oct 12, 2023

View reviewed changes

DrahtBot removed the request for review from BrandonOdiwuor October 12, 2023 12:57

instagibbs approved these changes Oct 12, 2023

View reviewed changes

BrandonOdiwuor reviewed Oct 12, 2023

View reviewed changes

glozow reviewed Oct 12, 2023

View reviewed changes

lorenzolfm mentioned this pull request Oct 15, 2023

Tópicos Outubro saopaulobitdevs/saopaulobitdevs.org#14

Closed

instagibbs mentioned this pull request Oct 16, 2023

type-safe(r) GenTxid constructors #28658

Closed

ajtowns reviewed Oct 19, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 23, 2023

build: Introduce internal kernel library #28690

Open

dergoegge mentioned this pull request Nov 1, 2023

Improve peformance of CTransaction::HasWitness (28107 follow-up) #28766

Merged

dergoegge mentioned this pull request Mar 20, 2024

Txid type-safety refactors dergoegge/bitcoin#3

Closed

AngusP mentioned this pull request Mar 25, 2024

refactor: Use typesafe Wtxid in compact block encoding message, instead of ambiguous uint256. dergoegge/bitcoin#4

Closed

fanquake mentioned this pull request Mar 27, 2024

refactor: Use typesafe Wtxid in compact block encodings #29752

Merged

Conversation

dergoegge commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko commented Jul 19, 2023

Uh oh!

dergoegge commented Jul 19, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Jul 20, 2023

Uh oh!

dergoegge commented Jul 20, 2023

Uh oh!

instagibbs commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dergoegge commented Jul 21, 2023

Uh oh!

sedited commented Jul 21, 2023

Uh oh!

luke-jr commented Jul 25, 2023

Uh oh!

dergoegge commented Jul 26, 2023

Uh oh!

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dergoegge commented Aug 7, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 12, 2023

Uh oh!

dergoegge commented Oct 12, 2023

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Oct 12, 2023

Uh oh!

fanquake commented Oct 12, 2023

Uh oh!

glozow commented Oct 12, 2023

Uh oh!

dergoegge commented Jul 19, 2023 •

edited

Loading

DrahtBot commented Jul 19, 2023 •

edited

Loading

instagibbs commented Jul 20, 2023 •

edited

Loading

jonatack commented Jul 20, 2023 •

edited

Loading

hebasto left a comment •

edited

Loading

glozow Oct 20, 2023 •

edited

Loading

ajtowns Oct 19, 2023 •

edited

Loading