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

glozow · 2023-05-07T19:44:25Z

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

DrahtBot · 2023-05-07T19:44:28Z

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.

src/rpc/mempool.cpp

mononaut · 2023-05-08T13:48:44Z

Concept ACK

Sjors · 2023-05-08T14:24:21Z

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 · 2023-06-23T16:38:22Z

Concept ACK, but should be split into two commits.

…p size CTxMemPoolEntry::GetTxSize() returns the maximum between vsize and "sigop size" which is used internally. Users of the RPC interface are probably not expecting this result from "vsize" (as it is not documented). Fix this, and add a separate result for "mempool_vsize". Github-Pull: bitcoin#27591 Rebased-From: 60bde2d

luke-jr · 2023-08-24T22:41:16Z

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

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

glozow · 2023-08-25T13:57:59Z

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.

luke-jr · 2023-08-28T18:14:39Z

src/rpc/mempool.cpp

@@ -130,7 +130,9 @@ static RPCHelpMan testmempoolaccept()
                    {RPCResult::Type::STR, "package-error", /*optional=*/true, "Package validation error, if any (only possible if rawtxs had more than 1 transaction)."},
                    {RPCResult::Type::BOOL, "allowed", /*optional=*/true, "Whether this tx would be accepted to the mempool and pass client-specified maxfeerate. "
                                                       "If not present, the tx was not fully validated due to a failure in another tx in the list."},
-                    {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)"},
+                    {RPCResult::Type::NUM, "vsize", /*optional=*/true, "Maximum of sigop-adjusted size (-bytespersigop) and virtual transaction size as defined in BIP 141."},


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

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

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 · 2023-09-06T19:13:44Z

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 · 2023-09-06T19:20:34Z

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 · 2023-09-07T16:50:21Z

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 · 2023-10-20T03:39:04Z

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

ajtowns · 2023-10-31T02:05:31Z

src/rpc/mempool.cpp

@@ -840,7 +845,8 @@ static RPCHelpMan submitpackage()
                    {RPCResult::Type::OBJ, "wtxid", "transaction wtxid", {
                        {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
                        {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."},
-                        {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."},
+                        {RPCResult::Type::NUM, "vsize", "Maximum of sigop-adjusted size (-bytespersigop) and virtual transaction size as defined in BIP 141."},
+                        {RPCResult::Type::NUM, "vsize_bip141", "Virtual transaction size as defined in BIP 141."},


PR description needs updating from "mempool_vsize"

ajtowns · 2023-10-31T02:19:57Z

src/rpc/mempool.cpp

@@ -130,7 +130,9 @@ static RPCHelpMan testmempoolaccept()
                    {RPCResult::Type::STR, "package-error", /*optional=*/true, "Package validation error, if any (only possible if rawtxs had more than 1 transaction)."},
                    {RPCResult::Type::BOOL, "allowed", /*optional=*/true, "Whether this tx would be accepted to the mempool and pass client-specified maxfeerate. "
                                                       "If not present, the tx was not fully validated due to a failure in another tx in the list."},
-                    {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)"},
+                    {RPCResult::Type::NUM, "vsize", /*optional=*/true, "Maximum of sigop-adjusted size (-bytespersigop) and virtual transaction size as defined in BIP 141."},


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

ajtowns · 2023-10-31T02:25:40Z

src/rpc/mempool.cpp

@@ -252,7 +255,8 @@ static RPCHelpMan testmempoolaccept()
 static std::vector<RPCResult> MempoolEntryDescription()
 {
    return {
-        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."},
+        RPCResult{RPCResult::Type::NUM, "vsize", "maximum of sigop-adjusted size (-bytespersigop) and virtual transaction size as defined in BIP 141."},
+        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."},


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)

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 · 2023-12-01T19:06:30Z

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

kristapsk · 2023-12-01T19:45:29Z

Concept ACK

DrahtBot · 2024-02-29T01:37:01Z

⌛ 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 · 2024-02-29T11:26:03Z

Concept ACK.

DrahtBot · 2024-05-28T00:34:16Z

⌛ 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 · 2024-06-06T12:37:38Z

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 (comment)

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

maflcko reviewed May 8, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

glozow added the Needs Conceptual Review label May 8, 2023

glozow added 2 commits August 25, 2023 14:20

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

6a371bc

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

9cf46b6

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

glozow force-pushed the 2023-05-mempool-vsize branch from 60bde2d to 9cf46b6 Compare August 25, 2023 13:56

luke-jr reviewed Aug 28, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 18, 2023

[WIP] Cluster mempool implementation #28676

Draft

8 tasks

ajtowns reviewed Oct 31, 2023

View reviewed changes

DrahtBot mentioned this pull request Nov 10, 2023

bugfix, Change up submitpackage results to return results for all transactions #28848

Merged

DrahtBot added the Needs rebase label Dec 1, 2023

glozow mentioned this pull request May 22, 2024

BIP431: Opt In Topologically Restricted Until Confirmation Transactions For More Robust Fee-bumping bitcoin/bips#1541

Merged

glozow added the Up for grabs label Jun 6, 2024

glozow closed this Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

glozow commented May 7, 2023

DrahtBot commented May 7, 2023 •

edited

mononaut commented May 8, 2023

Sjors commented May 8, 2023 •

edited

luke-jr commented Jun 23, 2023

luke-jr commented Aug 24, 2023

glozow commented Aug 25, 2023

luke-jr Aug 28, 2023

glozow Aug 29, 2023

ajtowns Oct 31, 2023

stevenroose commented Sep 6, 2023

sipa commented Sep 6, 2023 •

edited

stevenroose commented Sep 7, 2023

kevkevinpal commented Oct 20, 2023 •

edited

ajtowns Oct 31, 2023

ajtowns Oct 31, 2023

ajtowns Oct 31, 2023

glozow Nov 1, 2023

DrahtBot commented Dec 1, 2023

kristapsk commented Dec 1, 2023

DrahtBot commented Feb 29, 2024

hebasto commented Feb 29, 2024

DrahtBot commented May 28, 2024

glozow commented Jun 6, 2024

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

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

Conversation

glozow commented May 7, 2023

DrahtBot commented May 7, 2023 • edited

Code Coverage

Reviews

Conflicts

mononaut commented May 8, 2023

Sjors commented May 8, 2023 • edited

luke-jr commented Jun 23, 2023

luke-jr commented Aug 24, 2023

glozow commented Aug 25, 2023

luke-jr Aug 28, 2023

Choose a reason for hiding this comment

glozow Aug 29, 2023

Choose a reason for hiding this comment

ajtowns Oct 31, 2023

Choose a reason for hiding this comment

stevenroose commented Sep 6, 2023

sipa commented Sep 6, 2023 • edited

stevenroose commented Sep 7, 2023

kevkevinpal commented Oct 20, 2023 • edited

ajtowns Oct 31, 2023

Choose a reason for hiding this comment

ajtowns Oct 31, 2023

Choose a reason for hiding this comment

ajtowns Oct 31, 2023

Choose a reason for hiding this comment

glozow Nov 1, 2023

Choose a reason for hiding this comment

DrahtBot commented Dec 1, 2023

kristapsk commented Dec 1, 2023

DrahtBot commented Feb 29, 2024

hebasto commented Feb 29, 2024

DrahtBot commented May 28, 2024

glozow commented Jun 6, 2024

DrahtBot commented May 7, 2023 •

edited

Sjors commented May 8, 2023 •

edited

sipa commented Sep 6, 2023 •

edited

kevkevinpal commented Oct 20, 2023 •

edited