Skip to content

Use int32_t type for most transaction size/weight values #23962

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 13, 2023
Merged

Use int32_t type for most transaction size/weight values #23962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2f6d2a
Use `int32_t` type for most transaction size/weight values
hebasto Jun 12, 2023
3ef756a
Remove txmempool implicit-integer-sign-change sanitizer suppressions
hebasto Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions contrib/tracing/mempool_monitor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@
struct added_event
{
u8 hash[HASH_LENGTH];
u64 vsize;
s32 vsize;
s64 fee;
};

struct removed_event
{
u8 hash[HASH_LENGTH];
char reason[MAX_REMOVAL_REASON_LENGTH];
u64 vsize;
s32 vsize;
s64 fee;
u64 entry_time;
};
Expand All @@ -49,11 +49,11 @@
struct replaced_event
{
u8 replaced_hash[HASH_LENGTH];
u64 replaced_vsize;
s32 replaced_vsize;
s64 replaced_fee;
u64 replaced_entry_time;
u8 replacement_hash[HASH_LENGTH];
u64 replacement_vsize;
s32 replacement_vsize;
s64 replacement_fee;
};

Expand Down
8 changes: 4 additions & 4 deletions doc/tracing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ about the transaction.

Arguments passed:
1. Transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
2. Transaction virtual size as `uint64`
2. Transaction virtual size as `int32`
3. Transaction fee as `int64`

#### Tracepoint `mempool:removed`
Expand All @@ -231,7 +231,7 @@ about the transaction.
Arguments passed:
1. Transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
2. Removal reason as `pointer to C-style String` (max. length 9 characters)
3. Transaction virtual size as `uint64`
3. Transaction virtual size as `int32`
4. Transaction fee as `int64`
5. Transaction mempool entry time (epoch) as `uint64`

Expand All @@ -242,11 +242,11 @@ Passes information about the replaced and replacement transactions.

Arguments passed:
1. Replaced transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
2. Replaced transaction virtual size as `uint64`
2. Replaced transaction virtual size as `int32`
3. Replaced transaction fee as `int64`
4. Replaced transaction mempool entry time (epoch) as `uint64`
5. Replacement transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
6. Replacement transaction virtual size as `uint64`
6. Replacement transaction virtual size as `int32`
7. Replacement transaction fee as `int64`

Note: In cases where a single replacement transaction replaces multiple
Expand Down
2 changes: 1 addition & 1 deletion src/consensus/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ class BlockValidationState : public ValidationState<BlockValidationResult> {};
// using only serialization with and without witness data. As witness_size
// is equal to total_size - stripped_size, this formula is identical to:
// weight = (stripped_size * 3) + total_size.
static inline int64_t GetTransactionWeight(const CTransaction& tx)
static inline int32_t GetTransactionWeight(const CTransaction& tx)
{
return ::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(tx, PROTOCOL_VERSION);
}
Expand Down
22 changes: 12 additions & 10 deletions src/kernel/mempool_entry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class CTxMemPoolEntry
mutable Parents m_parents;
mutable Children m_children;
const CAmount nFee; //!< Cached to avoid expensive parent-transaction lookups
const size_t nTxWeight; //!< ... and avoid recomputing tx weight (also used for GetTxSize())
const int32_t nTxWeight; //!< ... and avoid recomputing tx weight (also used for GetTxSize())
const size_t nUsageSize; //!< ... and total memory usage
const int64_t nTime; //!< Local time when entering the mempool
const unsigned int entryHeight; //!< Chain height when entering the mempool
Expand All @@ -88,12 +88,14 @@ class CTxMemPoolEntry
// mempool; if we remove this transaction we must remove all of these
// descendants as well.
uint64_t nCountWithDescendants{1}; //!< number of descendant transactions
uint64_t nSizeWithDescendants; //!< ... and size
// Using int64_t instead of int32_t to avoid signed integer overflow issues.
int64_t nSizeWithDescendants; //!< ... and size
CAmount nModFeesWithDescendants; //!< ... and total fees (all including us)

// Analogous statistics for ancestor transactions
uint64_t nCountWithAncestors{1};
uint64_t nSizeWithAncestors;
// Using int64_t instead of int32_t to avoid signed integer overflow issues.
int64_t nSizeWithAncestors;
CAmount nModFeesWithAncestors;
int64_t nSigOpCostWithAncestors;

