Bugfix: Package relay / bytespersigop checks #28345
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
See also bitcoin-core/gui#750 |
src/policy/policy.h
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
luke-jr
force-pushed
the
fix_bytespersigop_checks
branch
from
fef601e to
9426712
Compare
|
|
luke-jr
force-pushed
the
fix_bytespersigop_checks
branch
from
9426712 to
6cd57e5
Compare
Github-Pull: bitcoin#28345 Rebased-From: a87d7c63fcf5c7d0152afcc8164ec415b035fb58
Github-Pull: bitcoin#28345 Rebased-From: e7a1cf071fc40ac06ca9b526580847d32c32be39
Github-Pull: bitcoin#28345 Rebased-From: 46e993c57e212725418aa3a6d8495a454b56b344
…ly account for non-weight vsize Github-Pull: bitcoin#28345 Rebased-From: ae232c72384c4b2b0cacb85eaeedd79fa78a9f73
…stead of assuming 1kvB Github-Pull: bitcoin#28345 Rebased-From: 6cd57e509c2f72ce4cd3abd71f18527c0a839695
|
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
force-pushed
the
fix_bytespersigop_checks
branch
from
6cd57e5 to
6b6554b
Compare
|
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. |
|
@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
force-pushed
the
fix_bytespersigop_checks
branch
from
6b6554b to
00b11c9
Compare
luke-jr
force-pushed
the
fix_bytespersigop_checks
branch
from
00b11c9 to
0c78cd8
Compare
Ok, dropping that (and the revert) then. |
…ly account for non-weight vsize
…stead of assuming 1kvB
luke-jr
force-pushed
the
fix_bytespersigop_checks
branch
from
0c78cd8 to
78a2565
Compare
…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
left a comment
glozow
left a comment
There was a problem hiding this comment.
LGTM though agree with @instagibbs' suggestion for the first commit. Could update the title + description? No longer package relay-related.
src/policy/policy.h
Outdated
There was a problem hiding this comment.
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: 46e993c57e212725418aa3a6d8495a454b56b344
…ly account for non-weight vsize Github-Pull: bitcoin#28345 Rebased-From: ae232c72384c4b2b0cacb85eaeedd79fa78a9f73
…stead of assuming 1kvB Github-Pull: bitcoin#28345 Rebased-From: 6cd57e509c2f72ce4cd3abd71f18527c0a839695
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Closing as up for grabs due to lack of activity. |
6 participants
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 tobitcoin_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
sendrawtransactionRPC and GUI transaction details remain buggy and not fixed in this PR.