Bugfix: Package relay / bytespersigop checks #28345

pull luke-jr wants to merge 4 commits into bitcoin:master from luke-jr:fix_bytespersigop_checks changing 23 files +45 −22
  1. luke-jr commented at 9:46 pm on August 26, 2023: member

    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.

  2. DrahtBot commented at 9:46 pm on August 26, 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 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.

  3. luke-jr commented at 9:46 pm on August 26, 2023: member
    See also https://github.com/bitcoin-core/gui/issues/750
  4. glozow added the label TX fees and policy on Aug 29, 2023
  5. in src/txmempool.cpp:200 in fef601e2a4 outdated 
    196@@ -197,13 +197,15 @@ util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimit
197 }
198 
199 bool CTxMemPool::CheckPackageLimits(const Package& package,
200+                                    const std::vector<int64_t> &all_tx_vsizes,
    glozow commented at 9:01 am on August 29, 2023:
    in fef601e2a4d790781fee316c1a55ea48a134a0f1 Given that this function is only interested in the total size, why not just pass in a int64_t total_vsize?
    instagibbs commented at 4:26 pm on August 30, 2023:

    why not just pass in a int64_t total_vsize?

    Indeed, then you don’t have to gather all the vsizes twice as well.

    luke-jr commented at 5:24 pm on September 2, 2023:
    Done
  6. in src/validation.cpp:991 in fef601e2a4 outdated 
    987@@ -987,6 +988,7 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
988 }
989 
990 bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txns,
991+                                         const std::vector<int64_t> &all_tx_vsizes,
    glozow commented at 9:02 am on August 29, 2023:
    in fef601e2a4d790781fee316c1a55ea48a134a0f1 nit: perhaps pass the vector of workspaces instead? It would follow the pattern of the other MemPoolAccept functions and be more extendable for future stuff.
  7. in src/test/util/transaction_utils.h:30 in 56f5d5ee85 outdated 
    26@@ -26,4 +27,10 @@ CMutableTransaction BuildSpendingTransaction(const CScript& scriptSig, const CSc
27 // the second nValues[2] and nValues[3] outputs paid to a TxoutType::PUBKEYHASH.
28 std::vector<CMutableTransaction> SetupDummyInputs(FillableSigningProvider& keystoreRet, CCoinsViewCache& coinsRet, const std::array<CAmount,4>& nValues);
29 
30+// Assumes a non-spammy tx
    glozow commented at 9:09 am on August 29, 2023:

    in 56f5d5ee85c3ad87c9862192412b7957e478363c

    Not sure if it’s immediately clear what spammy would mean. Maybe something a bit more descriptive would be

    0// Calculates the BIP 141 virtual size, without consideration for sigops.
    luke-jr commented at 5:25 pm on September 2, 2023:
    Done
  8. in src/validation.cpp:1290 in 9307e5c6fc outdated 
    1276@@ -1277,6 +1277,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1277     // package feerate (total modified fees / total virtual size) to check this requirement.
1278     const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0},
1279         [](int64_t sum, auto& ws) { return sum + ws.m_vsize; });
1280+    if (m_total_vsize > MAX_PACKAGE_SIZE * 1000) {
1281+        package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large");
    glozow commented at 9:16 am on August 29, 2023:

    in 9307e5c6fc1786e5fc734da1685e1cde52f4dd61

    good catch, perhaps an error string like package-exceeds-max-vsize would help distinguish the two errors.

    luke-jr commented at 5:21 pm on September 2, 2023:
    Should they be distinguished?
    glozow commented at 10:05 am on September 12, 2023:

    I think these are distinct rules (where “vsize” means sigop-adjusted): (a) Each tx has a maximum vsize (b) Ancestor and descendant packages within the mempool cannot exceed 101kvB vsize (c) “Packages cannot exceed MAX_PACKAGE_SIZE=101KvB total size” (doc).

    The bug being fixed here is that we don’t properly enforce (b) when we’re validating packages. I think your commit “Bugfix: Pass correct virtual size to CheckPackageLimits” can be followed by something like this commit instead of a new “package-too-large” check.

    Imo the intent of (c) is to make sure we don’t work on packages that are too large / potentially expensive to validate. For that, I think we should just change the description of (c) to “Packages cannot exceed MAX_PACKAGE_SIZE=404kWu total weight.”

  9. in src/rpc/mempool.cpp:87 in fef601e2a4 outdated 
    83@@ -84,7 +84,7 @@ static RPCHelpMan sendrawtransaction()
84                                                      DEFAULT_MAX_RAW_TX_FEE_RATE :
85                                                      CFeeRate(AmountFromValue(request.params[1]));
86 
87-            int64_t virtual_size = GetVirtualTransactionSize(*tx);
88+            int64_t virtual_size = GetVirtualTransactionSize(*tx, 0, 0 /* BUG */);
    glozow commented at 9:20 am on August 29, 2023:
    Perhaps the way to fix this is to overload BroadcastTransaction() to accept either a CFeeRate, CAmount, or nothing. After the test-accept of the transaction, you can query MempoolAcceptResult::m_vsize to check the feerate.
    instagibbs commented at 4:32 pm on August 30, 2023:
    in general would be useful to note in a comment what that bug could possibly result in with respect to behavior, maybe a test case :)
    luke-jr commented at 5:27 pm on September 2, 2023:
    @glozow Isn’t that exactly the behaviour that we refactored out of (to checking locally here)?
    luke-jr commented at 2:23 am on September 5, 2023:
    Ah, I see what you mean. Fixed.
  10. glozow commented at 9:23 am on August 29, 2023: member
    concept ACK
  11. glozow requested review from instagibbs on Aug 30, 2023
  12. in src/policy/policy.h:161 in 56f5d5ee85 outdated 
    157@@ -158,11 +158,6 @@ int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost, unsigned
