rpc: distinguish between vsize and sigop-adjusted mempool vsize

glozow commented at 7:44 pm on May 7, 2023: member

CTxMemPoolEntry::GetTxSize() returns the maximum between vsize and “sigop-adjusted size” which is used by mempool validation and mining code as a heuristic to avoid 2DKnapsacking the block weight and sigop limits.

Sigop-adjusted vsize is returned as the “vsize” value of a transaction for RPCs retrieving mempool entry information (e.g. getmempoolentry andgetrawmempool) and mempool acceptance (testmempoolaccept and submitpackage). The documentation for these RPCs usually says “virtual transaction size as defined in BIP 141” without mentioning the sigop adjustment. At least 1 user has expressed confusion in a tweet.

I think we should at least mention something in the docs about the sigop-adjusted size. But imo it would be better to provide 2 vsize results: one that is sigop-adjusted and one that isn’t.

Seeking feedback on what might be most clear/helpful for users.

This PR’s approach is to change the result for “vsize” to not include sigop limits. The sigop-adjusted vsize is returned as a new result, “mempool_vsize”.

glozow added the label RPC/REST/ZMQ on May 7, 2023

DrahtBot commented at 7:44 pm on May 7, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	mononaut, Sjors, luke-jr, kevkevinpal, kristapsk, hebasto

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28848 (bugfix, Change up submitpackage results to return results for all transactions by instagibbs)

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.

in src/rpc/mempool.cpp:289 in 60bde2dac0 outdated

284@@ -282,7 +285,8 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
285 {
286     AssertLockHeld(pool.cs);
287 
288-    info.pushKV("vsize", (int)e.GetTxSize());
289+    info.pushKV("vsize", (int)GetVirtualTransactionSize(e.GetTx(), 0, 0));
290+    info.pushKV("mempool_vsize", (int)e.GetTxSize());

maflcko commented at 6:55 am on May 8, 2023:

Is there still a reason with recent univalue to use potentially narrowing casts? You could try no cast or a w-narrowing one (with {})?

glozow commented at 1:56 pm on August 25, 2023:

removed

in src/rpc/mempool.cpp:135 in 60bde2dac0 outdated

131@@ -132,6 +132,7 @@ static RPCHelpMan testmempoolaccept()
132                     {RPCResult::Type::BOOL, "allowed", /*optional=*/true, "Whether this tx would be accepted to the mempool and pass client-specified maxfeerate. "
133                                                        "If not present, the tx was not fully validated due to a failure in another tx in the list."},
134                     {RPCResult::Type::NUM, "vsize", /*optional=*/true, "Virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted (only present when 'allowed' is true)"},
135+                    {RPCResult::Type::NUM, "mempool_vsize", /*optional=*/true, "Sigop-adjusted virtual transaction size used by mempool and mining."},

maflcko commented at 6:57 am on May 8, 2023:

Is there a setting to affect this value? If yes, does it make sense to mention the setting’s key?

Sjors commented at 2:18 pm on May 8, 2023:

I would call this adjusted_vsize. Or vvsize :-P

glozow commented at 1:53 pm on August 25, 2023:

Added mention of -bytespersigop

glozow added the label Needs Conceptual Review on May 8, 2023

mononaut commented at 1:48 pm on May 8, 2023: none

Concept ACK

Sjors commented at 2:24 pm on May 8, 2023: member

Concept ACK on clarifying and returning both variants.

It would useful to peruse a few other projects to see how they use getmempoolentry and getrawmempool, to determine if this is a breaking change (in practice).

luke-jr commented at 4:38 pm on June 23, 2023: member

Concept ACK, but should be split into two commits.

luke-jr referenced this in commit c7ace800c3 on Aug 16, 2023

luke-jr commented at 10:41 pm on August 24, 2023: member

On further thought, it might make more sense to correct the description and maybe add a vsize_bip141 field. Consensus only deals with weight anyway, so vsize doesn’t really exist outside policy (and BIP141).

[doc] mention sigop-adjusted size for vsize RPC result 6a371bc881

[rpc] add vsize_bip141 for non-sigop-adjusted vsize

Add a result for users who are expecting BIP141 vsize without sigop
adjustment.

9cf46b6a3f

glozow force-pushed on Aug 25, 2023

glozow commented at 1:57 pm on August 25, 2023: member

On further thought, it might make more sense to correct the description and maybe add a vsize_bip141 field.

Makes sense to me, changed. Also means no changes to the existing field apart from documentation.

Concept ACK, but should be split into two commits.

Did this, one editing the doc and another to add the new field

Also rebased since it’s been a few months.

in src/rpc/mempool.cpp:133 in 9cf46b6a3f

129@@ -130,7 +130,9 @@ static RPCHelpMan testmempoolaccept()
130                     {RPCResult::Type::STR, "package-error", /*optional=*/true, "Package validation error, if any (only possible if rawtxs had more than 1 transaction)."},
131                     {RPCResult::Type::BOOL, "allowed", /*optional=*/true, "Whether this tx would be accepted to the mempool and pass client-specified maxfeerate. "
132                                                        "If not present, the tx was not fully validated due to a failure in another tx in the list."},
133-                    {RPCResult::Type::NUM, "vsize", /*optional=*/true, "Virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted (only present when 'allowed' is true)"},
134+                    {RPCResult::Type::NUM, "vsize", /*optional=*/true, "Maximum of sigop-adjusted size (-bytespersigop) and virtual transaction size as defined in BIP 141."},

luke-jr commented at 6:14 pm on August 28, 2023:

I think we should just define “virtual size” to consider bytespersigop policy in general. (But the difficulty calculating it may be an issue)

glozow commented at 9:23 am on August 29, 2023:

Are you suggesting we add a doc/policy/virtual_size.md?

ajtowns commented at 2:19 am on October 31, 2023:

Perhaps doc/policy/feerates-and-vsize.md (or perhaps policy/mining-score.md) with the idea being to document how we prioritise txs for mining (namely via fee vs vsize), how vsize is calculated (weight scaled down by a factor of 4, then rounded up, and only sigop-adjusted when that is possible), and that for normal transactions sigop-adjustment doesn’t actually change anything. Could also add that -bytespersigop allows tweaking how the sigop-adjustment works, and why that’s important to do and can be a bad thing to change (bad fee estimation, suboptimal block templates).

stevenroose commented at 7:13 pm on September 6, 2023: contributor

Concept ack.

Kinda unfortunate that the “vsize” field is still kinda broken IMO, only it’s backwards compatible.

In fact, arguably since you changed the documentation, it is now also a breaking change in the API spec. And arguably the field not being implemented as it’s documented could be seen as a bug and fixing the field to actually be what it says to be is a bugfix.

Personally I’d change the semantics of the vsize field to keep it “as per 141” and add something else like “vsize_sigops” for the alternative weird mempool-specific value.

sipa commented at 7:20 pm on September 6, 2023: member

I disagree that the sigop-adjusted value is a “weird mempool-specific value”; in fact I’d contend it’s the only value anyone should ever use (which is why it got dropped in in the first place). It is our best estimate for how much effective block space a transaction uses, rescaled to 1000000, for purposes of block building, fee estimation, feerate-based policies one would want to enforce, …

Now, it’s also not perfect. Sadly it can only be computed in contexts where the input is known, and it is subject to a fudge factor (20 vbytes / sigop) that’ll likely be off in case a very large number of transactions would be high sigop. So I see why it’s annoying to define precisely and work with, but that doesn’t change the fact that uninformed users should use the adjusted value if it is available.

stevenroose commented at 4:50 pm on September 7, 2023: contributor

I suppose this is only relevant for pre-taproot (pre-segwit?) sigops, right? Because tapscript introduces this “validation weight/budget” concept? So I guess eventually the confusion will mostly disappear :)

