rpc: Distinguish between vsize and sigop adjusted mempool vsize

musaHaruna commented at 6:13 am on June 24, 2025: none

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 commented at 6:13 am on June 24, 2025: contributor

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
Concept ACK	hodlinator

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label CI failed on Jun 24, 2025

DrahtBot commented at 7:05 am on June 24, 2025: contributor

🚧 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.

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 force-pushed on Jun 24, 2025

DrahtBot removed the label CI failed on Jun 24, 2025

musaHaruna renamed this:
~~Distinguish between vsize and sigop adjusted mempool vsize~~
RPC: Distinguish between vsize and sigop adjusted mempool vsize
on Jun 24, 2025

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

musaHaruna renamed this:
~~RPC: Distinguish between vsize and sigop adjusted mempool vsize~~
rpc: Distinguish between vsize and sigop adjusted mempool vsize
on Jun 24, 2025

in doc/policy/feerates-and-vsize.md:38 in 89d1220a28 outdated

33+
34+## Rationale & Compatibility
35+Updating documentation to clarify that the existing `vsize` field in mempool RPCs is sigop‑adjusted avoids breaking changes
36+for clients relying on that behavior. Introducing the new `vsize_bip141` field is additive and non‑breaking, allowing
37+integrators to opt into the pure BIP‑141 metric. Future enhancements, such as adding a `sigopsize` field to expose raw sigop
38+counts or adjusted values directly, can be introduced without disrupting existing RPC contracts.

janb84 commented at 7:10 am on June 25, 2025:

This feels to me more as a PR comment than something that belongs in this documentation.

musaHaruna commented at 1:08 pm on June 26, 2025:

Yes I agree. Removed.

in doc/policy/feerates-and-vsize.md:1 in 89d1220a28

janb84 commented at 7:14 am on June 25, 2025:

Shouldn’t this file also be referenced in the README.md located in this directory ?

0This documentation is not an exhaustive list of all policy rules.
1
2- [Mempool Limits](mempool-limits.md)
3- [Mempool Replacements](mempool-replacements.md)
4- [Packages](packages.md)

hodlinator commented at 9:04 am on June 25, 2025:

PR description nits:

Suggest to use of automatic GitHub linking of issues instead of making an explicit markdown link. PR description is used in the merge-commit, so good to tidy up:

