Type-safe transaction identifiers (utils/log/libs)


Host: stickies-v  -  PR author: dergoegge


  • Since the activation of SegWit, transactions are identifiable by two types of identifiers: txid and wtxid. Both identifiers are a uint256 resulting from a double sha256 hash of serialized transaction data. The core difference is that wtxid includes the witness data, while txid does not.

  • Type-safety is a concept in programming to help avoid type errors, enforcing strict correspondence between a variable and its type. It ensures that a program performs only legal operations on data, with validation occurring during compile-time. This proactive error detection and prevention lead to more reliable and maintainable software, minimizing unintended behavior and vulnerabilities. An example of an earlier type-safety improvement in Bitcoin Core is the incorporation of std::chrono, introduced in #16908.

  • In the context, of transaction identifiers, type-safety is important because it is impossible to deduce the type difference from just the uint256 value. With GenTxid type checking can be done, but it is more verbose and done at run-time instead of at compile-time. Additionally, transaction identifier types improve readability of code by being explicit about expected types instead of solely relying on clear variable naming.

  • This PR aims to improve type safety by introducing the new transaction_identifier type, with Txid and Wtxid typedefs for brevity.

  • There are instances where wtxid and txid are treated identically, such as for example in most_recent_block_txs, where the uint256 type is used to handle this. In other contexts such as CTxMemPool::info where wtxid and txid are treated differently but either is accepted, the GenTxid type is adopted.



  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

  2. What does it mean for a transaction identifier to be type-safe? Why is that important or helpful? Are there any downsides?

  3. Why is it better to enforce types at compile-time instead of at run-time?

  4. Conceptually, when writing new code that requires referring to transactions, when should you use txid and when should you use wtxid? Can you point to any examples in the code where using one instead of the other could be very bad? (1 point per example)

  5. In which concrete way(s) could using transaction_identifier instead of uint256 help find existing bugs or prevent the introduction of new ones? On the other hand, could this change introduce new bugs?

  6. SegWit activated at block height 481,824. Do transactions in blocks < 481,824 also have a wtxid? For example, what about transaction with txid dc01120a639283c11eeada6ce540178adfbbca7fe0d2f41076367cbc9e901822?

  7. Are there any places where a (w)txid is returned as a uint256 where it would not make sense to return a transaction_identifier instead?

  8. The GenTxid class already exists. How does it already enforce type correctness, and how is it different from the approach in this PR?

  9. Are there any places where GenTxid is used but using transaction_identifier would be better?

Code Review

  1. How is transaction_identifier able to subclass uint256, given that, in C++, integers are types and not classes?

  2. Why does transaction_identifier subclass uint256 instead of being a completely new type?

  3. Txid and Wtxid are typedefs for transaction_identifier<has_witness>. Do you see any reason for code to directly use the transaction_identifier type? Should that be forbidden, and if so, how?

  4. transaction_identifier::Compare is defined to return a int, yet reinterpret_cast<const uint256&> is used in the return statement. Is this correct, and if so, why? Why reinterpret_cast instead of another cast, such as static_cast?

  5. Is the has_witness template parameter used for purposes other than type differentiation in transaction_identifier? What are the limitations of this approach? Do you see alternatives?

  6. Why is TxOrphanage a good place to start introducing type-safe transaction ids?

  7. Consider return m_wtxid_to_orphan_it.count(Wtxid::FromUint256(gtxid.GetHash()));. Why is it not type-safe to allow Wtxid to be constructed by just passing a uint256 to its constructor? Specifically, why doesnโ€™t Wtxid{gtxid.GetHash()} compile?

Meeting Log

