Bugfix: Package relay / bytespersigop checks #28345

luke-jr · 2023-08-26T21:46:20Z

GetVirtualTransactionSize(CTransaction&) passes by default 0 sigops and 0 -bytespersigop, which is incorrect. In many cases (ie, wallet), this is harmless since we have control over the transactions and don't do anything absurd in normal usage. But in other cases (including wallet received transactions), this isn't safe. To avoid invisible bugs like this, I delete (or rather, move to bitcoin_test) the function signature that allows for omitting sigop inputs.

This also then fixes the new package relay code to use the correct virtual sizes for its checks, and documents calling locations where the behaviour is broken or safe.

In particular, note that the sendrawtransaction RPC and GUI transaction details remain buggy and not fixed in this PR.

DrahtBot · 2023-08-26T21:46:23Z

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	glozow, instagibbs, ismaelsadeeq

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:

#28813 (rpc: permit any ancestor package through submitpackage by glozow)
#28785 (validation: return more helpful results for reconsiderable fee failures and skipped transactions by glozow)
#26711 (validate package transactions with their in-package ancestor sets by glozow)

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.

luke-jr · 2023-08-26T21:46:52Z

See also bitcoin-core/gui#750

glozow

concept ACK

src/txmempool.cpp

src/validation.cpp

src/test/util/transaction_utils.h

src/validation.cpp

src/rpc/mempool.cpp

instagibbs

concept ACK

instagibbs · 2023-08-30T16:03:59Z

src/policy/policy.h

@@ -158,11 +158,6 @@ int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost, unsigned
 int64_t GetVirtualTransactionSize(const CTransaction& tx, int64_t nSigOpCost, unsigned int bytes_per_sigop);
 int64_t GetVirtualTransactionInputSize(const CTxIn& tx, int64_t nSigOpCost, unsigned int bytes_per_sigop);

-static inline int64_t GetVirtualTransactionSize(const CTransaction& tx)


Alternative, move to wallet and rename to GetWalletVirtualTransactionSize with tests/assertions/comments saying why it's safe in one location instead of spread around? Would make grepping for the remaining bugs easier as well. e.g., anything like GetVirtualTransactionSize(tx, 0, 0) would be obviously a bug at that point

Would make grepping for the remaining bugs easier as well. e.g., anything like GetVirtualTransactionSize(tx, 0, 0) would be obviously a bug at that point

+1 to this, I think 1b163ae would feel even safer if the new function was called GetBIP141VirtualTransactionSize or even GetUnsafeVirtualTransactionSize since it's 99% test-only.

src/qt/walletmodeltransaction.h

src/txmempool.cpp

luke-jr · 2023-09-02T18:31:20Z

checkChainLimits should probably get a warning-only-for-wallet-use comment too

Github-Pull: bitcoin#28345 Rebased-From: a87d7c6

Github-Pull: bitcoin#28345 Rebased-From: e7a1cf0

Github-Pull: bitcoin#28345 Rebased-From: 46e993c

…ly account for non-weight vsize Github-Pull: bitcoin#28345 Rebased-From: ae232c7

…stead of assuming 1kvB Github-Pull: bitcoin#28345 Rebased-From: 6cd57e5

instagibbs · 2023-09-06T20:37:34Z

could we work on getting tests working? looking to do some deeper review

…ansaction package context eb8f58f Add functional test to catch too large vsize packages (Greg Sanders) 1a579f9 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders) bc013fe Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr) 533660c Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders) Pull request description: (Alternative) Minimal subset of bitcoin/bitcoin#28345 to: 1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion. 2) pass correct vsize to chain limit evaluations in package context 3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits) This should fix the known issues while not blocking additional refactoring later. ACKs for top commit: achow101: ACK eb8f58f ariard: Re-Code ACK eb8f58f glozow: reACK eb8f58f Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39

luke-jr · 2023-09-22T04:05:36Z

The CI failures appear to have been because CFeeRate(0) needed to be special-cased as "no limit" also. Fixed that.

Rebased on top of #28471, partially by reverting its first commit which appears to simply make the bug harder to fix.

glozow · 2023-09-22T14:21:48Z

@luke-jr I don’t think adding a new vsize-based total package limit is necessary, the existing ancestor size limits are sufficient. #28471 already incorporated your bug fix to have CheckPackageLimits look at package total vsize when enforcing ancestor size limits. If you feel there’s an additional bug, can you please write a test that fails on #28471 but passes on this branch? El El vie, 22 sept 2023 a las 4:05, Luke Dashjr ***@***.***> escribió:

…

The CI failures appear to have been because CFeeRate(0) needed to be special-cased as "no limit" also. Fixed that. Rebased on top of #28471 <#28471>, partially by reverting its first commit which appears to simply make the bug harder to fix. — Reply to this email directly, view it on GitHub <#28345 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGAEGGMJ5MNIMSGJ2OD4IVTX3UFBZANCNFSM6AAAAAA3727CQI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