158 int64_t GetVirtualTransactionSize(const CTransaction& tx, int64_t nSigOpCost, unsigned int bytes_per_sigop);
159 int64_t GetVirtualTransactionInputSize(const CTxIn& tx, int64_t nSigOpCost, unsigned int bytes_per_sigop);
160 
161-static inline int64_t GetVirtualTransactionSize(const CTransaction& tx)
    instagibbs commented at 4:03 pm on August 30, 2023:
    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
    glozow commented at 12:19 pm on September 26, 2023:

    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 1b163ae71697869899380a672966881224ca42ea would feel even safer if the new function was called GetBIP141VirtualTransactionSize or even GetUnsafeVirtualTransactionSize since it’s 99% test-only.

  13. in src/qt/walletmodeltransaction.h:32 in 56f5d5ee85 outdated 
    28@@ -29,6 +29,7 @@ class WalletModelTransaction
29     CTransactionRef& getWtx();
30     void setWtx(const CTransactionRef&);
31 
32+    //! WARNING: Currently only safe for self-generated transactions due to bypassing sigop logic of GetVirtualTransactionSize
    instagibbs commented at 4:06 pm on August 30, 2023:
    seems a bit scary with no actionable suggestion?
    luke-jr commented at 5:28 pm on September 2, 2023:
    I don’t see any obvious actionable solution (https://github.com/bitcoin-core/gui/issues/750). But this is currently only called where it is safe.
  14. instagibbs commented at 4:28 pm on August 30, 2023: member
    concept ACK
  15. luke-jr force-pushed on Sep 2, 2023
  16. luke-jr commented at 6:31 pm on September 2, 2023: member
    checkChainLimits should probably get a warning-only-for-wallet-use comment too
  17. DrahtBot added the label CI failed on Sep 2, 2023
  18. DrahtBot removed the label CI failed on Sep 4, 2023
  19. luke-jr force-pushed on Sep 5, 2023
  20. DrahtBot added the label CI failed on Sep 5, 2023
  21. luke-jr referenced this in commit 9868bf2457 on Sep 5, 2023
  22. luke-jr referenced this in commit 341f4ea3fc on Sep 5, 2023
  23. luke-jr referenced this in commit afe139de70 on Sep 5, 2023
  24. luke-jr referenced this in commit b649f95a28 on Sep 5, 2023
  25. luke-jr referenced this in commit d191c7f0b8 on Sep 5, 2023
  26. DrahtBot added the label Needs rebase on Sep 6, 2023
  27. instagibbs commented at 8:37 pm on September 6, 2023: member
    could we work on getting tests working? looking to do some deeper review
  28. achow101 referenced this in commit 2303fd2f43 on Sep 21, 2023
  29. luke-jr force-pushed on Sep 22, 2023
  30. luke-jr commented at 4:05 am on September 22, 2023: member

    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.

  31. DrahtBot removed the label Needs rebase on Sep 22, 2023
  32. glozow commented at 2:21 pm on September 22, 2023: member

    @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 https://github.com/bitcoin/bitcoin/pull/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 https://github.com/bitcoin/bitcoin/pull/28345#issuecomment-1730772463, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGAEGGMJ5MNIMSGJ2OD4IVTX3UFBZANCNFSM6AAAAAA3727CQI . You are receiving this because you were mentioned.Message ID: @.***>

  33. luke-jr force-pushed on Sep 22, 2023
  34. luke-jr force-pushed on Sep 22, 2023
  35. luke-jr commented at 6:03 pm on September 22, 2023: member

    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.

  36. policy: Delete buggy GetVirtualTransactionSize(CTransaction&) 1b163ae716
  37. node: Refactor BroadcastTransaction to accept a CFeeRate maximum 7864910e43
  38. Bugfix: RPC/Mempool: Pass CFeeRate to BroadcastTransaction to correctly account for non-weight vsize 8aebafd4ba
  39. Bugfix: GUI/PSBTOperationsDialog: Pass feerate limit as a CFeeRate instead of assuming 1kvB 78a256505f
  40. luke-jr force-pushed on Sep 22, 2023
  41. DrahtBot removed the label CI failed on Sep 22, 2023
  42. Frank-GER referenced this in commit da842f271a on Sep 25, 2023
  43. glozow commented at 1:34 pm on September 26, 2023: member
    LGTM though agree with @instagibbs’ suggestion for the first commit. Could update the title + description? No longer package relay-related.
  44. sidhujag referenced this in commit 12b1a34dce on Sep 26, 2023
  45. luke-jr referenced this in commit 14c2af07ae on Oct 20, 2023
  46. luke-jr referenced this in commit b152fb564a on Oct 20, 2023
  47. luke-jr referenced this in commit c10f1326aa on Oct 20, 2023
  48. DrahtBot commented at 11:13 am on November 8, 2023: contributor

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

  49. DrahtBot added the label Needs rebase on Nov 8, 2023
  50. ismaelsadeeq commented at 12:36 pm on November 15, 2023: member
    Concept ACK
  51. DrahtBot commented at 10:46 am on January 5, 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.
  52. DrahtBot commented at 1:51 am on April 3, 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.
  53. achow101 added the label Up for grabs on Apr 9, 2024
  54. achow101 commented at 2:17 pm on April 9, 2024: member
    Closing as up for grabs due to lack of activity.
  55. achow101 closed this on Apr 9, 2024


luke-jr DrahtBot glozow instagibbs ismaelsadeeq achow101

Labels
TX fees and policy Up for grabs Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-30 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me