Meeting 1

  117:00 <stickies-v> #startmeeting
  217:00 <dberkelmans> hi
  317:00 <dergoegge> hi
  417:00 <BrandonOdiwuor> Hello
  517:01 <lightlike> hi
  617:01 <larryruane_> hi
  717:01 <vmammal> first timer here. im here to learn
  817:01 <stickies-v> welcome everyone! Today we're looking at #28107, authored by dergoegge . The notes and questions are available on https://bitcoincore.reviews/28107
  917:02 <stickies-v> great, glad you're joining us vmammal! any other first timers today?
 1017:02 <Murch[m]> Hey
 1117:02 <stickies-v> even if you're just lurking, feel free to say hi!
 1217:02 <sipa> hi
 1317:02 <larryruane_> will we be back here tomorrow? are we covering the first set of questions today (concept)?
 1417:03 <stickies-v> correct larryruane_ ! same time tomorrow for the second batch of questions
 1517:03 <glozow> hi
 1617:03 <effexzi> Hi every1
 1717:03 <maxedwards> hi
 1817:03 <stickies-v> who got the chance to review the PR or read the notes? (y/n)
 1917:03 <Murch[m]> n
 2017:03 <maxedwards> y
 2117:03 <BrandonOdiwuor> y
 2217:03 <lightlike> y
 2317:03 <dberkelmans62> y
 2417:03 <abubakarsadiq> hi
 2517:03 <glozow> woohooo welcome newcomers!
 2617:03 <glozow> y
 2717:04 <stickies-v> alright wonderful, let's dive into the wonderfully wholesome world of type-safety
 2817:05 <larryruane_> i heard someone say years ago, "strong types are for weak minds" (haha I don't agree!)
 2917:05 <stickies-v> for those of you who were able to review, would you give it a Concept ACK, Approach ACK, Tested ACK, or NACK? what was your review approach?
 3017:05 <vmammal> concept ACK
 3117:06 <larryruane_> i'm still reviewing
 3217:06 <maxedwards> Concept ACK
 3317:06 <BrandonOdiwuor> Concept ACK
 3417:06 <abubakarsadiq> Concept ACK, did not do code review but I think this is very useful improvement, not only that uint256 can be block hask, header, txid and wtxid. It will be nice to distinguish between wtxid and txid with type. Because there is ambiguity in the code whereby hash is referred to txid, wtxid and blockhash. Using type specifier for wtxid and txid will help distinguish even if an ambiguous variable name is used
 3517:07 <maxedwards> built and ran tests, read through the code, looked at the super class of uint256
 3617:07 <stickies-v> abubakarsadiq: I agree! compile-time checks (which we'll get to in a bit) are a great improvement on their own, but just improving code readability is quite nice too imo
 3717:08 <stickies-v> so, diving into the conceptual side of things a bit
 3817:08 <stickies-v> 2. What does it mean for a transaction identifier to be type-safe? Why is that important or helpful? Are there any downsides?
 3917:08 <larryruane_> before I approach ACK, I'm trying to figure out if it would be better for a `Txid` and a `Wtxid` to *inheret* from `uint256`, or for them to simply *include* a `uint256` (and nothing more, you could call this wrapping)
 4017:09 <BrandonOdiwuor> A 'transaction identifier' is considered 'type-safe' when it undergoes rigorous type-checking during compile-time, guaranteeing that operations involving these identifiers are thoroughly validated and constrained to their intended data types before the program is executed, thus preventing type-related errors and ensuring program reliability.
 4117:09 <BrandonOdiwuor> Some advantages include: Early detection of type-errors at compile time and preventing runtime errors
 4217:10 <larryruane_> type safe means the two kinds of transaction IDs have distinct types (instead of both being `uint256`), so trying to assign one type to a variable of another type (for example) is like trying to assign a `float` to an `int`
 4317:10 <stickies-v> larryruane_: I have had similar thoughts. I think inheritance makes the transition easier, i.e. if we don't want to update all places that use a transaction identifier
 4417:10 <stickies-v> but ignoring that it seems like just having a `uint256` member seems like it would simplify the implementation quite a bit
 4517:11 <larryruane_> @stickies-v thanks, that's my suspicion too, but wasn't sure, want to investigate further
 4617:12 <abubakarsadiq> there are two types of transaction identifiers in post segwit bitcoin, txid and wtxid.
 4717:12 <abubakarsadiq> To avoid ambiguity and logic error where both are interchanged, it will be better if they are distinct types
 4817:12 <abubakarsadiq> The code won't compile if one is used in the place of another.
 4917:13 <larryruane_> a C codebase i worked on years ago had a really cool concept of "wrapped" integers, so you couldn't assign a block count (this was a storage system) to a byte count, etc. .... so similar goal as here, but it used the wrapped approach (since there's no inheretence in C)
 5017:13 <stickies-v> BrandonOdiwuor: would you say the error catching happens mostly at compile time or at runtime?
 5117:13 <maxedwards> can you elaborate on the simplicity enabled by having an internal uint256 vs subclassing?
 5217:13 <stickies-v> larryruane_: is type safety guaranteed by just having two distinct types, though?
 5317:14 <BrandonOdiwuor94> stickies-v it would be better to catch the errors at compile-time rather than do runtime assertions to catch the errors
 5417:14 <larryruane_> no, a block count type was a C struct with just one member, conventionally called `i` (for `integer`), and the byte count was a different C struct (also with just `i`) ... so you couldn't assign one to the other for example
 5517:15 <maxedwards> simply providing the types is not enough, they have to be consumed
 5617:15 <Murch[m]> I assume that with inheritance assignments might permit to autoconvert between the types without complaining
 5717:15 <stickies-v> BrandonOdiwuor94: yeah, catching stuff at compile time is great - makes it quick for the developer to spot their error as they are developing, as opposed to relying on writing extensive test suites etc (that still may miss things) to catch things in runtime
 5817:15 <larryruane_> i.e. so it wasn't just two typedefs (which does NOT give type safety)
 5917:16 <stickies-v> larryruane_: i don't just mean typedefs: in a naive implementation, just having Txid and Wtxid each subclass uint256 does not provide type safety on its own, as e.g. we can still do comparisons between them
 6017:16 <larryruane_> yeah trying to catch things are runtime is tricky, you might never happen to go through the buggy code path! (but for sure one of our users will!)
 6117:17 <stickies-v> well, it does provide some type safety, just not entirely
 6217:17 <stickies-v> Murch[m]: yes exactly@
 6317:17 <maxedwards> Murch[m]: I believe there is a comment about doing just that in the PR
 6417:18 <stickies-v> maxedwards: so the complexity that I was talking about is that currently we have to specify which kinds of comparisons etc we allow, because currently both `Txid` and `Wtxid` inherit from `uint256`
 6517:19 <stickies-v> I guess we already largely covered the next question but I'll throw it in here regardless in case anyone has something to add:
 6617:19 <larryruane_> to me, with my not-great understanding of subclassing, the wrapping approach seems simpler and safer -- two classes both with just one member, a `uint256` (like we did with the wrapped integers in C) ... but never mind, we can move on, sorry to distrupt
 6717:19 <stickies-v> 3. Why is it better to enforce types at compile-time instead of at run-time?
 6817:19 <glozow> find bug moar fast.
 6917:19 <BrandonOdiwuor> Early Error Detection: Compile-time checking allows for the identification of type-related errors and mismatches at compile time before the program is even executed. This means that issues are caught during the development and testing phase, reducing the chances of runtime errors and unexpected program behavior.
 7017:20 <larryruane_> stickies-v: kind of answered already, but it prevents coding mistakes where the wrong kind of ID is used (txid instead of wtxid or the reverse)
 7117:20 <stickies-v> larryruane_: but with wrapping, would you still be able to not update all callsites of functions that return a Txid/Wtxid right away?
 7217:20 <stickies-v> I think there's merit to allowing this to be a gradual improvement
 7317:20 <stickies-v> glozow: much accurate
 7417:20 <larryruane_> @stickies-v yes very true! that's an important consideration
 7517:21 <stickies-v> BrandonOdiwuor: yeah it's generally much quicker
 7617:22 <BrandonOdiwuor> I saw this comment on the notes (https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261648754) I think it would help with this
 7717:23 <glozow> yeah I write this bug approximately once every 2 weeks
 7817:23 <maxedwards> assuming less than perfect test coverage and a bug, a runtime check could end up throwing on production
 7917:23 <maxedwards> worst case and I'm sure with the scrutiny of bitcoin it would be caught in an earlier stage
 8017:24 <glozow> Usually I catch it before I push a PR tho. always need to have a "try 2 transactions with same txid but diff witness" test
 8117:24 <glozow> catching at compile time would be nice
 8217:25 <stickies-v> maxedwards: even if it can be tested, being able to remove tests because it's checked at compile time is a big win
 8317:25 <abubakarsadiq> glozow: with this you dont have to do that right \o/?
 8417:25 <stickies-v> launching the next question already - but as always, feel free to continue the conversation on previous questions too!
 8517:25 <stickies-v> 4. Conceptually, when writing new code that requires referring to transactions, when should you use `txid` and when should you use `wtxid`? Can you point to any examples in the code where using one instead of the other could be very bad? (1 point per example)
 8617:26 <lightlike> seems like testing this would still be useful. having type safety won't prevent us from using the wrong type of id (but consistently) in the first place.
 8717:27 <larryruane_> stickies-v: not sure if this is what you're asking, but two pure segwit transactions with the same txid (even if different wtxids) have the same *effect* (modify the UTXO set the same way)
 8817:27 <larryruane_> lightlike: +1
 8917:27 <glozow> lightlike +1
 9017:28 <larryruane_> (pure meaning *all* the inputs are segwit)
 9117:28 <glozow> this is a good example imo: https://github.com/bitcoin/bitcoin/blob/3cd02806ecd2edd08236ede554f1685866625757/src/net_processing.cpp#L4334
 9217:28 <sipa> larryruane_: two non-segwit transactions with the same txid also have the same effect :p
 9317:28 <glozow> if you use txid, that means I can censor any tx I want to you by sending you a version with a mutated witness first
 9417:29 <stickies-v> glozow: 1 point!
 9517:30 <larryruane_> sipa: oh right... two tx with different txids could have the same effect (i think??)
 9617:30 <larryruane_> (that's malleability)
 9717:30 <sipa> larryruane_: yes
 9817:30 <sipa> the same txid implies the same effect, but different txid does not imply different effect
 9917:31 <lightlike> so "when in doubt use wtxid" is a good role of thumb i guess?
10017:31 <stickies-v> so even if you haven't found any examples, conceptually - what do you think is a good heuristic for when to use wtxid instead of txid, or txid instead of wtxid?
10117:32 <stickies-v> lightlike: that seems a reasonable starting point :-D
10217:33 <glozow> pretty much always wtxid unless it's prevout-related
10317:33 <sipa> or tx relay for pre-BIP339 peers
10417:33 <larryruane_> stickies-v: in the mempool, we definitely want to use txids as keys, because if we used wtxid, two tx with the same txid could be there simultaneously, and if that happened, an output from both of them could be spent separately, leading to double-spend bug?
10517:34 <stickies-v> glozow: yeah, and also almost always wtxid when it's about unconfirmed txs?
10617:35 <larryruane_> stickies-v: that sounds opposite to what i said? (but i may be wrong)
10717:36 <abubakarsadiq> we could have identical transaction with multiple wtxid's using wtxid almost everytime, will that not bring duplication especially for unconfirmed txs?
10817:36 <stickies-v> larryruane_: well i think gloria covered that already by it being prevout related?
10917:36 <sipa> the mempool is indexed by both txid and wtxid, because we need to be able to look up by both
11017:36 <glozow> unsure if unconfirmed txs are special. But one distinction that springs to mind is you should never try to cache feerate to a txid
11117:37 <glozow> I think larryruane's point might be that having an index by txid can be a useful way to bake in the assumption that no 2 transactions in the data structure should have the same txid
11217:37 <larryruane_> sipa: gotcha, but we definitely don't want to allow > 1 tx with the same txid
11317:38 <maxedwards> am I right in thinking that in theory this same/different effect shouldn't be a concern for this PR as the existing code should already be using the correct id, it's just that it's represented by the same type?
11417:39 <lightlike> larryruane: we don't want conflicting transactions in general, even if they don't share the txid.
11517:39 <sipa> my guess is actually that if we dropped the unicity of the txid index in the mempool, nothing would break, as different-wtxid-same-txid transactions would still just conflict with each other
11617:39 <sipa> the unicity just allows enforcing that constraint independently
11717:39 <sipa> uniqueness? unicity?
11817:40 <stickies-v> maxedwards: this is more conceptual discussion about txid/wtxid indeed and not directly critical for the PR
11917:40 <larryruane_> lightlike: oh yes that's right, as glozow said, the prevout is txid-based so would be the same
12017:40 <sipa> (tangent, english stackexchange says "I suspect most people would take "Unicity" to be a town where everyone rides unicycles.")
12117:40 <maxedwards> stickies-v: very interesting still
12217:41 <larryruane_> i took bicycle riding lessons but ran out of money halfway through, so now i can only ride a unicycle
12317:41 <glozow> sipa: tbh I would have thought of it as the british equivalent of "college town"
12417:41 <glozow> larryruane: are there 100 seats on 100 unicycles...
12517:42 <sipa> :D
12617:42 <stickies-v> alright time to move on ๐Ÿ˜…
12717:42 <sipa> glozow: and they ride one by one into a room with 100 boxes, ...
12817:42 <stickies-v> 5. In which concrete way(s) could using `transaction_identifier` instead of `uint256` help find existing bugs or prevent the introduction of new ones? On the other hand, could this change introduce new bugs?
12917:42 <glozow> ๐Ÿ˜‚
13017:43 <larryruane_> stickies-v: is it because lots of other things besides txids and wtxids can be uint256 type? so using `transaction_identifier` would exclude those accidental uses?
13117:45 <BrandonOdiwuor88> larryruane_ I think it would help prevent mixing up using Txid and Wtxid unintentionally such as the comment that was linked in the notes(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261648754)
13217:45 <stickies-v> larryruane_: yup. a function that takes a `uint256 hash` as an argument can specify in the docs that `hash` is supposed to be a wtxid, but if a developer (and the reviewers) don't read that properly then it's trivial to pass a txid and potentially (probably) break things
13317:46 <glozow> fun anecdote related to this: one time while working in wallet code, I saw a variable named `wtxid` and assumed it was a witness ID. nope, it stood for "wallet transaction id"
13417:47 <stickies-v> on the other hand - does having this explicit type potentially introduce other, new bugs? can you think of any?
13517:47 <stickies-v> glozow: oooh yeah those are sneaky
13617:47 <maxedwards> can you pass a uint256 to a function that asks for a txid?
13717:47 <maxedwards> or do you have to cast?
13817:47 <larryruane_> i just noticed that there are 1687 occurrences of `uint256` in our code base -- many (maybe most) of which are not transaction ID related!
13917:49 <dergoegge> maxedwards: you would have to use `Txid::FromUint256`
14017:49 <glozow> maxedwards: try and see if it compiles!
14117:50 <stickies-v> maxedwards: see also question 16 that we'll be dealing with tomorrow!
14217:50 <lightlike> dergoegge: did you try to change it everywhere, just to see how big the effort is?
14317:50 <maxedwards> even with this, would it not be possible to change a parameter to a txid when it should be a wtxid and everything would still work?
14417:50 <maxedwards> the uint256 was representing a wtxid
14517:51 <maxedwards> but the type isn't actually enforcing that it is
14617:51 <maxedwards> and could lead to much confusion
14717:51 <maxedwards> even more confusion than had it been left as a uint256
14817:51 <BrandonOdiwuor88> maxedwards I think the compiler would throw an error you have to cast explicitly with reinterpret_cast
14917:51 <maxedwards> yes so I'm assuming you do cast
15017:52 <stickies-v> maxedwards: what do you mean with change a parameter? the function signature?
15117:52 <maxedwards> but there's nothing stopping you from casting a uint256 that represented a wtxid to a txid is there?
15217:52 <maxedwards> yes function signature
15317:52 <stickies-v> well obviously the function signature has to be correct, if you need a wtxid and have your function signature take a `Txid txid` then that's going to lead to problems
15417:53 <maxedwards> not compile time problems though is my question
15517:53 <dergoegge> lightlike: no haven't tried, my guess would be that it is a bunch of effort but also not to crazy
15617:54 <stickies-v> it depends on the callsites of course. sorry i'm not sure i fully understand your question - type-safety doesn't prevent a developer from writing wrong code, indeed
15717:55 <vmammal> dergoegge do you plan to continue the refactor in a future PR?
15817:55 <dergoegge> `Txid::FromUint256(Wtxid{})` might actually work because `uint256 a = Wtxid{}` is allowed.
15917:55 <stickies-v> gonna skip a few questions as we're running out of time
16017:55 <dergoegge> vmammal: yes but I'd also be happy to see other people going for it
16117:55 <stickies-v> 8. The `GenTxid` class already exists. How does it already enforce type correctness, and how is it different from the approach in this PR?
16217:55 <stickies-v> (link: https://github.com/bitcoin/bitcoin/blob/dcfbf3c2107c3cb9d343ebfa0eee78278dea8d66/src/primitives/transaction.h#L425)
16317:58 <dberkelmans> It also contains a bool saying if the witness is included
16417:58 <dberkelmans> It's one type instead of 2
16517:58 <dberkelmans> So it doesn't make the code more readable
16617:59 <larryruane_> for one thing, an instance of a txid and a wtxid (contained within a GenTxid) compare as false, even if the hashes are true ... but there's more I'm sure
16717:59 <stickies-v> dberkelmans: if it doesn't make the code more readable, should we then replace it entirely with transaction_identifier?
16818:00 <BrandonOdiwuor88> I think GenTxid uses uint256 to store the hash and the bool m_is_wtxid to indicate whether its Wtxid or Txid
16918:00 <dberkelmans> I haven't seen enough code to say anything sensible about that
17018:01 <stickies-v> I think an important difference is that GenTxid allows type checking, by exposing the `IsWtxid()` function, but it's still on the user to do that checking
17118:01 <stickies-v> and more importantly, it happens at runtime
17218:01 <dberkelmans> I suppose if you only need the uint256 then having it in one type makes sense
17318:02 <larryruane_> stickies-v: so it's runtime checking rather than compile-time
17418:02 <stickies-v> with transaction_identifer, the type checking is enforced at compile time
17518:02 <stickies-v> larryruane_: yeah
17618:02 <stickies-v> dberkelmans: well one important use case that GenTxid solves is that in quite a few places we want to be able to take input that can be either a txid or a wtxid, as opposed to just one or the other
17718:03 <dergoegge> If we go for the type safe ids, my thinking was that we would replace `GenTxid` with `std::variant<Txid, Wtxid>`
17818:03 <stickies-v> actually, from my quick skimming it seems like *all* the places where we use GenTxid we want to be able to accept both types (which surprised me a little)
17918:03 <stickies-v> so, we're at time for this meeting
18018:03 <stickies-v> #endmeeting