0- Fixes [32775](explicit link to PR)
1+ Fixes [#32775](/bitcoin-bitcoin/32775/)

Could add back-ticks for code: `CTxMemPoolEntry::GetTxSize()`

Could also mention that this is somewhat of a re-take of #27591.

0- To resolve this, all mempool-related RPCs now return two separate fields in all :
1+ To resolve this, all mempool-related RPCs now return two separate fields:

More consistent styling with the rest of the description:

0- A new field, sigopsize, has also been added to the getrawtransaction
1+ A new field, **sigopsize**, has also been added to the `getrawtransaction`

Typo “Heres” -> “Here’s”, but could be changed to:

0- Heres an example of a user finding this confusing [tweet](https://x.com/mononautical/status/1646166180145577990?s=20) discrepancy can be misleading, as the reported size may differ from the pure BIP 141 definition.
1+ Since the reported size may differ from the pure BIP 141 definition, this confuses people as in this example tweet: <https://x.com/mononautical/status/1646166180145577990?s=20>

This field explicitly reports the sigop-adjusted size as calculated by sigop count × bytes_per_sigop.

In code you have entry->GetSigOpCost() × nBytesPerSigOp. I assume cost is not equivalent to count?

Q: What are the outer symbols around “⌈weight/4⌉” - some maths notation I haven’t seen before?

hodlinator commented at 9:48 am on June 25, 2025:

nit: Would suggest wrapping the line width at 80 or 100 if you want to wrap it, or something closer to what other docs do in this folder.

musaHaruna commented at 1:00 pm on June 26, 2025:

The symbols are “⌈⌉” ceiling function, can also be written as ceil(weight/4) fixed it in the PR decription for easy readablity. All other NITs addressed. Thanks!

musaHaruna commented at 1:09 pm on June 26, 2025:

Thanks for the review!!! I have added [Feerates and vsize](feerates-and-vsize.md) to the README.md to make the list exhaustive

musaHaruna commented at 5:12 pm on June 26, 2025:

Fixed

hodlinator commented at 10:19 am on June 27, 2025:

Re ceiling - Ah, should have looked the symbols up. I’m fine either way, but would guess this is clearer for most programmers.

Sorry, didn’t mean it so literally, maybe something like:

0- Note: this is somewhat a retake of [#27591](/bitcoin-bitcoin/27591/)
1+ Note: this picks up work from the closed [#27591](/bitcoin-bitcoin/27591/).

I would put that together with the “Fixes #32775” at the bottom of the description, up to you.

You’re still using sigop count in the formula where I think sigop cost would be more precise. Should probably also divide by WITNESS_SCALE_FACTOR and ceil like is done in the Python code?

janb84 commented at 7:40 am on June 25, 2025: contributor

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

in src/rpc/mempool.cpp:236 in 89d1220a28 outdated

232@@ -231,6 +233,7 @@ static RPCHelpMan testmempoolaccept()
233                         // These can be used to calculate the feerate.
234                         result_inner.pushKV("allowed", true);
235                         result_inner.pushKV("vsize", virtual_size);
236+                        result_inner.pushKV("vsize_bip141", GetVirtualTransactionSize(*tx, 0, 0));

hodlinator commented at 7:48 am on June 25, 2025:

nit: Here and in other such cases - Would be nice to either make parameter names explicit to the reader (validated by clang-tidy):

0                        result_inner.pushKV("vsize_bip141", GetVirtualTransactionSize(*tx, /*nSigOpCost=*/0, /*bytes_per_sigop=*/0));

…or use the overload that doesn’t take sigops parameters:

0                        result_inner.pushKV("vsize_bip141", GetVirtualTransactionSize(*tx));

musaHaruna commented at 3:07 pm on June 26, 2025:

Done I used GetVirtualTransactionSize(*tx), check 83b6253. Thanks!

hodlinator commented at 11:24 am on June 27, 2025:

Still doing it in these places: https://github.com/bitcoin/bitcoin/blob/83b6253eb3921a79231f450bb43e1977ea835b5e/src/rpc/mempool.cpp#L298 https://github.com/bitcoin/bitcoin/blob/83b6253eb3921a79231f450bb43e1977ea835b5e/src/rpc/mempool.cpp#L1110

musaHaruna commented at 10:57 am on June 28, 2025:

Fixed in b5d472. Thanks!

in test/functional/mempool_accept.py:422 in 89d1220a28 outdated

418@@ -419,7 +419,7 @@ def run_test(self):
419         tx.vout[0] = CTxOut(COIN - 1000, DUMMY_MIN_OP_RETURN_SCRIPT)
420         assert_equal(len(tx.serialize_without_witness()), MIN_STANDARD_TX_NONWITNESS_SIZE)
421         self.check_mempool_result(
422-            result_expected=[{'txid': tx.txid_hex, 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal('0.00001000')}}],
423+            result_expected=[{'txid': tx.txid_hex, 'allowed': True, 'vsize': tx.get_vsize(),'vsize_bip141': tx.get_vsize(), 'fees': { 'base': Decimal('0.00001000')}}],

hodlinator commented at 8:00 am on June 25, 2025:

nit - Inconsistent space:

0            result_expected=[{'txid': tx.txid_hex, 'allowed': True, 'vsize': tx.get_vsize(), 'vsize_bip141': tx.get_vsize(), 'fees': { 'base': Decimal('0.00001000')}}],

musaHaruna commented at 3:08 pm on June 26, 2025:

Done in 83b6253. Thanks!

in test/functional/mempool_sigoplimit.py:136 in 26c501e14b outdated

131@@ -131,8 +132,8 @@ def test_sigops_limit(self, bytes_per_sigop, num_sigops):
132         assert_equal(entry_parent['ancestorcount'], 1)
133         assert_equal(entry_parent['ancestorsize'], parent_tx.get_vsize())
134         assert_equal(entry_parent['descendantcount'], 2)
135-        assert_equal(entry_parent['descendantsize'], parent_tx.get_vsize() + sigop_equivalent_vsize)
136-
137+        assert_equal(entry_parent['descendantsize'], parent_tx.get_vsize() + sigop_equivalent_vsize)    
138+

hodlinator commented at 8:03 am on June 25, 2025:

Please remove the random whitespace noise here in 26c501e14ba1aa80568257e35b114ccc9513abe7.

Could help to set the editor to remove trailing spaces on save if possible (that’s what I did after having this kind of thing pointed out).

musaHaruna commented at 4:29 pm on June 26, 2025:

Fixed

hodlinator commented at 10:15 am on June 27, 2025:

Still seeing it in 83b6253eb3921a79231f450bb43e1977ea835b5e.

musaHaruna force-pushed on Jun 25, 2025

in src/rpc/rawtransaction.cpp:394 in c8da5c1576 outdated

395+            // bytes per sigop = witness scale factor (4)
396+            const uint64_t sigopSize = uint64_t(entry->GetSigOpCost()) * uint64_t(nBytesPerSigOp);
397+            result.pushKV("sigopsize", sigopSize);
398+            }
399+        }
400+    }

hodlinator commented at 8:20 am on June 25, 2025:

Suggestions:

Correct vSize calculation - txSize is being sent in as the nWeight parameter which I think is wrong since txSize is already be affected by sigops.
Remove only used once variables txSize & vSize and adjusted “Only report sigopsize when vSize is strictly less than the raw tx size”-comment.
Remove “bytes per sigop = witness scale factor (4)"-comment - I don’t understand it, and DEFAULT_BYTES_PER_SIGOP which sets nBytesPerSigOp is 20?
Use snake_case for new variable names as per developer-notes.md (if you decide on keeping any).
Use comment instead of fromMempool which only covers part of the condition.
Should be possible to remove casts from sigopsize calculation since the factors are int64_t & unsigned int which should be promoted to the largest type int64_t, which should be big enough to contain this value without needing unsigned.
Fix weird indentation.

0    // Add sigopsize if the transaction exists in the mempool and it's size is affected by it.
1    if (blockindex == nullptr && hash_block.IsNull() && node.mempool) {
2        LOCK(node.mempool->cs);
3        if (auto entry = node.mempool->GetEntry(Txid::FromUint256(hash))) {
4            // Only report sigopsize when it affects the raw tx size.
5            if (GetVirtualTransactionSize(*tx) < entry->GetTxSize()) {
6                result.pushKV("sigopsize", entry->GetSigOpCost() * nBytesPerSigOp);
7            }
8        }
9    }

musaHaruna commented at 4:27 pm on June 26, 2025:

Done! Thanks for the suggestion it was realy insightful.

hodlinator commented at 11:30 am on June 27, 2025:

Thanks for incorporating the feedback! These should be snake_case: https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/src/rpc/rawtransaction.cpp#L392-L393 as per: https://github.com/bitcoin/bitcoin/blob/be75fa48fa77c417b9ebdd3be4d1ddb212ab16b3/doc/developer-notes.md?plain=1#L89-L90

in test/functional/mempool_sigoplimit.py:142 in c8da5c1576 outdated

137+        txid = tx.txid_hex
138+        self.log.info(f"Pushing padded tx {txid} into mempool for getrawtransaction sigopsize field check")
139+        self.nodes[0].sendrawtransaction(hexstring=tx_hex, maxburnamount='1.0')
140+        # Fetch with verbosity=1 and assert sigopsize is present and correct
141+        raw = self.nodes[0].getrawtransaction(txid, 1)
142+        assert 'sigopsize' in raw, "sigopsize field missing in getrawtransaction output"

hodlinator commented at 9:24 am on June 25, 2025:

Would be good to verify the value as well:

0        assert_equal(raw['sigopsize'], <calculation>)

musaHaruna commented at 4:36 pm on June 26, 2025:

Added in d6f4caf. Thanks.

hodlinator commented at 10:21 am on June 27, 2025:

Nice!

in src/rpc/rawtransaction.cpp:401 in c8da5c1576 outdated

397@@ -398,7 +398,6 @@ static RPCHelpMan getrawtransaction()
398             }
399         }
400     }
401-

