dergoegge
commented at 2:10 pm on July 19, 2023:
member
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
commented at 2:10 pm on July 19, 2023:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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
commented at 2:17 pm on July 19, 2023:
member
How is this different from GenTxid?
dergoegge
commented at 2:28 pm on July 19, 2023:
member
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
commented at 10:54 am on July 20, 2023:
contributor
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
commented at 12:28 pm on July 20, 2023:
member
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
commented at 12:29 pm on July 20, 2023:
member
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
commented at 3:24 pm on July 20, 2023:
member
DrahtBot added the label
Utils/log/libs
on Jul 21, 2023
dergoegge force-pushed
on Jul 21, 2023
DrahtBot added the label
CI failed
on Jul 21, 2023
dergoegge force-pushed
on Jul 21, 2023
dergoegge marked this as ready for review
on Jul 21, 2023
dergoegge
commented at 3:31 pm on July 21, 2023:
member
Ready for review
dergoegge force-pushed
on Jul 21, 2023
TheCharlatan
commented at 4:30 pm on July 21, 2023:
contributor
Concept ACK
DrahtBot removed the label
CI failed
on Jul 21, 2023
luke-jr
commented at 6:26 pm on July 25, 2023:
member
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
commented at 12:51 pm on July 26, 2023:
member
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.
in
src/util/transaction_identifier.h:65
in
76fe2c3042outdated
46+};
47+
48+/** Txid commits to all transaction fields except the witness. */
49+using Txid = transaction_identifier<false>;
50+/** Wtxid commits to all transaction fields including the witness. */
51+using Wtxid = transaction_identifier<true>;
That is what I originally had in this PR but it actually doesn’t achieve the same on all fronts, so I decided to make it explicit to avoid subtle bugs.
e.g. the following was still permitted:
0Txid txid;
1Wtxid wtxid{txid};
2// or
3wtxid.Compare(txid);
4// or
5bool equal = (wtxid == txid); // same for !=
instagibbs
commented at 4:14 pm on August 3, 2023:
member
ACK76fe2c304201646d63b1b01b397e18a99252a2d9
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.
dergoegge
commented at 1:40 pm on August 7, 2023:
member
@MarcoFalke could you give this another look? You’ve done similar work on our time type-safety, so I’d appreciate your feedback.
in
src/util/transaction_identifier.h:54
in
db8d42b562outdated
49+using Txid = transaction_identifier<false>;
50+/** Wtxid commits to all transaction fields including the witness. */
51+using Wtxid = transaction_identifier<true>;
52+
53+#endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H
54+
The “opposite” type would be any type other than the type itself. For example, currently it is possible to compare uint256 (a txid) with Wtxid, or uint256 (a wtxid) with Txid.
dergoegge
commented at 10:33 am on August 8, 2023:
I’m OK with going either way but disallowing uint256 comparisons in this PR causes a lot of churn, i.e. going around casting all the uint256 to the right type. Most of these casts would later be reverted in follow up PRs that convert more things to use the new types (e.g. mempool, net processing, wallet).
I think I’d prefer doing the switch to the new type system first and then disallowing uint256 comparisons afterwards. What do you think?
Any reason to delete these? The compiler already has them deleted and provides a better error message than yours: candidate function (the implicit move assignment operator) not viable: no known conversion from 'transaction_identifier<true>' to 'transaction_identifier<false>' for 1st argument.
Would be better to just remove those lines, unless there is a reason to do add lines here.
Apart from compare, assignment isn’t type safe either. For example:
0uint256 b = Txid{};
1Wtxid a {b};
compiles.
So my preference would be to add a comment that this is allowed, and remove this code that tries to imply the opposite and could be confusing reviewers.
Alternatively it could be disallowed properly.
dergoegge
commented at 10:59 am on August 9, 2023:
Alternatively it could be disallowed properly.
converting from uint256 can now only be done using {Txid,Wtxid}::FromUint256. This required less changes then expected so I think that is actually preferable.
Yea I think this makes sense while we are converting, otherwise I suspect there’ll be more churn (similar to the comparisons).
It is fine, if reviewers prefer this. Though, I wouldn’t call it “type-safe” and it would be better to at least add a comment about the trade-offs you are making.
But how would you suggest to forbid this?
It can be forbidden by not allowing it. If you are asking how to do it in C++, something like this should work:
0classtransaction_identifier{
1const uint256 m_data;
2public:3// TODO implement methods that are allowed
4};
dergoegge
commented at 1:32 pm on September 28, 2023:
Added a comment to transaction_identifier to note that it is not fully type-safe yet due to allowing implicit conversions to uint256 in the interim.
dergoegge force-pushed
on Aug 9, 2023
dergoegge force-pushed
on Aug 9, 2023
DrahtBot added the label
CI failed
on Aug 9, 2023
DrahtBot removed the label
CI failed
on Aug 9, 2023
dergoegge force-pushed
on Aug 17, 2023
dergoegge closed this
on Aug 17, 2023
dergoegge reopened this
on Aug 17, 2023
DrahtBot added the label
CI failed
on Aug 21, 2023
maflcko
commented at 1:49 pm on August 28, 2023:
member
Maybe mark as draft as long as CI is red?
DrahtBot added the label
Needs rebase
on Aug 31, 2023
dergoegge marked this as a draft
on Sep 11, 2023
glozow
commented at 1:02 pm on September 28, 2023:
member
Rebase pls?
dergoegge force-pushed
on Sep 28, 2023
dergoegge force-pushed
on Sep 28, 2023
dergoegge marked this as ready for review
on Sep 28, 2023
DrahtBot removed the label
Needs rebase
on Sep 28, 2023
DrahtBot removed the label
CI failed
on Sep 28, 2023
dergoegge
commented at 4:32 pm on September 28, 2023:
member
Can someone restart the win64 job? failure is unrelated
DrahtBot added the label
Needs rebase
on Oct 2, 2023
dergoegge force-pushed
on Oct 2, 2023
dergoegge
commented at 11:57 am on October 2, 2023:
member
Rebased
DrahtBot removed the label
Needs rebase
on Oct 2, 2023
in
src/net_processing.cpp:5701
in
f313245722outdated
nit, may be slightly more efficient, since the hash within inv wouldn’t be getting set and then overwritten in the witness case. Also arguably slightly more readable.
mzumsande
commented at 6:51 pm on October 4, 2023:
We need a different inv type MSG_WTX for wtxids!
Although the order could be switched (first create MSG_WTX and maybe overwrite that), since wtxid-based relay should be the standard by now and txid the exception.
dergoegge
commented at 10:19 am on October 6, 2023:
Switched the order
ismaelsadeeq
commented at 3:07 pm on October 4, 2023:
member
Concept ACK
BrandonOdiwuor
commented at 4:03 pm on October 4, 2023:
contributor
Concept ACK
The introduction of transaction_identifier (including Txid and Wtxid) is a valuable enhancement that enforces compile-time checks. This proactive approach will significantly reduce the likelihood of runtime bugs caused by inadvertently confusing Txid and Wtxid. It also adds more clarity to the codebase.
in
src/util/transaction_identifier.h:26
in
f313245722outdated
dergoegge
commented at 12:29 pm on October 9, 2023:
Done
in
src/util/transaction_identifier.h:14
in
c4c7b6ea9foutdated
9+/** transaction_identifier represents the two canonical transaction identifier
10+ * types (txid, wtxid).
11+ *
12+ * TODO: This type is not fully type-safe yet as we allow implicit conversion
13+ * to uint256. We should make it impossible to implicitly convert from
14+ * transaction_identifier to uint256 (e.g. by making the base class private).
Generally, I think it would be good to be as close as possible to the final result, and only have additional (temporary) lines, which are marked for deletion, with a reason for deletion.
dergoegge
commented at 1:17 pm on October 6, 2023:
Why not make it private and add a method operator uint256, which can be removed easily?
I’m getting the following error for this: Conversion function converting 'transaction_identifier<has_witness>' to its base class 'uint256' will never be used
Otherwise, making it private could hide other member methods as well, making the transition harder?
We can just expose the required methods with using uint256::begin; etc.
in
src/util/transaction_identifier.h:9
in
c4c7b6ea9foutdated
12+ * TODO: This type is not fully type-safe yet as we allow implicit conversion
13+ * to uint256. We should make it impossible to implicitly convert from
14+ * transaction_identifier to uint256 (e.g. by making the base class private).
15+ * This should be easy to do once most of the code base has converted to the
16+ * Txid and Wtxid types to avoid churn. */
17+template <bool has_witness>
stickies-v
commented at 1:44 pm on October 6, 2023:
Given that has_witness is only used for templating, what do you think about this approach? I think it’s a bit more straightforward, and as a probably not-very-useful benefit, it also allows for adding more txid types when necessary.
0diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
1index b5c0f25093..a845c5c0f9 100644
2--- a/src/util/transaction_identifier.h
3+++ b/src/util/transaction_identifier.h
4@@ -14,7 +14,7 @@
5 * transaction_identifier to uint256 (e.g. by making the base class private).
6 * This should be easy to do once most of the code base has converted to the
7 * Txid and Wtxid types to avoid churn. */
8-template <bool has_witness>
9+template <typename Derived>
10 class transaction_identifier : public uint256
11 {
12 public:
13@@ -24,8 +24,7 @@ public:
14 transaction_identifier(const Other& other)
15 : uint256{other}
16 {
17- static_assert(std::is_same_v<Other, transaction_identifier<has_witness>>,
18- "Forbidden copy type");
19+ static_assert(std::is_same_v<Other, Derived>, "Forbidden copy type");
20 }
2122 // Allow comparison with the same transaction_identifier type
23@@ -39,7 +38,7 @@ public:
24 constexpr int Compare(const Other& other) const
25 {
26 if constexpr (std::is_same_v<Other, uint256> || // TODO forbid uint256 comparisons
27- std::is_same_v<Other, transaction_identifier<has_witness>>) {
28+ std::is_same_v<Other, Derived>) {
29 return reinterpret_cast<const uint256&>(*this).Compare(other);
30 } else {
31 static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
32@@ -49,12 +48,12 @@ public:
33 uint256 Uint256() const { return reinterpret_cast<const uint256&>(*this); }
3435 template <typename Other>
36- static transaction_identifier FromUint256(const Other& other)
37+ static Derived FromUint256(const Other& other)
38 {
39 // TODO this does not need to be a template function after we disallow
40- // `uint256 a = transaction_identifier<has_witness>{};`
41+ // `uint256 a = Derived{};`
42 if constexpr (std::is_same_v<Other, uint256>) {
43- return reinterpret_cast<const transaction_identifier&>(other);
44+ return reinterpret_cast<const Derived&>(other);
45 } else {
46 static_assert(ALWAYS_FALSE<Other>, "FromUint256 only allows uint256");
47 }
48@@ -62,8 +61,8 @@ public:
49 };
5051 /** Txid commits to all transaction fields except the witness. */
52-using Txid = transaction_identifier<false>;
53+class Txid : public transaction_identifier<Txid> {};
54 /** Wtxid commits to all transaction fields including the witness. */
55-using Wtxid = transaction_identifier<true>;
56+class Wtxid : public transaction_identifier<Wtxid> {};
5758 #endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H
dergoegge
commented at 12:30 pm on October 9, 2023:
I prefer the bool, allowing for more txid types would trivial in any case.
stickies-v
commented at 3:59 pm on October 6, 2023:
contributor
Approach ACKc4c7b6ea9fe2e7ba89a994127073c54af30d6f88(did a fair amount of code review too so not expecting too many more comments there, looks great)
I think the gradual rollout is definitely the way to go.
I’ve been trying to see if using a wrapped uint256 m_hash (without inheritance) makes more sense, but given that we want all transaction_identifier subclasses and uin256 have an identical but non-interchangeable interface, I just ended up with more overhead, so that seems like a no-go.
Private inheritance seems the most natural approach in the long term, but seems like it is incompatible with a gradual rollout, as we can’t use a conversion operator.
So, I think it makes sense to stick with the public inheritance approach used now, and switch to private inheritance once all code is updated to the stricter types. This switch should be very limited in scope anyway, so I don’t see an issue with doing things gradually.
[net processing] Use HasWitness over comparing (w)txidscdb14d79e8
dergoegge force-pushed
on Oct 9, 2023
dergoegge
commented at 12:29 pm on October 9, 2023:
member
Changed the approach, so that Wtxid and Txid are now wrapped uint256s. A conversion function to uint256 was added to allow for smoother transition to these types. Once we have converted most of our code we can simply delete the conversion function for full type-safety. @maflcko this is what you suggested quite early on, what do you think?
in
src/util/transaction_identifier.h:46
in
f35638f72coutdated
dergoegge
commented at 9:58 am on October 12, 2023:
Ok great gonna include this
maflcko approved
maflcko
commented at 12:39 pm on October 9, 2023:
member
lgtm
DrahtBot added the label
CI failed
on Oct 9, 2023
in
src/util/transaction_identifier.h:63
in
f35638f72coutdated
58+ * Note: new code should use `ToUint256`.
59+ *
60+ * TODO: This should be removed once the majority of the code has switched
61+ * to using the Txid and Wtxid types. Until then it makes for a smoother
62+ * transition to allow this conversion. */
63+ operator const uint256&() const { return m_wrapped; }
You can try (for fun) LIEFTIMEBOUND, to see if it catches this? In any case, you’ll probably have to return a copy here, regardless of the approach?
dergoegge
commented at 3:38 pm on October 9, 2023:
It now returns a copy, which results in a few compiler errors/warnings but those were trivial to fix.
dergoegge
commented at 3:44 pm on October 9, 2023:
You can try (for fun) LIEFTIMEBOUND, to see if it catches this?
It actually does, good stuff!
dergoegge force-pushed
on Oct 9, 2023
dergoegge force-pushed
on Oct 9, 2023
DrahtBot removed the label
CI failed
on Oct 9, 2023
in
src/util/transaction_identifier.h:57
in
c0e7f86c66outdated
56+ /** Conversion function to `uint256`.
57+ *
58+ * Note: new code should use `ToUint256`.
59+ *
60+ * TODO: This should be removed once the majority of the code has switched
61+ * to using the Txid and Wtxid types. Until then it makes for a smoother
Question about this todo: are we expecting to change everything to Txid or Wtxid? I imagine that something like m_recent_confirmed_transactions will always hold both?
dergoegge
commented at 10:51 am on October 10, 2023:
are we expecting to change everything to Txid or Wtxid?
I think almost everything using uint256 for transaction ids will be converted (including GenTxid which can just be a std::variant<Txid, Wtxid>) but there are probably a few exceptions.
I don’t expect m_recent_confirmed_transactions or m_recent_rejects etc. to change as the underlying data structure (bloom filter) does not deal with txids, it simply accepts some bytes (Span<const uint8_t>). So converting the new types to a span (e.g. using ToUint256) should be fine.
instagibbs
commented at 3:49 pm on October 11, 2023:
If a specific bloom filter is only meant to take one or the other I could potentially see them using the types more directly, but that would be a case by case basis clearly.
glozow
commented at 9:22 am on October 10, 2023:
member
Instead of adding the include here, I’d say it could make sense to export it from primitives/transaction. There shouldn’t be a reason to having to include both.
dergoegge
commented at 10:08 am on October 12, 2023:
The identifiers are now exported from primitives/transaction.
in
src/util/transaction_identifier.h:26
in
68b9c2f268outdated
stickies-v
commented at 2:42 pm on October 11, 2023:
Since we’re no longer inheriting, could avoid using type_traits and just overload Compare? Less complex, imo. I think the templated Compare could even be dropped completely and replaced with a docstring? (but included in this diff for completeness)
0diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
1index c832895ff7..4427b34427 100644
2--- a/src/util/transaction_identifier.h
3+++ b/src/util/transaction_identifier.h
4@@ -4,8 +4,6 @@
5 #include <uint256.h>
6 #include <util/types.h>
7 8-#include <type_traits>
9-
10 /** transaction_identifier represents the two canonical transaction identifier
11 * types (txid, wtxid).*/
12 template <bool has_witness>
13@@ -15,20 +13,18 @@ class transaction_identifier
1415 transaction_identifier(const uint256& wrapped) : m_wrapped{wrapped} {}
1617+ /** TODO: Comparisons with uint256 should be disallowed once we have converted most of the code
18+ * to using the new txid types. */
19+ constexpr int Compare(const uint256& other) const { return m_wrapped.Compare(other); }
20+ constexpr int Compare(const transaction_identifier<has_witness>& other) const { return m_wrapped.Compare(other.m_wrapped); }
21 template <typename Other>
22- constexpr int Compare(const Other& other) const
23+ constexpr int Compare(const Other&) const
24 {
25- if constexpr (std::is_same_v<Other, uint256>) {
26- // TODO: Comparisons with uint256 should be disallowed once we have
27- // converted most of the code to using the new txid types.
28- return m_wrapped.Compare(other);
29- } else if constexpr (std::is_same_v<Other, transaction_identifier<has_witness>>) {
30- return m_wrapped.Compare(other.m_wrapped);
31- } else {
32- static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
33- }
34+ static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
35+ return 0; // never reached, but doesn't compile without
36 }
3738+
39 public:
40 transaction_identifier() : m_wrapped{} {}
41
dergoegge
commented at 10:08 am on October 12, 2023:
Done
in
src/txorphanage.cpp:163
in
68b9c2f268outdated
159@@ -159,7 +160,7 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
160 for (const auto& elem : it_by_prev->second) {
161 // Get this source peer's work set, emplacing an empty set if it didn't exist
162 // (note: if this peer wasn't still connected, we would have removed the orphan tx already)
163- std::set<uint256>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second;
164+ std::set<Txid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second;
stickies-v
commented at 3:38 pm on October 11, 2023:
nit: That’s a lot of first’s and second’s. This PR doesn’t make it worse, but if you wanna sneak in some readability improvements I’d very happily support that, e.g.
0diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp
1index d4c07b0543..80ae4af662 100644
2--- a/src/txorphanage.cpp
3+++ b/src/txorphanage.cpp
4@@ -158,13 +158,16 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
5 const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i));
6 if (it_by_prev != m_outpoint_to_orphan_it.end()) {
7 for (const auto& elem : it_by_prev->second) {
8+ const auto& txid = elem->first;
9+ const auto& orphan_tx = elem->second;
10 // Get this source peer's work set, emplacing an empty set if it didn't exist
11 // (note: if this peer wasn't still connected, we would have removed the orphan tx already)
12- std::set<Txid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second;
13+ auto [it, inserted] = m_peer_work_set.try_emplace(orphan_tx.fromPeer);
14+ std::set<Txid>& orphan_work_set = it->second;
15 // Add this tx to the work set
16- orphan_work_set.insert(elem->first);
17+ orphan_work_set.insert(txid);
18 LogPrint(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n",
19- tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), elem->second.fromPeer);
20+ tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), orphan_tx.fromPeer);
21 }
22 }
23 }
instagibbs
commented at 5:00 pm on October 11, 2023:
@stickies-v every time I read this function I have to rediscover whatever the heck it’s doing(at least 6 times now), strong concept ACK on a follow-up PR to clean it up
wdym readability. You look up the elem corresponding to the outpoint in this map, and elem refers to an iterator into another map, which you dereference to get the value of the OrphanTx object, so that you can extract the key to look up in a third map, so that you can obtain a reference to the element you just inserted or was already there.
dergoegge
commented at 10:09 am on October 12, 2023:
Not gonna touch that in this PR
stickies-v approved
stickies-v
commented at 3:45 pm on October 11, 2023:
contributor
ACK68b9c2f268e7a924f4c9182ab1b99c30af86f5d7
The wrapped approach is less overhead than I thought it would be, and makes for a pretty simple implementation. Nice.
DrahtBot requested review from BrandonOdiwuor
on Oct 11, 2023
DrahtBot requested review from glozow
on Oct 11, 2023
DrahtBot requested review from luke-jr
on Oct 11, 2023
DrahtBot requested review from TheCharlatan
on Oct 11, 2023
DrahtBot requested review from ismaelsadeeq
on Oct 11, 2023
DrahtBot removed review request from BrandonOdiwuor
on Oct 11, 2023
DrahtBot requested review from BrandonOdiwuor
on Oct 11, 2023
DrahtBot removed review request from BrandonOdiwuor
on Oct 11, 2023
DrahtBot requested review from BrandonOdiwuor
on Oct 11, 2023
in
src/net_processing.cpp:5697
in
c0e7f86c66outdated
stickies-v
commented at 10:46 am on October 12, 2023:
nit: no longer needed here. In txorphanage.cpp and test/orphanage_tests.cpp, #include <util/transaction_identifier.h> should be replaced with #include <primitives/transaction.h> since we need that include already anyway.
dergoegge force-pushed
on Oct 12, 2023
in
src/util/transaction_identifier.h:25
in
940a49978c
DrahtBot removed review request from BrandonOdiwuor
on Oct 12, 2023
DrahtBot requested review from BrandonOdiwuor
on Oct 12, 2023
hebasto approved
hebasto
commented at 12:57 pm on October 12, 2023:
member
ACK940a49978c70453e1aaf2c4a0bcb382872b844a5, I have reviewed the code and it looks OK.
nit in 940a49978c70453e1aaf2c4a0bcb382872b844a5: using auto instead of explicit iterator types might make the code more concise, readable and maintainable.
using std::byte instead of unsigned char + needed casts #28107 (review)
glozow
commented at 3:04 pm on October 12, 2023:
member
Assuming we should wait until branch off to merge?
maflcko added this to the milestone 27.0
on Oct 12, 2023
fanquake
commented at 3:09 pm on October 12, 2023:
member
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
commented at 3:19 pm on October 12, 2023:
member
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.
in
src/net_processing.cpp:1855
in
cdb14d79e8outdated
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.
dergoegge
commented at 10:47 am on October 20, 2023:
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)?
ajtowns
commented at 11:14 am on October 20, 2023:
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.
in
src/util/transaction_identifier.h:2
in
ed70e65016outdated
What’s the advantage of FromUint256 over just declaring this as explicit and making it public?
dergoegge
commented at 10:38 am on October 20, 2023:
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…
ajtowns
commented at 11:15 am on October 20, 2023:
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.
in
src/util/transaction_identifier.h:38
in
ed70e65016outdated
in
src/util/transaction_identifier.h:59
in
ed70e65016outdated
54+ * Note: new code should use `ToUint256`.
55+ *
56+ * TODO: This should be removed once the majority of the code has switched
57+ * to using the Txid and Wtxid types. Until then it makes for a smoother
58+ * transition to allow this conversion. */
59+ operator uint256() const { return m_wrapped; }
I’m not sure if handing out a reference to the underlying uint256 is a great idea, i guess it could work with LIFETIMEBOUND.
Copying seems safer and shouldn’t happen all that often once we are using the types in most places.
ajtowns
commented at 11:18 am on October 20, 2023:
Well, this is going away anyway, so sure. Feels to me like foo& x = create_a_new_temporary(); is already a weird thing to do though.
maflcko
commented at 10:57 am on October 27, 2023:
in
src/util/transaction_identifier.h:60
in
ed70e65016outdated
55+ *
56+ * TODO: This should be removed once the majority of the code has switched
57+ * to using the Txid and Wtxid types. Until then it makes for a smoother
58+ * transition to allow this conversion. */
59+ operator uint256() const { return m_wrapped; }
60+};
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-21 06:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