luke-jr · 2023-09-22T18:03:34Z

I don’t think adding a new vsize-based total package limit is
necessary, the existing ancestor size limits are sufficient.

Ok, dropping that (and the revert) then.

…ly account for non-weight vsize

…stead of assuming 1kvB

…n package context eb8f58f Add functional test to catch too large vsize packages (Greg Sanders) 1a579f9 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders) bc013fe Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr) 533660c Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders) Pull request description: (Alternative) Minimal subset of bitcoin#28345 to: 1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion. 2) pass correct vsize to chain limit evaluations in package context 3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits) This should fix the known issues while not blocking additional refactoring later. ACKs for top commit: achow101: ACK eb8f58f ariard: Re-Code ACK eb8f58f glozow: reACK eb8f58f Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39

glozow

LGTM though agree with @instagibbs' suggestion for the first commit. Could update the title + description? No longer package relay-related.

glozow · 2023-09-26T12:19:12Z

src/policy/policy.h

@@ -158,11 +158,6 @@ int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost, unsigned
 int64_t GetVirtualTransactionSize(const CTransaction& tx, int64_t nSigOpCost, unsigned int bytes_per_sigop);
 int64_t GetVirtualTransactionInputSize(const CTxIn& tx, int64_t nSigOpCost, unsigned int bytes_per_sigop);

-static inline int64_t GetVirtualTransactionSize(const CTransaction& tx)


Would make grepping for the remaining bugs easier as well. e.g., anything like GetVirtualTransactionSize(tx, 0, 0) would be obviously a bug at that point

+1 to this, I think 1b163ae would feel even safer if the new function was called GetBIP141VirtualTransactionSize or even GetUnsafeVirtualTransactionSize since it's 99% test-only.

…n package context eb8f58f Add functional test to catch too large vsize packages (Greg Sanders) 1a579f9 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders) bc013fe Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr) 533660c Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders) Pull request description: (Alternative) Minimal subset of bitcoin#28345 to: 1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion. 2) pass correct vsize to chain limit evaluations in package context 3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits) This should fix the known issues while not blocking additional refactoring later. ACKs for top commit: achow101: ACK eb8f58f ariard: Re-Code ACK eb8f58f glozow: reACK eb8f58f Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39

Github-Pull: bitcoin#28345 Rebased-From: 46e993c

…ly account for non-weight vsize Github-Pull: bitcoin#28345 Rebased-From: ae232c7

…stead of assuming 1kvB Github-Pull: bitcoin#28345 Rebased-From: 6cd57e5

DrahtBot · 2023-11-08T11:13:08Z

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

ismaelsadeeq

Concept ACK

DrahtBot · 2024-01-05T10:46:50Z

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.

DrahtBot · 2024-04-03T01:51:00Z

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

achow101 · 2024-04-09T14:17:17Z

Closing as up for grabs due to lack of activity.

This was referenced Aug 27, 2023

Wallet: don't underestimate the fees when spending a Taproot output #26573

Open

Wallet: estimate the size of signed inputs using descriptors #26567

Merged

policy: nVersion=3 and Package RBF #25038

Closed

glozow added the TX fees and policy label Aug 29, 2023

glozow reviewed Aug 29, 2023

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/test/util/transaction_utils.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

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

glozow requested a review from instagibbs August 30, 2023 09:16

instagibbs reviewed Aug 30, 2023

View reviewed changes

luke-jr force-pushed the fix_bytespersigop_checks branch from fef601e to 9426712 Compare September 2, 2023 17:24

DrahtBot added CI failed and removed CI failed labels Sep 2, 2023

luke-jr force-pushed the fix_bytespersigop_checks branch from 9426712 to 6cd57e5 Compare September 5, 2023 02:24

DrahtBot added the CI failed label Sep 5, 2023

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Sep 5, 2023

Bugfix: Re-check package vsize limit taking sigops into account

9868bf2

Github-Pull: bitcoin#28345 Rebased-From: a87d7c6

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Sep 5, 2023

Bugfix: Pass correct virtual size to CheckPackageLimits

341f4ea

Github-Pull: bitcoin#28345 Rebased-From: e7a1cf0

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Sep 5, 2023

node: Refactor BroadcastTransaction to accept a CFeeRate maximum

afe139d

Github-Pull: bitcoin#28345 Rebased-From: 46e993c

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Sep 5, 2023

Bugfix: RPC/Mempool: Pass CFeeRate to BroadcastTransaction to correct…

b649f95

…ly account for non-weight vsize Github-Pull: bitcoin#28345 Rebased-From: ae232c7

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Sep 5, 2023

Bugfix: GUI/PSBTOperationsDialog: Pass feerate limit as a CFeeRate in…

d191c7f

…stead of assuming 1kvB Github-Pull: bitcoin#28345 Rebased-From: 6cd57e5

DrahtBot added the Needs rebase label Sep 6, 2023

