rpc: Distinguish between vsize and sigop adjusted mempool vsize #32800

musaHaruna · 2025-06-24T06:13:34Z

Motivation and Problem

CTxMemPoolEntry::GetTxSize() returns the larger of two values: the BIP 141 virtual size (vsize) and the "sigop-adjusted size." This sigop-adjusted size is used by mempool validation and mining algorithms as a safeguard to prevent overfilling blocks with transactions that approach both the weight and signature operation (sigop) limits in a way that could exploit block space efficiency.

In the current implementation, the sigop-adjusted size is reported as the "vsize" in RPCs that provide mempool transaction data, such as getmempoolentry, getrawmempool, testmempoolaccept, and submitpackage. However, the documentation for these RPCs typically describes this value simply as the "virtual transaction size as defined in BIP 141," without acknowledging the sigop adjustment. Since the reported size may differ from the pure BIP 141 definition, this confuses people as in this tweet, discrepancy can be misleading, as the reported size may differ from the pure BIP 141 definition.

Proposed Solution

To resolve this, all mempool-related RPCs now return two separate fields:

vsize: the sigop-adjusted size, i.e. max(BIP 141 vsize, sigop-adjusted size), which reflects the value previously returned under the vsize label and continues to drive mempool acceptance and block template scoring.

vsize_bip141: the pure BIP 141 virtual size, strictly ceil(weight/4), matching the consensus definition and now reported separately for clarity.

This preserves the behavior for clients that depend on the mempool policy size being reported as vsize while making the consensus size explicitly accessible under vsize_bip141.

Additionally, this PR updates the relevant RPC help text to clearly document the distinction between these two sizes, and adds supporting documentation doc/policy/feerates-and-vsize.md to better explain fee rates, virtual size calculations, sigop adjustments, and the mempool policy heuristics.

A new field, sigopsize, has also been added to the getrawtransaction RPC result when input information (transaction is in the mempool) is available. This field explicitly reports the sigop-adjusted size as calculated by ceil(sigop cost * bytes_per_sigop / WITNESS_SCALE_FACTOR). Exposing this value provides users with more precise insight into how the transaction’s sigops impact its effective size for policy and fee estimation.

Note: This picks up work from the closed #27591
Fixes #32775

DrahtBot · 2025-06-24T06:13:38Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32800.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator
Concept ACK	jonatack

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34113 (refactor: [rpc] Remove confusing and brittle integral casts by maflcko)

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.