Expand All @@ -104,7 +106,7 @@ class CTxMemPoolEntry
int64_t sigops_cost, LockPoints lp)
: tx{tx},
nFee{fee},
nTxWeight(GetTransactionWeight(*tx)),
nTxWeight{GetTransactionWeight(*tx)},
nUsageSize{RecursiveDynamicUsage(tx)},
nTime{time},
entryHeight{entry_height},
Expand All @@ -121,11 +123,11 @@ class CTxMemPoolEntry
const CTransaction& GetTx() const { return *this->tx; }
CTransactionRef GetSharedTx() const { return this->tx; }
const CAmount& GetFee() const { return nFee; }
size_t GetTxSize() const
int32_t GetTxSize() const
{
return GetVirtualTransactionSize(nTxWeight, sigOpCost, ::nBytesPerSigOp);
}
size_t GetTxWeight() const { return nTxWeight; }
int32_t GetTxWeight() const { return nTxWeight; }
std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; }
unsigned int GetHeight() const { return entryHeight; }
int64_t GetSigOpCost() const { return sigOpCost; }
Expand All @@ -134,9 +136,9 @@ class CTxMemPoolEntry
const LockPoints& GetLockPoints() const { return lockPoints; }

// Adjusts the descendant state.
void UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount);
void UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount);
// Adjusts the ancestor state
void UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps);
void UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps);
// Updates the modified fees with descendants/ancestors.
void UpdateModifiedFee(CAmount fee_diff)
{
Expand All @@ -152,13 +154,13 @@ class CTxMemPoolEntry
}

uint64_t GetCountWithDescendants() const { return nCountWithDescendants; }
uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
int64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we actually bumping into overflow issues with int32_t somewhere for size with descendants?

I don't think so.

However, UBSan raises "signed-integer-overflow" warnings about nSizeWithAncestors += modifySize; etc.

CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; }

bool GetSpendsCoinbase() const { return spendsCoinbase; }

uint64_t GetCountWithAncestors() const { return nCountWithAncestors; }
uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
int64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; }
int64_t GetSigOpCostWithAncestors() const { return nSigOpCostWithAncestors; }

Expand Down
2 changes: 1 addition & 1 deletion src/policy/policy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};
/** Default for -blockmintxfee, which sets the minimum feerate for a transaction in blocks created by mining code **/
static constexpr unsigned int DEFAULT_BLOCK_MIN_TX_FEE{1000};
/** The maximum weight for transactions we're willing to relay/mine */
static constexpr unsigned int MAX_STANDARD_TX_WEIGHT{400000};
static constexpr int32_t MAX_STANDARD_TX_WEIGHT{400000};
/** The minimum non-witness size for transactions we're willing to relay/mine: one larger than 64 */
static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{65};
/** Maximum number of signature check operations in an IsStandard() P2SH script */
Expand Down
2 changes: 1 addition & 1 deletion src/test/miniminer_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ BOOST_FIXTURE_TEST_CASE(miniminer_1p1c, TestChain100Setup)

std::vector<CTransactionRef> all_transactions{tx1, tx2, tx3, tx4, tx5, tx6, tx7, tx8};
struct TxDimensions {
size_t vsize; CAmount mod_fee; CFeeRate feerate;
int32_t vsize; CAmount mod_fee; CFeeRate feerate;
};
std::map<uint256, TxDimensions> tx_dims;
for (const auto& tx : all_transactions) {
Expand Down
26 changes: 13 additions & 13 deletions src/txmempool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan
}
// descendants now contains all in-mempool descendants of updateIt.
// Update and add to cached descendant map
int64_t modifySize = 0;
int32_t modifySize = 0;
CAmount modifyFee = 0;
int64_t modifyCount = 0;
for (const CTxMemPoolEntry& descendant : descendants) {
Expand All @@ -91,7 +91,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendan
// Don't directly remove the transaction here -- doing so would
// invalidate iterators in cachedDescendants. Mark it for removal
// by inserting into descendants_to_remove.
if (descendant.GetCountWithAncestors() > uint64_t(m_limits.ancestor_count) || descendant.GetSizeWithAncestors() > uint64_t(m_limits.ancestor_size_vbytes)) {
if (descendant.GetCountWithAncestors() > uint64_t(m_limits.ancestor_count) || descendant.GetSizeWithAncestors() > m_limits.ancestor_size_vbytes) {
descendants_to_remove.insert(descendant.GetTx().GetHash());
}
}
Expand Down Expand Up @@ -278,8 +278,8 @@ void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors
for (const CTxMemPoolEntry& parent : parents) {
UpdateChild(mapTx.iterator_to(parent), it, add);
}
const int64_t updateCount = (add ? 1 : -1);
const int64_t updateSize = updateCount * it->GetTxSize();
const int32_t updateCount = (add ? 1 : -1);
const int32_t updateSize{updateCount * it->GetTxSize()};
const CAmount updateFee = updateCount * it->GetModifiedFee();
for (txiter ancestorIt : setAncestors) {
mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e) { e.UpdateDescendantState(updateSize, updateFee, updateCount); });
Expand Down Expand Up @@ -323,7 +323,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
setEntries setDescendants;
CalculateDescendants(removeIt, setDescendants);
setDescendants.erase(removeIt); // don't update state for self
int64_t modifySize = -((int64_t)removeIt->GetTxSize());
int32_t modifySize = -removeIt->GetTxSize();
CAmount modifyFee = -removeIt->GetModifiedFee();
int modifySigOps = -removeIt->GetSigOpCost();
for (txiter dit : setDescendants) {
Expand Down Expand Up @@ -365,21 +365,21 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
}
}