glozow mentioned this pull request Sep 11, 2023

Package Relay Project Tracking #27463

Open

56 tasks

instagibbs mentioned this pull request Sep 13, 2023

Fix virtual size limit enforcement in transaction package context #28471

Merged

luke-jr force-pushed the fix_bytespersigop_checks branch from 6cd57e5 to 6b6554b Compare September 22, 2023 04:04

DrahtBot removed the Needs rebase label Sep 22, 2023

luke-jr force-pushed the fix_bytespersigop_checks branch from 6b6554b to 00b11c9 Compare September 22, 2023 17:56

luke-jr force-pushed the fix_bytespersigop_checks branch from 00b11c9 to 0c78cd8 Compare September 22, 2023 18:03

luke-jr added 4 commits September 22, 2023 20:36

policy: Delete buggy GetVirtualTransactionSize(CTransaction&)

1b163ae

node: Refactor BroadcastTransaction to accept a CFeeRate maximum

7864910

Bugfix: RPC/Mempool: Pass CFeeRate to BroadcastTransaction to correct…

8aebafd

…ly account for non-weight vsize

Bugfix: GUI/PSBTOperationsDialog: Pass feerate limit as a CFeeRate in…

78a2565

…stead of assuming 1kvB

luke-jr force-pushed the fix_bytespersigop_checks branch from 0c78cd8 to 78a2565 Compare September 22, 2023 20:37

DrahtBot removed the CI failed label Sep 22, 2023

glozow reviewed Sep 26, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 27, 2023

validate package transactions with their in-package ancestor sets #26711

Closed

DrahtBot mentioned this pull request Oct 18, 2023

[WIP] Cluster mempool implementation #28676

Draft

8 tasks

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2023

node: Refactor BroadcastTransaction to accept a CFeeRate maximum

14c2af0

Github-Pull: bitcoin#28345 Rebased-From: 46e993c

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2023

Bugfix: RPC/Mempool: Pass CFeeRate to BroadcastTransaction to correct…

b152fb5

…ly account for non-weight vsize Github-Pull: bitcoin#28345 Rebased-From: ae232c7

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2023

Bugfix: GUI/PSBTOperationsDialog: Pass feerate limit as a CFeeRate in…

c10f132

…stead of assuming 1kvB Github-Pull: bitcoin#28345 Rebased-From: 6cd57e5

This was referenced Oct 31, 2023

Fuzz: Check individual and package transaction invariants #28764

Merged

validation: return more helpful results for reconsiderable fee failures and skipped transactions #28785

Merged

rpc: permit any ancestor package through submitpackage #28813

Closed

DrahtBot added the Needs rebase label Nov 8, 2023

ismaelsadeeq reviewed Nov 15, 2023

View reviewed changes

ismaelsadeeq mentioned this pull request Dec 21, 2023

v3 transaction policy for anti-pinning #28948

Merged

achow101 added the Up for grabs label Apr 9, 2024

achow101 closed this Apr 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Package relay / bytespersigop checks #28345

Bugfix: Package relay / bytespersigop checks #28345

luke-jr commented Aug 26, 2023

DrahtBot commented Aug 26, 2023 •

edited

luke-jr commented Aug 26, 2023

glozow left a comment

instagibbs left a comment

instagibbs Aug 30, 2023

glozow Sep 26, 2023

luke-jr commented Sep 2, 2023

instagibbs commented Sep 6, 2023

luke-jr commented Sep 22, 2023

glozow commented Sep 22, 2023 via email

luke-jr commented Sep 22, 2023

glozow left a comment

glozow Sep 26, 2023

DrahtBot commented Nov 8, 2023

ismaelsadeeq left a comment

DrahtBot commented Jan 5, 2024

DrahtBot commented Apr 3, 2024

achow101 commented Apr 9, 2024

Bugfix: Package relay / bytespersigop checks #28345

Bugfix: Package relay / bytespersigop checks #28345

Conversation

luke-jr commented Aug 26, 2023

DrahtBot commented Aug 26, 2023 • edited

Code Coverage

Reviews

Conflicts

luke-jr commented Aug 26, 2023

glozow left a comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

instagibbs Aug 30, 2023

Choose a reason for hiding this comment

glozow Sep 26, 2023

Choose a reason for hiding this comment

luke-jr commented Sep 2, 2023

instagibbs commented Sep 6, 2023

luke-jr commented Sep 22, 2023

glozow commented Sep 22, 2023 via email

luke-jr commented Sep 22, 2023

glozow left a comment

Choose a reason for hiding this comment

glozow Sep 26, 2023

Choose a reason for hiding this comment

DrahtBot commented Nov 8, 2023

ismaelsadeeq left a comment

Choose a reason for hiding this comment

DrahtBot commented Jan 5, 2024

DrahtBot commented Apr 3, 2024

achow101 commented Apr 9, 2024

DrahtBot commented Aug 26, 2023 •

edited