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

hebasto · 2022-01-03T17:58:00Z

From #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 8b5a4de

maflcko · 2022-01-04T09:44:17Z

Concept ACK. Could squash, given that the other pull was closed?

Things to look out for in review:

On 32-bit this will potentially (minimally) reduce the number of txs that fit into the mempool
types in cstdint aren't type safe and call sites will need to be reviewed as well

DrahtBot · 2022-01-04T10:22:29Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

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.

Conflicts

No conflicts as of last run.

hebasto · 2022-01-04T13:51:53Z

Updated 9a8a8ad -> 39c82fa (pr23962.01 -> pr23962.02).

Could squash, given that the other pull was closed?

Commits have been reordered to keep changes focused.

shaavan

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.

Here. The type of entry_size can be changed to int64_t. This will lead to making appropriate changes at the other places in the file.
Here. Type of totalSizeWithAncestors could be changed.

Btw I was curious about one thing. In line 232 in this file, there is implicit typecasting occurring:

totalSizeWithAncestors += 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.

glozow

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?

maflcko · 2022-01-04T16:54:14Z

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.

hebasto · 2022-01-05T08:54:59Z

@glozow

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.

luke-jr

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.

hebasto · 2022-01-24T22:33:54Z

Updated 39c82fa -> 295a4f0 (pr23962.02 -> pr23962.03):

rebased
addressed @luke-jr's comment:

This wastes memory for no benefit - we should probably go to int32_t instead.

@shaavan

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.

hebasto · 2022-01-25T09:23:44Z

Rebased 295a4f0 -> 960f996 (pr23962.03 -> pr23962.04) due to the conflict with #21464.

shaavan

Changes since my review:

Replaced int64_t with int32_t for the transaction variables. This is done for the reason suggested in this comment, with which I agree.
Rebased over the current master.

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.

This change gets rid of a few casts and makes the following commit diff smaller.

murchandamus

codereview ACK 3ef756a

murchandamus · 2023-06-12T19:19:30Z

src/kernel/mempool_entry.h

@@ -152,13 +154,13 @@ class CTxMemPoolEntry
    }

    uint64_t GetCountWithDescendants() const { return nCountWithDescendants; }
-    uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
+    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_t somewhere for size with descendants?

I don't think so.

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

murchandamus · 2023-06-12T19:21:03Z

src/kernel/mempool_entry.h

    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; }


ryanofsky

Code review ACK 3ef756a. Since last review, just rebased with more type changes in test and tracing code

achow101 · 2023-06-12T21:52:20Z

ACK 3ef756a

achow101 · 2023-06-12T21:59:19Z

Something that just occurred to me: do/should we consider tracepoints to be a stable api? Since this changes the types for some of the tracepoints, this would ostensibly break any third party programs that rely on those tracepoints.

hebasto · 2023-06-13T06:06:22Z

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?

0xB10C · 2023-06-13T14:14:18Z

ACK 3ef756a. 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.

… values

maflcko · 2023-06-15T15:27:31Z

This causes warnings when compiling for 32 bit?