kevkevinpal commented at 3:39 am on October 20, 2023: contributor

Concept ACK 9cf46b6

I took a look at some places where we could add tests for vsize_bip141 and made this commit 4ab1f14

feel free to cherry commit or not use

in src/rpc/mempool.cpp:849 in 9cf46b6a3f

844@@ -840,7 +845,8 @@ static RPCHelpMan submitpackage()
845                     {RPCResult::Type::OBJ, "wtxid", "transaction wtxid", {
846                         {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
847                         {RPCResult::Type::STR_HEX, "other-wtxid", /*optional=*/true, "The wtxid of a different transaction with the same txid but different witness found in the mempool. This means the submitted transaction was ignored."},
848-                        {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."},
849+                        {RPCResult::Type::NUM, "vsize", "Maximum of sigop-adjusted size (-bytespersigop) and virtual transaction size as defined in BIP 141."},
850+                        {RPCResult::Type::NUM, "vsize_bip141", "Virtual transaction size as defined in BIP 141."},

ajtowns commented at 2:05 am on October 31, 2023:

PR description needs updating from “mempool_vsize”

in src/rpc/mempool.cpp:259 in 9cf46b6a3f

254@@ -252,7 +255,8 @@ static RPCHelpMan testmempoolaccept()
255 static std::vector<RPCResult> MempoolEntryDescription()
256 {
257     return {
258-        RPCResult{RPCResult::Type::NUM, "vsize", "virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted."},
259+        RPCResult{RPCResult::Type::NUM, "vsize", "maximum of sigop-adjusted size (-bytespersigop) and virtual transaction size as defined in BIP 141."},
260+        RPCResult{RPCResult::Type::NUM, "vsize_bip141", "virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted."},

ajtowns commented at 2:25 am on October 31, 2023:

I think rather than adding vsize_bip141 to mempool RPCs, it would be better to (a) document mempool vsize as being sigop-adjusted, and (b) document rawtransaction vsize as not being sigop-adjusted? (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?)

Could be sensible to add “sigopsize” for the cases where we have a sigop count – in that case vsize is simply max(floor((weight+3)/4), sigopsize), and the presence of a sigopsize with the exact same value as the weird vsize might be mostly self-explanatory as to what’s going on. (And for the normal case where the weight dominates, it actually shows why the sigop adjustment isn’t relevant)

glozow commented at 10:38 am on November 1, 2023:

IIUC the suggestion is:

Add docs for where vsize is sigop-adjusted and where it isn’t.
Add some doc/policy/?.md describing feerate, vsize, mining score
Add an optional “sigopsize” result for getrawtransaction and the mempool RPCs when we’re able to provide it

Will work on this

DrahtBot commented at 7:06 pm on December 1, 2023: contributor

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

DrahtBot added the label Needs rebase on Dec 1, 2023

kristapsk commented at 7:45 pm on December 1, 2023: contributor

Concept ACK

DrahtBot commented at 1:37 am on February 29, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

hebasto commented at 11:26 am on February 29, 2024: member

Concept ACK.

DrahtBot commented at 0:34 am on May 28, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

glozow commented at 12:37 pm on June 6, 2024: member

Sorry, I kept thinking I would get back to this but haven’t. Closing + marking up for grabs. Note for anybody who wants to pick this up, note you’ll want to write the doc described in #27591 (review)

glozow added the label Up for grabs on Jun 6, 2024

glozow closed this on Jun 6, 2024

rpc: distinguish between vsize and sigop-adjusted mempool vsize #27591

Code Coverage

Reviews

Conflicts