void CTxMemPoolEntry::UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount)
void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount)
{
nSizeWithDescendants += modifySize;
assert(int64_t(nSizeWithDescendants) > 0);
assert(nSizeWithDescendants > 0);
nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee);
nCountWithDescendants += modifyCount;
nCountWithDescendants += uint64_t(modifyCount);
assert(int64_t(nCountWithDescendants) > 0);
Comment on lines -373 to 374
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Remove txmempool implicit-integer-sign-change sanitizer suppressions" (93daff4)

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:

  1. Casting modifyCount to uint64_t produces an equivalent value mod 264
  2. Adding two uint64_t values is performed mod 264

Also, 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:

nCountWithDescendants += uint64_t(modifyCount);
assert(int64_t(nCountWithDescendants) > 0);

could be replaced with:

nCountWithDescendants += modifyCount;
assert(nCountWithDescendants > 0);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanofsky

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way seems fine. I suggested it for a followup because it isn't directly related to using a consistent type for transaction sizes. If you will make a follow up PR, you could consider dropping the second commit from this PR to avoid churn, since the second commit 93daff4 also isn't directly related to tx sizes. But whatever you want to do is fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just started reading the open threads on this pull and realized I did this yesterday in #27890 ?

}

void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps)
void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps)
{
nSizeWithAncestors += modifySize;
assert(int64_t(nSizeWithAncestors) > 0);
assert(nSizeWithAncestors > 0);
nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, modifyFee);
nCountWithAncestors += modifyCount;
nCountWithAncestors += uint64_t(modifyCount);
assert(int64_t(nCountWithAncestors) > 0);
nSigOpCostWithAncestors += modifySigOps;
assert(int(nSigOpCostWithAncestors) >= 0);
Expand Down Expand Up @@ -699,7 +699,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
// Verify ancestor state is correct.
auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits())};
uint64_t nCountCheck = ancestors.size() + 1;
uint64_t nSizeCheck = it->GetTxSize();
int32_t nSizeCheck = it->GetTxSize();
CAmount nFeesCheck = it->GetModifiedFee();
int64_t nSigOpCheck = it->GetSigOpCost();

Expand All @@ -720,7 +720,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
// Check children against mapNextTx
CTxMemPoolEntry::Children setChildrenCheck;
auto iter = mapNextTx.lower_bound(COutPoint(it->GetTx().GetHash(), 0));
uint64_t child_sizes = 0;
int32_t child_sizes{0};
for (; iter != mapNextTx.end() && iter->first->hash == it->GetTx().GetHash(); ++iter) {
txiter childit = mapTx.find(iter->second->GetHash());
assert(childit != mapTx.end()); // mapNextTx points to in-mempool transactions
Expand Down
2 changes: 1 addition & 1 deletion src/txmempool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ struct TxMempoolInfo
CAmount fee;

/** Virtual size of the transaction. */
size_t vsize;
int32_t vsize;

/** The fee delta. */
int64_t nFeeDelta;
Expand Down
8 changes: 4 additions & 4 deletions test/functional/interface_usdt_mempool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@
struct added_event
{
u8 hash[HASH_LENGTH];
u64 vsize;
s32 vsize;
s64 fee;
};

struct removed_event
{
u8 hash[HASH_LENGTH];
char reason[MAX_REMOVAL_REASON_LENGTH];
u64 vsize;
s32 vsize;
s64 fee;
u64 entry_time;
};
Expand All @@ -57,11 +57,11 @@
struct replaced_event
{
u8 replaced_hash[HASH_LENGTH];
u64 replaced_vsize;
s32 replaced_vsize;
s64 replaced_fee;
u64 replaced_entry_time;
u8 replacement_hash[HASH_LENGTH];
u64 replacement_vsize;
s32 replacement_vsize;
s64 replacement_fee;
};

Expand Down
1 change: 0 additions & 1 deletion test/sanitizer_suppressions/ubsan
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ implicit-integer-sign-change:prevector.h
implicit-integer-sign-change:script/bitcoinconsensus.cpp
implicit-integer-sign-change:script/interpreter.cpp
implicit-integer-sign-change:serialize.h
implicit-integer-sign-change:txmempool.cpp
implicit-signed-integer-truncation:crypto/
implicit-unsigned-integer-truncation:crypto/
shift-base:arith_uint256.cpp
Expand Down