txmempool.cpp:174:60: error: comparison of integers of different signs: 'long long' and 'uint64_t' (aka 'unsigned long long') [-Werror,-Wsign-compare]
        if (stageit->GetSizeWithDescendants() + entry_size > static_cast<uint64_t>(limits.descendant_size_vbytes)) {
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

hebasto · 2023-06-15T15:34:44Z

This causes warnings when compiling for 32 bit?

The relevant CI tasks logs are clean. How to reproduce it?

maflcko · 2023-06-15T15:53:42Z

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

glozow · 2023-07-20T12:51:09Z

This causes warnings when compiling for 32 bit?

@hebasto did you end up looking into this?

hebasto · 2023-07-20T13:29:45Z

This causes warnings when compiling for 32 bit?

This causes warnings when compiling for 32 bit?

@hebasto did you end up looking into this?

I apologise for forgetting to mention that #28059 addresses that warnings.

…iables signed 92de74e refactor: Make more transaction size variables signed (Hennadii Stepanov) Pull request description: This PR is a continuation of bitcoin/bitcoin#23962 and it: - gets rid of two static casts, - addresses bitcoin/bitcoin#23962 (comment), - is useful for bitcoin/bitcoin#25972, see the failed ARM and multiprocess CI jobs. ACKs for top commit: MarcoFalke: lgtm ACK 92de74e 🥔 glozow: ACK 92de74e Tree-SHA512: 84225961af8e08439664e75661b98fe86560217e891e5633a28316bf248d88df317a0c6b5a5f6b03feb2b0e0fd40a1f91dd4a85a0610d567470805bf47a84487

hebasto mentioned this pull request Jan 3, 2022

Remove txmempool implicit-integer-sign-change sanitizer suppressions #23957

Closed

DrahtBot added the Mempool label Jan 3, 2022

DrahtBot mentioned this pull request Jan 4, 2022

refactor tests to fix ubsan suppressions #23629

Merged

hebasto force-pushed the 220103-txsize branch from 9a8a8ad to 39c82fa Compare January 4, 2022 13:48

shaavan reviewed Jan 4, 2022

View reviewed changes

glozow reviewed Jan 4, 2022

View reviewed changes

This was referenced Jan 4, 2022

Mempool Update Cut-Through Optimization #21464

Merged

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

DrahtBot mentioned this pull request Jan 5, 2022

Fix signed integer overflow in prioritisetransaction RPC #23418

Merged

luke-jr suggested changes Jan 12, 2022

View reviewed changes

DrahtBot added the Needs rebase label Jan 20, 2022

hebasto force-pushed the 220103-txsize branch from 39c82fa to 295a4f0 Compare January 24, 2022 22:25

hebasto changed the title ~~Use int64_t type for transaction sizes consistently~~ Use int32_t type for transaction sizes consistently Jan 24, 2022

hebasto changed the title ~~Use int32_t type for transaction sizes consistently~~ Use int32_t type for transaction size/weight consistently Jan 24, 2022

DrahtBot added Needs rebase and removed Needs rebase labels Jan 25, 2022

maflcko added Consensus and removed Mempool labels Jan 25, 2022

hebasto force-pushed the 220103-txsize branch from 295a4f0 to 960f996 Compare January 25, 2022 09:22

DrahtBot removed the Needs rebase label Jan 25, 2022

shaavan reviewed Jan 25, 2022

View reviewed changes

This was referenced Jan 26, 2022

Optimize Mempool Reorg logic using Epochs, improving memory usage and runtime. #24158

Closed

test: Fix sanitizer suppresions in streams_tests #24190

Merged

DrahtBot added the CI failed label Jun 9, 2023

hebasto marked this pull request as draft June 12, 2023 17:35

hebasto added 2 commits June 12, 2023 19:47

Use int32_t type for most transaction size/weight values

d2f6d2a

This change gets rid of a few casts and makes the following commit diff smaller.

Remove txmempool implicit-integer-sign-change sanitizer suppressions

3ef756a

hebasto force-pushed the 220103-txsize branch from 65e8adf to 3ef756a Compare June 12, 2023 18:49

murchandamus reviewed Jun 12, 2023

View reviewed changes

DrahtBot requested review from achow101 and ryanofsky June 12, 2023 19:32

ryanofsky approved these changes Jun 12, 2023

View reviewed changes

hebasto marked this pull request as ready for review June 12, 2023 20:30

DrahtBot removed the CI failed label Jun 12, 2023

DrahtBot removed the request for review from achow101 June 12, 2023 21:52

achow101 merged commit 58b36fc into bitcoin:master Jun 13, 2023

hebasto deleted the 220103-txsize branch June 13, 2023 16:23

hebasto mentioned this pull request Jun 13, 2023

test: handle failed assert_equal() assertions in bcc callback functions #27831

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 15, 2023

erge bitcoin#23962: Use int32_t type for most transaction size/weight…

753f92b

… values

ryanofsky mentioned this pull request Jun 15, 2023

refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp #27890

Merged

hebasto mentioned this pull request Jul 10, 2023

refactor: Make more transaction size variables signed #28059

Merged

azuchi mentioned this pull request Dec 26, 2023

Tracepoints for mempool in tapyrus chaintope/tapyrus-core#281

Merged

bitcoin locked and limited conversation to collaborators Jul 19, 2024

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

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

Uh oh!

Conversation

hebasto commented Jan 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 4, 2022

Uh oh!

DrahtBot commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

hebasto commented Jan 4, 2022

Uh oh!

shaavan 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!

maflcko commented Jan 4, 2022

Uh oh!

hebasto commented Jan 5, 2022

Uh oh!

luke-jr 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!

hebasto commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 25, 2022

Uh oh!

shaavan 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!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

murchandamus Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 12, 2023

Uh oh!

achow101 commented Jun 12, 2023

Uh oh!

hebasto commented Jun 13, 2023

Uh oh!

0xB10C commented Jun 13, 2023

Uh oh!

maflcko commented Jun 15, 2023

Uh oh!

hebasto commented Jun 15, 2023

Uh oh!

maflcko commented Jun 15, 2023

Uh oh!

glozow commented Jul 20, 2023

Uh oh!

hebasto commented Jul 20, 2023

Uh oh!

Uh oh!

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

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

hebasto commented Jan 3, 2022 •

edited

Loading

DrahtBot commented Jan 4, 2022 •

edited

Loading

luke-jr left a comment •

edited

Loading

hebasto commented Jan 24, 2022 •

edited

Loading

shaavan left a comment •

edited

Loading