DrahtBot · 2025-06-24T07:05:35Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/44658392174}
_{LLM reason (✨ experimental): Linting errors caused the CI failure.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

janb84

A few comments / NITs on the documentation side of this PR.

doc/policy/feerates-and-vsize.md

hodlinator

Reviewed c8da5c1576636e31acb3422e52b8cb685dbbba64

src/rpc/mempool.cpp

test/functional/mempool_accept.py

test/functional/mempool_sigoplimit.py

src/rpc/rawtransaction.cpp

src/rpc/mempool.cpp

doc/policy/feerates-and-vsize.md

hodlinator · 2025-06-25T10:14:26Z

src/rpc/rawtransaction.cpp

+            const size_t txSize = entry->GetTxSize();
+            const size_t vSize = GetVirtualTransactionSize(txSize, 0, 0);
+            // Only report sigopsize when vSize is strictly less than the raw tx size
+            if (vSize < txSize) {


Q1: Why do we want to only conditionally report sigopsize, is there some comment to that affect from the old PR? I don't get that impression from #27591 (comment).

Q2: In the above linked comment, do you understand what is meant by "(Or, even better, do the sigop adjustment in cases where we can -- getrawtransaction of txs in blocks could pull up the utxo being spent via rev*.dat and calculate the sigop adjustment?)"? I must admit I don't fully understand it myself.

Q1. Yeah there's no comment about that in #27591, Am still skeptical about conditionally reporting the sigosize , the initial idea's to check when txSize which is the transaction in the mempool (which might be sigops adjusted from entry->GetTxSize()) is less than vSize GetVirtualTransactionSize(txSize, 0, 0) which is not sigop adjusted. But I think it will make more sense to remove the codition and report the sigopsize.

Q2. My understanding of the comment is; I think it’s suggesting that getrawtransaction could compute sigopsize for confirmed transactions by retrieving spent output scripts from rev*.dat. While possible, this is expensive and sometimes not feasible (e.g., on pruned nodes). That’s why sigopsize is only reported for mempool transactions, where all required data is readily available in memory. (I might wrong in as well)

Q1. Yeah, I think the interface is less confusing if we remove the condition. #27591 seems to use the term "sigop-adjusted size" rather than "sigop-equivalent size". So could change the description from:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L284
to:

"Sigop-adjusted size in bytes, only present for mempool transactions."

(Unless we implement Q2, which I agree is not critical).

Maybe replace "sigop-equivalent" with "sigop-adjusted" everywhere else too unless there is precedent for it?

Fixed as suggested in 70316b5. Thanks

Realized the comment is now out of date:

- // Add sigopsize if the transaction exists in the mempool and its size is affected by it. + // Add sigopsize if the transaction exists in the mempool.

hodlinator · 2025-06-25T10:16:56Z

src/rpc/rawtransaction.cpp

+            if (vSize < txSize) {
+            // bytes per sigop = witness scale factor (4)
+            const uint64_t sigopSize = uint64_t(entry->GetSigOpCost()) * uint64_t(nBytesPerSigOp);
+            result.pushKV("sigopsize", sigopSize);


nit: #27591 (comment) mentions adding sigopsize to other RPCs as well. Not sure if necessary.

Yeah, I don't think it's necessary that's why I decide to report it (sigopsize) only in getrawtransaction RPC

src/rpc/rawtransaction.cpp

musaHaruna · 2025-06-26T17:15:17Z

Thanks @hodlinator and @janb84 for the detailed review. Well Appreciated, Please let me know if there's any comment left unaddressed.

luke-jr · 2025-06-26T21:33:27Z

It makes much more sense to stick to weight for consensus-related matters, and vsize for policy in all cases.

musaHaruna · 2025-06-27T06:51:35Z

It makes much more sense to stick to weight for consensus-related matters, and vsize for policy in all cases.

Sorry, I didn't quite fully understand the comment (do you mean sigopsize should return only int64_t sigopWeight = entry->GetSigOpCost() * nBytesPerSigOp;) can you elaborate more on it, and what you are suggesting. Thanks.

hodlinator

Concept ACK be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3

Might be good to add release notes for this PR as it is adding to the user-facing RPC interface, similarly to: https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/doc/release-notes-27826.md

test/functional/mempool_sigoplimit.py

src/rpc/mempool.cpp

test/functional/mempool_sigoplimit.py

src/rpc/mempool.cpp

src/rpc/rawtransaction.cpp

hodlinator · 2025-06-27T11:49:05Z

src/rpc/rawtransaction.cpp

+            const size_t txSize = entry->GetTxSize();
+            const size_t vSize = GetVirtualTransactionSize(txSize, 0, 0);
+            // Only report sigopsize when vSize is strictly less than the raw tx size
+            if (vSize < txSize) {


Q1. Yeah, I think the interface is less confusing if we remove the condition. #27591 seems to use the term "sigop-adjusted size" rather than "sigop-equivalent size". So could change the description from:
https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L284
to:

"Sigop-adjusted size in bytes, only present for mempool transactions."

(Unless we implement Q2, which I agree is not critical).

Maybe replace "sigop-equivalent" with "sigop-adjusted" everywhere else too unless there is precedent for it?

DrahtBot · 2025-08-11T16:31:06Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/47833880904}
_{LLM reason (✨ experimental): The CI failure is caused by a missing trailing newline in a documentation file during the lint check.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

musaHaruna · 2025-08-11T17:09:37Z

I think documentation is definitely helpful, but I'm not sure an extra vsize_bip141 field is really necessary (they can just divide weight by 4) and I don't really see a reason for ca16b18

I understand it can be calculated from weight / 4, but I thought adding vsize_bip141 might save users from recalculating and improve clarity by avoiding confusion between policy-adjusted vsize and consensus BIP-141 vsize. I'm happy to adjust if you think it's uneccessary. Thanks.

I can remove ca16b18 if you feel strongly about it. I added it because of the discussion here: #27591.

Thank you so much!

hodlinator

ACK 74530ac9e05aafa91aaa00cc4f53bb4f26a55153

Clarifies the distinction between sigop-adjusted and non-sigop-adjusted vsize for RPC users.

Minor fixups and corrections since my last review #32800 (review)

hodlinator · 2025-08-29T10:08:38Z

doc/policy/feerates-and-vsize.md

+defaulting to 20 vbytes per sigop). Using a single combined metric like this simplifies block template
+construction by avoiding the need to optimize for both weight and sigops separately — a potential
+“two-dimensional knapsack” problem — though in practice the impact of that problem is often limited.
+Some miners prefer to handle weight and sigops as separate constraints, while others use the sigop-adjusted
+size for simplicity.


info: Verified that Bitcoin Core seems to use sigop-adjusted size for block template construction as it sounds like that in this part. IIUC, we do, going by:

Checking the block is full with packageSize...
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/node/miner.cpp#L209-L214
...which comes from GetSizeWithAncestors()...
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/node/miner.cpp#L376-L390
...which comes from...
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/kernel/mempool_entry.h#L126
...which is sigop-adjusted:
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/kernel/mempool_entry.h#L140-L143

Ordering by feerate, which uses GetTxSize():
https://github.com/bitcoin/bitcoin/blob/ca9c5219a4029901db6f49678620d1935bf1634f/src/node/miner.cpp#L313

hodlinator · 2025-08-29T11:01:18Z

doc/release-notes-32800.md

@@ -0,0 +1,5 @@
+- Mempool-related RPCs (`getrawmempool`, `getmempoolentry`, `testmempoolaccept`, `submitpackage`) now
+clarify the difference between BIP-141 virtual size (`vsize_bip141`) and policy-adjusted virtual size


nit: The adjusted beginning is nicer, but would rather bring back the end from 1523e8d6c981efff1394bd1669fbbbea5ff7ac97:

Additionally, getrawtransaction now includes
+a sigopsize field and continues to report the unadjusted BIP-141 virtual size in its vsize field.

hodlinator · 2025-08-29T11:10:52Z

doc/policy/feerates-and-vsize.md

+
+Both methods are valid: the first prioritizes simplicity, the second prioritizes potentially optimal
+space usage. For consensus-level weight calculations, refer to `vsize_bip141`. If you need to inspect
+the raw BIP-141 size of a transaction in the mempool, use `getrawtransaction`; if you need the
+policy-adjusted size, use `getmempoolentry` or `getrawmempool`.


nit: I think this part is repetitive and somewhat arbitrary, currently the PR ensures sigop-adjusted and non-sigop-adjusted sizes are available for all mentioned RPCs. If I was using RPCs for each transaction for this calculation I would just pick the fastest one. Maybe I'm overlooking something though.

Suggested change

Both methods are valid: the first prioritizes simplicity, the second prioritizes potentially optimal

space usage. For consensus-level weight calculations, refer to `vsize_bip141`. If you need to inspect

the raw BIP-141 size of a transaction in the mempool, use `getrawtransaction`; if you need the

policy-adjusted size, use `getmempoolentry` or `getrawmempool`.

jonatack

Concept ACK, appears to clarify these values, and adds a new "vsize_bip141" field to the API while not changing the existing vsize values returned.

musaHaruna · 2025-10-22T22:12:16Z

nit: The adjusted beginning is nicer, but would rather bring back the end from 1523e8d

Addressed by bring back the last sentence in commit 1523e8d

nit: I think this part is repetitive and somewhat arbitrary, currently the PR ensures sigop-adjusted and non-sigop-adjusted sizes are available for all mentioned RPCs. If I was using RPCs for each transaction for this calculation I would just pick the fastest one. Maybe I'm overlooking something though.

Agreed. I removed the entire paragraph from the docs.

All addressed changes are in 967d16

hodlinator

re-ACK c62b40d7abd8de1972bacb8c600bbab55a231b20

Changes since my first ACK (#32800 (review)):

Adjusted docs based on my latest feedback.
Avoids re-converting tx hash in the RPC code.

This commit adds a new `vsize_bip141` field to mempool acceptance and submission RPCs, including `testmempoolaccept` and `submitpackage`, to explicitly report the virtual transaction size as defined in BIP 141. RPC help texts are updated to reflect this addition. Tests in `mempool_accept.py, mempool_accept, p2p_segwit, rpc_packages, mempool_sigoplimit` are extended to verify the presence and correctness of the new field. Co-authored-by: Gloria Zhao <[email protected]>

…sactions Extend the `getrawtransaction` RPC to include a new field `sigopsize` when the transaction is in the mempool. The `sigopsize` provides the mempool's accounting size for the transaction based on its sigop cost, which can exceed its serialized vsize under `-bytespersigop` policies. Test coverage is added to verify the correct calculation and exposure of the `sigopsize` field via `mempool_sigoplimit.py`.

This commit introduces a new policy document at `doc/policy/feerates‑and‑vsize.md` that clarifies: - How transaction weight, BIP‑141 virtual size, and sigop‑adjusted size are calculated. - Which RPCs expose “vsize” (sigop‑adjusted) versus “vsize_bip141” (pure BIP‑141). - Guidance for users on fee estimation, block template construction, and consensus‑level size reporting.

musaHaruna · 2025-12-12T11:47:54Z

Rebased and fixed conflict.

hodlinator

re-ACK ad07093

Just resolved conflict with b0417ba in doc/policy/README.md (also removing double-newline at EOF) since my previous ACK (#32800 (review)).

DrahtBot · 2025-12-27T17:51:45Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the CI failed label Jun 24, 2025

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch 8 times, most recently from 969193c to 89d1220 Compare June 24, 2025 12:02

DrahtBot removed the CI failed label Jun 24, 2025

musaHaruna changed the title ~~Distinguish between vsize and sigop adjusted mempool vsize~~ RPC: Distinguish between vsize and sigop adjusted mempool vsize Jun 24, 2025

DrahtBot added the RPC/REST/ZMQ label Jun 24, 2025

musaHaruna changed the title ~~RPC: Distinguish between vsize and sigop adjusted mempool vsize~~ rpc: Distinguish between vsize and sigop adjusted mempool vsize Jun 24, 2025

janb84 reviewed Jun 25, 2025

View reviewed changes

doc/policy/feerates-and-vsize.md Outdated Show resolved Hide resolved

doc/policy/feerates-and-vsize.md Show resolved Hide resolved

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch from 89d1220 to c8da5c1 Compare June 25, 2025 08:18

hodlinator reviewed Jun 25, 2025

View reviewed changes

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch 3 times, most recently from 4cc9be2 to be75fa4 Compare June 26, 2025 17:09

hodlinator reviewed Jun 27, 2025

View reviewed changes

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch 4 times, most recently from abe0c4d to ede20b4 Compare June 28, 2025 10:49

DrahtBot added the CI failed label Jun 28, 2025

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch from 68ec63e to 05cc5e5 Compare August 11, 2025 16:30

DrahtBot added the CI failed label Aug 11, 2025

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch from 05cc5e5 to 74530ac Compare August 11, 2025 16:38

DrahtBot removed the CI failed label Aug 11, 2025

hodlinator approved these changes Aug 29, 2025

View reviewed changes

jonatack reviewed Oct 13, 2025

View reviewed changes

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch from 74530ac to c61bc4f Compare October 22, 2025 22:03

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch from c61bc4f to c62b40d Compare October 24, 2025 17:42

hodlinator approved these changes Oct 28, 2025

View reviewed changes

DrahtBot requested a review from jonatack October 28, 2025 19:45

ajtowns mentioned this pull request Nov 11, 2025

Relax standardness rules regarding CHECKMULTISIG #33755

Open

DrahtBot mentioned this pull request Nov 30, 2025

Cluster mempool followups #33591

Merged

DrahtBot added the Needs rebase label Dec 2, 2025

musaHaruna and others added 4 commits December 10, 2025 08:21

doc: add release notes for 32800

ad07093

musaHaruna force-pushed the distinguish_between_vsize_and_sigop-adjusted_mempool_vsize branch from c62b40d to ad07093 Compare December 10, 2025 07:36

DrahtBot added CI failed and removed Needs rebase CI failed labels Dec 10, 2025

hodlinator approved these changes Dec 12, 2025

View reviewed changes

This was referenced Dec 19, 2025

rpc: [mempool] Remove erroneous Univalue integral casts #34112

Merged

refactor: [rpc] Remove confusing and brittle integral casts #34113

Merged

DrahtBot added the Needs rebase label Dec 27, 2025

		@@ -0,0 +1,5 @@
		- Mempool-related RPCs (`getrawmempool`, `getmempoolentry`, `testmempoolaccept`, `submitpackage`) now
		clarify the difference between BIP-141 virtual size (`vsize_bip141`) and policy-adjusted virtual size

rpc: Distinguish between vsize and sigop adjusted mempool vsize #32800

Are you sure you want to change the base?

rpc: Distinguish between vsize and sigop adjusted mempool vsize #32800

Uh oh!

Conversation

musaHaruna commented Jun 24, 2025 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Problem

Proposed Solution

Uh oh!

DrahtBot commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Jun 24, 2025

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

musaHaruna commented Jun 26, 2025

Uh oh!

luke-jr commented Jun 26, 2025

Uh oh!

musaHaruna commented Jun 27, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 11, 2025

Uh oh!

musaHaruna commented Aug 11, 2025

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

musaHaruna commented Jun 24, 2025 •

edited by fanquake

Loading

DrahtBot commented Jun 24, 2025 •

edited

Loading