hodlinator commented at 9:38 am on June 25, 2025:

Whitespace change in final commit should be moved to the commit introducing this block.

musaHaruna commented at 4:36 pm on June 26, 2025:

Fixed

in doc/policy/feerates-and-vsize.md:10 in c8da5c1576 outdated

 5+based strictly on block weight, the mempool policy introduces a *sigop adjustment* to account for potential
 6+denial‑of‑service vectors from transactions with unusually high signature‑operation counts. This document explains the
 7+definitions, how they are calculated, how they are exposed through RPC, and provides guidance for users and integrators.
 8+
 9+## Weight, Virtual Size, and Sigop‑Adjusted Size
10+**Transaction weight** is defined in BIP‑141 as the sum of four times the stripped transaction size plus the witness data

hodlinator commented at 9:42 am on June 25, 2025:

An empty line after all headers keeps the style consistent with other markdown files in the project:

0## Weight, Virtual Size, and Sigop‑Adjusted Size
1
2**Transaction weight** is defined in BIP‑141 as the sum of four times the stripped transaction size plus the witness data

musaHaruna commented at 5:11 pm on June 26, 2025:

Fixed

in doc/policy/feerates-and-vsize.md:12 in c8da5c1576 outdated

 7+definitions, how they are calculated, how they are exposed through RPC, and provides guidance for users and integrators.
 8+
 9+## Weight, Virtual Size, and Sigop‑Adjusted Size
