int32_t type for most transaction size/weight values
#23962
From bitcoin/bitcoin#23957 which has been incorporated into this PR:
A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
Fix that by using per-statement casts.
This refactor doesn’t change behavior because the now explicit casts were previously done implicitly.
Similar to commit 8b5a4de904b414fb3a818732cd0a2c90b91bc275
Concept ACK. Could squash, given that the other pull was closed?
Things to look out for in review:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Xekyo, ryanofsky, achow101, 0xB10C |
| Concept NACK | luke-jr |
| Concept ACK | MarcoFalke, shaavan, glozow, stickies-v |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
Updated 9a8a8adfff029f237a2934c2f9ec5e62644ea8e6 -> 39c82fa26093c7309f7b960959b5b395bba74a10 (pr23962.01 -> pr23962.02).
Could squash, given that the other pull was closed?
Commits have been reordered to keep changes focused.
Concept ACK
I think it makes sense to use int64_t consistently for all transaction sizes.
I have yet to thoroughly review the file to check if all the transaction size variables are correctly covered in this PR; I still find some that could use a proper type assignment.
entry_size can be changed to int64_t. This will lead to making appropriate changes at the other places in the file.totalSizeWithAncestors could be changed.Btw I was curious about one thing. In line 232 in this file, there is implicit typecasting occurring:
0totalSizeWithAncestors += stageit->GetTxSize();
because type of totalSizeWithAncestors is size_t and return type of GetTxSize() is changed to int64_t in this PR (line 114) . Shouldn’t it cause implicit-conversion-warning for this.
Concept ACK to unifying types for transaction sizes. But if we’re going to do this, why use a signed type? We don’t have negative sizes, do we?
The rule of thumb is to use signed types for anything that involves arithmetic operations (addition or subtraction). Obviously the standard library doesn’t make that easy with size_t being unsigned.
Concept ACK to unifying types for transaction sizes. But if we’re going to do this, why use a signed type? We don’t have negative sizes, do we?
Also see Signed and Unsigned Types in Interfaces.
Concept NACK [edit: to 64-bit sizes, which this PR no longer is]. This wastes memory for no benefit - we should probably go to int32_t instead.
Transactions should never approach 2 GB, and it doesn’t make sense to calculate a sum that large either (the largest package that could fit in a block is 4 MB).
If using int32_t produces warnings somewhere, those warnings are probably real issues that should be fixed.
Updated 39c82fa26093c7309f7b960959b5b395bba74a10 -> 295a4f08728213270a0958680594c7548832ccfa (pr23962.02 -> pr23962.03):
This wastes memory for no benefit - we should probably go to
int32_tinstead.
I think it makes sense to use int64_t consistently for all transaction sizes.
You are right. But such a pr could be unmanageable in size. So here, in the first commit, the minimal set of changes included which makes the second commit possible and concise. Other changes left for follow ups.
Changes since my review:
I observed the following implicit conversion warning:
Ideally, this needs to be addressed by changing the ancestor_size_limit to int32_t and making other subsequent changes. But for this PR, we can go with at-place manual conversion, if that’s possible.
@hebasto
You are right. But such a pr could be unmanageable in size. So here, in the first commit, the minimal set of changes included which makes the second commit possible and concise. Other changes left for follow ups.
That makes sense. Let’s take up changing small sections of transaction_size variables one by one in follow-ups.
I observed the following implicit conversion warning:
Must be fixed now.
Changes since my last review:
ancestor_size_limit in CTxMemPool::UpdateForDescendants function changed from uint64_t -> int64_tI tried experimenting with converting the type of ancestor_size_limit to int32_t, and it compiled successfully without warnings.
So what do you think about converting this argument type to int32_t instead of int64_t. This would be in line with our end goal to do so for all transaction size/weight variables?
@hebasto I wonder what you think about modifying https://github.com/bitcoin/bitcoin/blob/a41976ab770d4453ccb043f72e48a6eb15a7106a/src/policy/feerate.cpp#L12
to
0 CFeeRate::CFeeRate(const CAmount& nFeePaid, int32_t num_bytes)
Not necessarily in this PR though.
This change would help me in #21422 (https://github.com/bitcoin/bitcoin/pull/21422/files#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1687) to remove the need for casting.
I don’t know if it is a good idea. Just asking.
Changes since my last review:
Suggestion:
I want to propose one suggestion.
The variables named modifySize deals with transaction size and are added with transaction size variables on multiple occasions. For example:
nSizeWithDescendants += modifySize;
However, its type is kept as int64_t.
This might be a deliberate choice based on the maximum possible size of integer that maybe get stored in it. But in case it’s not, I would suggest changing its type to int32_t.
I was able to successfully compile this PR with the following related diff:
0--- a/src/txmempool.cpp
1+++ b/src/txmempool.cpp
2@@ -25,7 +25,7 @@
3 // Helpers for modifying CTxMemPool::mapTx, which is a boost multi_index.
4 struct update_descendant_state
5 {
6- update_descendant_state(int64_t _modifySize, CAmount _modifyFee, int64_t _modifyCount) :
7+ update_descendant_state(int32_t _modifySize, CAmount _modifyFee, int64_t _modifyCount) :
8 modifySize(_modifySize), modifyFee(_modifyFee), modifyCount(_modifyCount)
9 {}
10
11@@ -33,14 +33,14 @@ struct update_descendant_state
12 { e.UpdateDescendantState(modifySize, modifyFee, modifyCount); }
13
14 private:
15- int64_t modifySize;
16+ int32_t modifySize;
17 CAmount modifyFee;
18 int64_t modifyCount;
19 };
20
21 struct update_ancestor_state
22 {
23- update_ancestor_state(int64_t _modifySize, CAmount _modifyFee, int64_t _modifyCount, int64_t _modifySigOpsCost) :
24+ update_ancestor_state(int32_t _modifySize, CAmount _modifyFee, int64_t _modifyCount, int64_t _modifySigOpsCost) :
25 modifySize(_modifySize), modifyFee(_modifyFee), modifyCount(_modifyCount), modifySigOpsCost(_modifySigOpsCost)
26 {}
27
28@@ -48,7 +48,7 @@ struct update_ancestor_state
29 { e.UpdateAncestorState(modifySize, modifyFee, modifyCount, modifySigOpsCost); }
30
31 private:
32- int64_t modifySize;
33+ int32_t modifySize;
34 CAmount modifyFee;
35 int64_t modifyCount;
36 int64_t modifySigOpsCost;
37@@ -134,7 +134,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan
38 }
39 // descendants now contains all in-mempool descendants of updateIt.
40 // Update and add to cached descendant map
41- int64_t modifySize = 0;
42+ int32_t modifySize = 0;
43 CAmount modifyFee = 0;
44 int64_t modifyCount = 0;
45 for (const CTxMemPoolEntry& descendant : descendants) {
46@@ -387,7 +387,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
47 setEntries setDescendants;
48 CalculateDescendants(removeIt, setDescendants);
49 setDescendants.erase(removeIt); // don't update state for self
50- int64_t modifySize = -((int64_t)removeIt->GetTxSize());
51+ int32_t modifySize = -removeIt->GetTxSize();
52 CAmount modifyFee = -removeIt->GetModifiedFee();
53 int modifySigOps = -removeIt->GetSigOpCost();
54 for (txiter dit : setDescendants) {
55@@ -431,7 +431,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
56 }
57 }
58
59-void CTxMemPoolEntry::UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount)
60+void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount)
61 {
62 nSizeWithDescendants += modifySize;
63 assert(nSizeWithDescendants > 0);
64@@ -440,7 +440,7 @@ void CTxMemPoolEntry::UpdateDescendantState(int64_t modifySize, CAmount modifyFe
65 assert(int64_t(nCountWithDescendants) > 0);
66 }
67
68-void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps)
69+void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps)
70 {
71 nSizeWithAncestors += modifySize;
72 assert(nSizeWithAncestors > 0);
73diff --git a/src/txmempool.h b/src/txmempool.h
74index 7ba9fbefd..fe52995fc 100644
75--- a/src/txmempool.h
76+++ b/src/txmempool.h
77@@ -136,9 +136,9 @@ public:
78 const LockPoints& GetLockPoints() const { return lockPoints; }
79
80 // Adjusts the descendant state.
81- void UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount);
82+ void UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount);
83 // Adjusts the ancestor state
84- void UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps);
85+ void UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps);
86 // Updates the fee delta used for mining priority score, and the
87 // modified fees with descendants/ancestors.
88 void UpdateFeeDelta(CAmount newFeeDelta);
Updated eb64c286e97c10109ff378834f649e1f379978ac -> 03300afb2fdb31a0516cb94dc253e985c35dbce2 (pr23962.07 -> pr23962.08):
Concept ACK. Using 32 bits seems appropriate for sizes of transactions and transaction sets. Under the 100kb standardness rule, an unconfirmed transaction cannot exceed 400,000WU, which means this should never overflow for sets <= (2^31)/400,000 = 5,368 (unconfirmed) transactions which seems sufficient given the current limitations on ancestors and descendants, with room to grow.
Moving to signed integers seems like a worthwhile trade-off: half the range, but increased visibility into unexpected under/overflows.
I’ll do a full review once it’s rebased.
GetBlockWeight, GetTransactionInputWeight, GetVirtualTransactionSize, and GetVirtualTransactionInputSize to also use int32_t?
ISTM we should also change
GetBlockWeight,GetTransactionInputWeight,GetVirtualTransactionSize, andGetVirtualTransactionInputSizeto also useint32_t?
Agree. But I’d rather leave this PR as is, because suggested changes does look trivial and could be left for follow ups. For example, GetVirtualTransactionSize involves arithmetic operations with int64_t nSigOpCost, and additional changes/checks could be required.
366@@ -367,7 +367,7 @@ void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFe
367 nSizeWithDescendants += modifySize;
368 assert(nSizeWithDescendants > 0);
369 nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee);
370- nCountWithDescendants += modifyCount;
371+ nCountWithDescendants += uint64_t(modifyCount);
372 assert(int64_t(nCountWithDescendants) > 0);
In commit “Remove txmempool implicit-integer-sign-change sanitizer suppressions” (93daff4b4577a07994b57218df9bb83bed7bf743)
IIUC, the code here casting modifyCount to be unsigned and adding it even though modifyCount might be negative is safe because the standard guarantees that:
modifyCount to uint64_t produces an equivalent value mod 264uint64_t values is performed mod 264Also, the new code is equivalent to previous code because previous code was just doing the same conversion implicitly.
However, it seems like it be nice to follow up by making nCountWithDescendants signed so the two casts could be dropped. So:
0nCountWithDescendants += uint64_t(modifyCount);
1assert(int64_t(nCountWithDescendants) > 0);
could be replaced with:
0nCountWithDescendants += modifyCount;
1assert(nCountWithDescendants > 0);
I agree with the suggested changes. In addition, it seems reasonable to combine them with making signed nCountWithAncestors and return types of the related getter methods as well. I’d leave such changes for a follow up. Or do you prefer to incorporate them in here?
Silent merge conflict (with -werror)
0../../../src/test/miniminer_tests.cpp: In member function ‘void miniminer_tests::miniminer_1p1c::test_method()’:
1../../../src/test/miniminer_tests.cpp:145:66: error: narrowing conversion of ‘((const boost::operators_impl::dereferenceable<boost::multi_index::detail::hashed_index_iterator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry> > > > > > >, boost::multi_index::detail::bucket_array<std::allocator<CTxMemPoolEntry> >, boost::multi_index::detail::hashed_unique_tag, boost::multi_index::detail::hashed_index_global_iterator_tag>, const CTxMemPoolEntry*, boost::operators_impl::iterator_helper<std::forward_iterator_tag, CTxMemPoolEntry, long int, const CTxMemPoolEntry*, const CTxMemPoolEntry&> >*)(& it))->boost::operators_impl::dereferenceable<boost::multi_index::detail::hashed_index_iterator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry> > > > > > >, boost::multi_index::detail::bucket_array<std::allocator<CTxMemPoolEntry> >, boost::multi_index::detail::hashed_unique_tag, boost::multi_index::detail::hashed_index_global_iterator_tag>, const CTxMemPoolEntry*, boost::operators_impl::iterator_helper<std::forward_iterator_tag, CTxMemPoolEntry, long int, const CTxMemPoolEntry*, const CTxMemPoolEntry&> >::operator->()->CTxMemPoolEntry::GetTxSize()’ from ‘int32_t’ {aka ‘int’} to ‘size_t’ {aka ‘long unsigned int’} [-Werror=narrowing]
2 145 | tx_dims.emplace(tx->GetHash(), TxDimensions{it->GetTxSize(), it->GetModifiedFee(),
3 | ~~~~~~~~~~~~~^~
4cc1plus: all warnings being treated as errors
Following patch fixes the silent merge conflict for me after a rebase:
0--- a/src/test/miniminer_tests.cpp
1+++ b/src/test/miniminer_tests.cpp
2@@ -137,7 +137,7 @@ BOOST_FIXTURE_TEST_CASE(miniminer_1p1c, TestChain100Setup)
3
4 std::vector<CTransactionRef> all_transactions{tx1, tx2, tx3, tx4, tx5, tx6, tx7, tx8};
5 struct TxDimensions {
6- size_t vsize; CAmount mod_fee; CFeeRate feerate;
7+ int32_t vsize; CAmount mod_fee; CFeeRate feerate;
8 };
9 std::map<uint256, TxDimensions> tx_dims;
10 for (const auto& tx : all_transactions) {
Silent merge conflict (with
-werror)
Rebased. Sorry for not addressing comments promptly.
Following patch fixes the silent merge conflict for me after a rebase: @ryanofsky Thanks! Applied.
This change gets rid of a few casts and makes the following commit diff
smaller.
153@@ -152,13 +154,13 @@ class CTxMemPoolEntry
154 }
155
156 uint64_t GetCountWithDescendants() const { return nCountWithDescendants; }
157- uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
158+ int64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
Re-reviewing this basically from scratch, I stumble here. int32_t should be big enough to represent the weight of more than 5,000 max standard weight transactions. Are we actually bumping into overflow issues with int32_t somewhere for size with descendants?
I see that this was brought up before, so I assume it was left a 64-bit value to limit the scope of this PR. Is that right?
Are we actually bumping into overflow issues with
int32_tsomewhere for size with descendants?
I don’t think so.
However, UBSan raises “signed-integer-overflow” warnings about nSizeWithAncestors += modifySize; etc.
160
161 bool GetSpendsCoinbase() const { return spendsCoinbase; }
162
163 uint64_t GetCountWithAncestors() const { return nCountWithAncestors; }
164- uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
165+ int64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
The silent conflict with the #26531 has been resolved in the recent push. As this PR touches tracepoints now, @0xB10C @virtu maybe you want to look into it?
Something that just occurred to me: do/should we consider tracepoints to be a stable api?
No, we shouldn’t. I think that classes like CTxMemPoolEntry and their public access method return types should be considered as mempool implementation details for all purposes. Maybe tracepoint docs need an explicit note about that?
ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f. I’ve focused my testing and code review on the tracepoint related changes. The docs, the test, and the mempool_monitor.py demo script are updated. I ran the interface_usdt_mempool.py test and the mempool_monitor.py script. The mempool_monitor.py output looks correct.
Something that just occurred to me: do/should we consider tracepoints to be a stable api?
No, we shouldn’t. I think that classes like CTxMemPoolEntry and their public access method return types should be considered as mempool implementation details for all purposes.
The goal of the tracepoints is to expose implementation details. This means, if an implementation detail changes, the tracepoint interface can change too. As an extreme example: If we’d remove the mempool, we’d also drop the mempool tracepoints. Third party programs have to adapt in this case.
This causes warnings when compiling for 32 bit?
0txmempool.cpp:174:60: error: comparison of integers of different signs: 'long long' and 'uint64_t' (aka 'unsigned long long') [-Werror,-Wsign-compare]
1 if (stageit->GetSizeWithDescendants() + entry_size > static_cast<uint64_t>(limits.descendant_size_vbytes)) {
2 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31 error generated.
This causes warnings when compiling for 32 bit?
The relevant CI tasks logs are clean. How to reproduce it?
Steps to reproduce:
( cd depends && make HOST=i686-pc-linux-gnu CC='clang -m32' CXX='clang++ -m32 -Wsign-compare' NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 -j $(nproc) ) && ./autogen.sh && CONFIG_SITE="$PWD/depends/i686-pc-linux-gnu/share/config.site" ./configure && make -j $(nproc) src/bitcoind