10+**Transaction weight** is defined in BIP‑141 as the sum of four times the stripped transaction size plus the witness data
11+size. This weight metric is what consensus rules use to enforce the maximum block weight of 4 000 000 units.
12+**BIP‑141 virtual size** (sometimes referred to as `vsize_bip141`) is simply the block weight divided by four, rounded up

hodlinator commented at 9:45 am on June 25, 2025:

Missing punctuation (also found by Drahtbot LLM):

0**BIP‑141 virtual size** (sometimes referred to as `vsize_bip141`) is simply the block weight divided by four, rounded up.

musaHaruna commented at 5:12 pm on June 26, 2025:

Fixed

in doc/policy/feerates-and-vsize.md:23 in c8da5c1576 outdated

18+
19+## RPC Field Exposures
20+In mempool‑related RPCs (`getrawmempool`, `getmempoolentry`, `testmempoolaccept`, and `submitpackage`), the field named
21+`vsize` represents the **sigop‑adjusted size**, reflecting the policy‑adjusted estimate of block space usage. These same
22+RPCs also provide a field called `vsize_bip141` which exposes the pure BIP‑141 virtual size without sigop adjustment. When
23+querying a transaction via `getrawtransaction`, the reported `vsize` is the BIP‑141 value; sigop adjustment is not applied

hodlinator commented at 9:54 am on June 25, 2025:

nit:

0RPCs also provide a field called `vsize_bip141` which exposes the pure BIP‑141 virtual size without sigop adjustment. An
1exception to this rule is `getrawtransaction`, where the reported `vsize` is the BIP‑141 value; sigop adjustment is not applied

musaHaruna commented at 5:12 pm on June 26, 2025:

Added as suggested. Thanks!

in src/rpc/rawtransaction.cpp:394 in c8da5c1576 outdated

389+        if (auto entry = node.mempool->GetEntry(Txid::FromUint256(hash))) {
390+            // Compute base size and virtual size
391+            const size_t txSize = entry->GetTxSize();
392+            const size_t vSize = GetVirtualTransactionSize(txSize, 0, 0);
393+            // Only report sigopsize when vSize is strictly less than the raw tx size
394+            if (vSize < txSize) {

hodlinator commented at 10:14 am on June 25, 2025:

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 (review).

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.

musaHaruna commented at 2:15 pm on June 26, 2025:

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)

hodlinator commented at 11:49 am on June 27, 2025:

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?

musaHaruna commented at 10:59 am on June 28, 2025:

Fixed as suggested in 70316b5. Thanks

hodlinator commented at 11:27 am on July 18, 2025:

Realized the comment is now out of date:

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

in src/rpc/rawtransaction.cpp:397 in c8da5c1576 outdated

392+            const size_t vSize = GetVirtualTransactionSize(txSize, 0, 0);
393+            // Only report sigopsize when vSize is strictly less than the raw tx size
394+            if (vSize < txSize) {
395+            // bytes per sigop = witness scale factor (4)
396+            const uint64_t sigopSize = uint64_t(entry->GetSigOpCost()) * uint64_t(nBytesPerSigOp);
397+            result.pushKV("sigopsize", sigopSize);

hodlinator commented at 10:16 am on June 25, 2025:

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

musaHaruna commented at 2:17 pm on June 26, 2025:

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

in src/rpc/rawtransaction.cpp:396 in c8da5c1576 outdated

391+            const size_t txSize = entry->GetTxSize();
392+            const size_t vSize = GetVirtualTransactionSize(txSize, 0, 0);
393+            // Only report sigopsize when vSize is strictly less than the raw tx size
394+            if (vSize < txSize) {
395+            // bytes per sigop = witness scale factor (4)
396+            const uint64_t sigopSize = uint64_t(entry->GetSigOpCost()) * uint64_t(nBytesPerSigOp);

hodlinator commented at 11:45 am on June 25, 2025:

Should this value be scaled down and and rounded up as is done here?

https://github.com/bitcoin/bitcoin/blob/c8da5c1576636e31acb3422e52b8cb685dbbba64/src/policy/policy.cpp#L307

musaHaruna commented at 4:32 pm on June 26, 2025:

You are right it should be scalled down and rounded up as done in return (std::max(nWeight, nSigOpCost * bytes_per_sigop) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR;. Fixed in d6f4caf. Thanks.

hodlinator commented at 11:59 am on June 25, 2025: contributor

Reviewed c8da5c1576636e31acb3422e52b8cb685dbbba64

musaHaruna force-pushed on Jun 26, 2025

musaHaruna commented at 5:15 pm on June 26, 2025: none

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

luke-jr commented at 9:33 pm on June 26, 2025: member

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

musaHaruna commented at 6:51 am on June 27, 2025: none

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 commented at 12:12 pm on June 27, 2025: contributor

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

musaHaruna force-pushed on Jun 28, 2025

rpc: add explicit vsize_bip141 field to mempool-related RPCs

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 <gloriajzhao@gmail.com>

b5d472e1c5

musaHaruna force-pushed on Jun 28, 2025

DrahtBot added the label CI failed on Jun 28, 2025

DrahtBot commented at 10:49 am on June 28, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/44974262205 LLM reason (✨ experimental): The CI failure is caused by the ’trailing_newline’ lint check failure due to missing a required trailing newline in a documentation file.

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 commented at 10:53 am on June 28, 2025: none

Concept ACK be75fa4

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

Added in ede20b4. Thank you so much!!!

DrahtBot removed the label CI failed on Jun 28, 2025

fanquake requested review from glozow on Jul 2, 2025

musaHaruna force-pushed on Jul 7, 2025

in src/rpc/mempool.cpp:236 in 8158ca70a7 outdated

232@@ -231,6 +233,7 @@ static RPCHelpMan testmempoolaccept()
233                         // These can be used to calculate the feerate.
234                         result_inner.pushKV("allowed", true);
235                         result_inner.pushKV("vsize", virtual_size);
236+                        result_inner.pushKV("vsize_bip141", GetVirtualTransactionSize(*tx));

hodlinator commented at 8:16 pm on July 16, 2025:

Side note: I think it’s weird that we would not include the sizes when exceeding the max fee rate - one would think that it would be interesting to know then too. But it seems to have been the same since 2233a93a109b10b6fe0f5f26c2bb529c8de3dde7 when vsize was added. Not suggesting it be changed in this PR, but possible follow-up if others agree.

musaHaruna commented at 9:34 am on July 28, 2025:

Yeah agreed, to be left as follow-up if others agree.

in test/functional/mempool_sigoplimit.py:101 in 8158ca70a7 outdated

 96@@ -97,8 +97,8 @@ def test_sigops_limit(self, bytes_per_sigop, num_sigops):
 97         tx.vout[0].scriptPubKey = CScript([OP_RETURN, b'X'*(256+vsize_to_pad+1)])
 98         res = self.nodes[0].testmempoolaccept([tx.serialize().hex()])[0]
 99         assert_equal(res['allowed'], True)
100-        assert_equal(res['vsize'], sigop_equivalent_vsize+1)
101-

hodlinator commented at 12:09 pm on July 17, 2025:

meganit: This newline was logically separating the two commented code blocks and should probably be brought back.

hodlinator commented at 12:00 pm on August 2, 2025:

From #32800 (comment):

Addressed in 46acee8

The newline is added back in the second commit now, it would be better to change the first commit to avoid removing it.

in doc/policy/feerates-and-vsize.md:21 in 8158ca70a7 outdated

16+**BIP‑141 virtual size** (sometimes referred to as `vsize_bip141`) is simply the block weight divided
17+by four, rounded up. This value represents the consensus‑level “size” of a transaction without any
18+consideration of signature operations. To protect against “2D‑Knapsack” attacks that exploit a
19+combination of block weight and high sigop counts, the mempool applies
20+a **sigop‑adjusted size**. This value is the maximum of the pure BIP‑141 vsize and a sigop‑based
21+calculation (sigop count multiplied by a configurable `bytesPerSigOp` parameter, which defaults to

hodlinator commented at 12:18 pm on July 17, 2025:

nit: It would be good to hyphen-prefix to make it stand out visually (from fields like vsize etc) and also call it “setting” or “argument” rather than “parameter” as those are the terms used in the code.

0calculation (sigop count multiplied by a configurable `-bytesPerSigOp` setting, which defaults to

in doc/policy/feerates-and-vsize.md:31 in 8158ca70a7 outdated

26+
27+In mempool‑related RPCs (`getrawmempool`, `getmempoolentry`, `testmempoolaccept`, and `submitpackage`),
28+the field named `vsize` represents the **sigop‑adjusted size**, reflecting the policy‑adjusted estimate
29+of block space usage. These same RPCs also provide a field called `vsize_bip141` which exposes the
30+pure BIP‑141 virtual size without sigop adjustment. An exception to this rule is `getrawtransaction`,
31+when the reported `vsize` is the BIP‑141 value; sigop adjustment is not applied in that context by default.

hodlinator commented at 12:21 pm on July 17, 2025:

nit: I think the end is a bit redundant due to prior sentances, better to explain why:

0when the reported `vsize` is the BIP‑141 value due to backwards compatibility.

hodlinator commented at 12:19 pm on August 2, 2025:

Would be better to change “when” to “where” in the beginning of the line. Sorry for not noticing initially!

in doc/policy/feerates-and-vsize.md:38 in 8158ca70a7 outdated

33+## Guidance for Users
34+
35+When performing fee estimation and building block templates, **always use the sigop‑adjusted `vsize`**
36+from the mempool RPCs, as this reflects how Bitcoin Core internally prioritizes transactions. For
37+consensus‑level weight calculations, refer instead to `vsize_bip141`. If you need to inspect the raw
38+BIP‑141 size of a transaction not yet in the mempool, use `getrawtransaction`; but if you need the

hodlinator commented at 12:25 pm on July 17, 2025:

Documentation of getrawtransaction says:

By default, this call only returns a transaction if it is in the mempool.

So not sure what you mean by “not yet in the mempool”.

in doc/policy/feerates-and-vsize.md:36 in 8158ca70a7 outdated

31+when the reported `vsize` is the BIP‑141 value; sigop adjustment is not applied in that context by default.
32+
33+## Guidance for Users
34+
35+When performing fee estimation and building block templates, **always use the sigop‑adjusted `vsize`**
36+from the mempool RPCs, as this reflects how Bitcoin Core internally prioritizes transactions. For

hodlinator commented at 11:36 am on July 18, 2025:

When building block templates, I’m not sure we want to recommend using the sigop-adjusted vsize? That might leave space over in the block if some transactions have a lot of sigops. This would make a miner earn less fees due to not filling up the block.

hodlinator commented at 11:45 am on July 18, 2025: contributor

Reviewed 8158ca70a7b6b492607918d860947497efd72cd3

Found some final things before A-C-K.

DrahtBot added the label CI failed on Jul 26, 2025

musaHaruna force-pushed on Jul 27, 2025

rpc: add sigopsize field to getrawtransaction output for mempool transactions

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`.

46acee8900

doc: add fee‑rates and virtual‑size policy documentation

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.

af3f15a2ab

docs: add release notes for 32800 1523e8d6c9

musaHaruna force-pushed on Jul 27, 2025

DrahtBot removed the label CI failed on Jul 27, 2025

musaHaruna commented at 9:48 am on July 28, 2025: none

meganit: This newline was logically separating the two commented code blocks and should probably be brought back.

Addressed in 46acee8

nit: It would be good to hyphen-prefix to make it stand out visually (from fields like vsize etc) and also call it “setting” or “argument” rather than “parameter” as those are the terms used in the code.

Fixed as suggested

nit: I think the end is a bit redundant due to prior sentances, better to explain why:

Fixed as suggested

Documentation of getrawtransaction says: By default, this call only returns a transaction if it is in the mempool.

Corrected to “transaction in the mempool”

Realized the comment is now out of date:

Fixed as suggested

When building block templates, I’m not sure we want to recommend using the sigop-adjusted vsize? That might leave space over in the block if some transactions have a lot of sigops. This would make a miner earn less fees due to not filling up the block.

New change is now “When performing mempool policy operations (e.g fee estimation, eviction, or acceptance), use the sigop-adjusted vsize. However, when building block templates, miners should prioritize transactions by actual fee per weight unit and be mindful of the 80,000 sigop limit, rather than relying solely on sigop-adjusted vsize from the mempool RPCs”. Check af3f15a for docs chanages.

Thanks for the review!!! @hodlinator

in doc/policy/feerates-and-vsize.md:42 in 1523e8d6c9

37+by actual fee per weight unit and be mindful of the 80,000 sigop limit, rather than relying solely
38+on sigop-adjusted `vsize` from the mempool RPCs. For consensus‑level weight calculations, refer instead
39+to `vsize_bip141`. If you need to inspect the raw BIP‑141 size of a transaction in the mempool, use
40+`getrawtransaction`; but if you need the policy‑adjusted size, use `getmempoolentry` or `getrawmempool`.
41+Node operators should be cautious when changing the `-bytespersigop` setting, as it can affect fee
42+estimation accuracy and block utilization without impacting consensus rules.

hodlinator commented at 12:37 pm on August 2, 2025:

Reworded initial sentence to account for fee estimation not being a policy itself.
Use plain “size” instead of “vsize” to avoid loading foot-gun around getrawtransaction’s vsize.
Add sentence motivate reasoning behind initial recommendation.
Bring up the -bytespersigop sentence up to section around sigop-adjusted size and split paragraph.
Use non-breaking space instead of comma as numeric separator for consistency with “4 000 000 units” above.
Avoid detailing RPCs as the prior section on them should be enough.

0When performing operations involving or affected by mempool policy (e.g fee estimation, eviction, or acceptance), use the
1sigop-adjusted size. Doing so should lead to better accuracy assuming most of the
2network/miners do the same. Node operators should also be cautious when changing the
3`-bytespersigop` setting, as it affects these aspects.
4
5However, when building block templates, miners should prioritize transactions
6by actual fee per weight unit and be mindful of the 80 000 sigop limit, instead of using the
7sigop-adjusted size from the mempool RPCs.

hodlinator commented at 12:57 pm on August 2, 2025: contributor

Reviewed 1523e8d6c981efff1394bd1669fbbbea5ff7ac97

Still had some late suggestions which will hopefully get this in better shape for other reviewers too.

Might be worth pinging some participants from #27591 at this point to see what they think.

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

Motivation and Problem

Proposed Solution

Code Coverage & Benchmarks

Reviews