fix assert crash when specified change output spend size is unknown #14380

pull instagibbs wants to merge 2 commits into bitcoin:master from instagibbs:unknown_change_size changing 4 files +59 −3
  1. instagibbs commented at 5:45 am on October 3, 2018: member

    This is triggered anytime a fundraw type call(psbt or legacy) is used with a change output address that the wallet doesn’t know how to sign for.

    This regression was added in 6a34ff5335786615771ca423134a484b04831c4e since BnB coin selection actually cares about this.

    The fix is to assume the smallest typical spend, a P2SH-P2WPKH, which is calculated using a “prototype” dummy signature flow. Future work could generalize this infrastructure to get estimated sizes of inputs for a variety of types.

    I also removed a comment which I believe is stale and misleading.

  2. instagibbs commented at 5:45 am on October 3, 2018: member
    Opening this to see if people think this is the right solution.
  3. fanquake added the label Wallet on Oct 3, 2018
  4. instagibbs force-pushed on Oct 3, 2018
  5. practicalswift commented at 7:57 am on October 3, 2018: contributor
    Very nice find!
  6. gmaxwell commented at 11:45 pm on October 3, 2018: contributor

    The code wants to know the change signature size so it can figure out if it would have created change that was worthless to spend (because the spending will take more fees than the output is worth).

    Assuming the size is smallest plausible signature size would be conservative: it’ll overvalue the change, at a risk of keeping change it could have eliminated. I think doing that would be strictly superior to not using BnB at all, since preventing BnB will always keep change it should eliminate.

    So, e.g. assuming any spend of an unknown type will take at least 32(txid)+4(vout)+4(sequence)+1(len)+64*0.25(signature) = 57 bytes would work. (don’t assume my math is right. :) )

  7. instagibbs commented at 11:52 pm on October 3, 2018: member
    Hm yes I suppose you’re right. Not running BnB is admitting defeat even if we actually end up being able to discard a larger value. I’ll take a look at constructing something that makes a minimal segwit input weight.
  8. DrahtBot commented at 5:47 am on October 4, 2018: member
    Coverage Change (pull 14380) Reference (master)
    Lines -0.0083 % 87.0471 %
    Functions +0.0154 % 84.1130 %
    Branches +0.0057 % 51.5403 %
  9. in src/wallet/wallet.cpp:2732 in 268f4952d9 outdated 
    2727@@ -2730,6 +2728,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
2728                     nValueIn = 0;
2729                     setCoins.clear();
2730                     coin_selection_params.change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
2731+                    // If the wallet doesn't know how to sign change output, don't rely on it
2732+                    if (coin_selection_params.change_spend_size == -1) {
    practicalswift commented at 9:29 am on October 4, 2018:
    change_spend_size is size_t and thus cannot be -1 :-)
  10. practicalswift commented at 9:42 am on October 4, 2018: contributor

    Isn’t the problem that coin_selection_params.change_spend_size is set to std::numeric_limits<size_t>::max() (SIZE_MAX) instead of the intended -1 since change_spend_size is of type size_t?

    0[cling]$ size_t change_spend_size = -1;
1[cling]$ change_spend_size
2(unsigned long) 18446744073709551615
3[cling]$ (size_t)-1
4(unsigned long) 18446744073709551615
5[cling]$ #include <limits>
6[cling]$ std::numeric_limits<size_t>::max()
7(unsigned long) 18446744073709551615

    The definition of change_spend_size:

    https://github.com/bitcoin/bitcoin/blob/b012bbe358311cc4a73300ccca41b64272250277/src/wallet/wallet.h#L573

    CalculateMaximumSignedInputSize(…) returns -1 here:

    https://github.com/bitcoin/bitcoin/blob/b012bbe358311cc4a73300ccca41b64272250277/src/wallet/wallet.cpp#L1510-L1520

    Thechange_spend_size assignment is performed here:

    https://github.com/bitcoin/bitcoin/blob/b012bbe358311cc4a73300ccca41b64272250277/src/wallet/wallet.cpp#L2725

    Related open pull requests in need of review:

    • #14252 – build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) (includes -fsanitize=integer which tackles unsigned integer wraparounds (non-UB!))
    • #14226 – mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false
    • #14224 – Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations
    • #11551 – Fix unsigned integer wrap-around in GetBlockProofEquivalentTime
    • #11535 – Avoid unintentional unsigned integer wraparounds

    Please help reviewing those :-)

  11. instagibbs commented at 3:40 am on October 5, 2018: member
    @practicalswift yes that’s why the assert is finally hit, but it’s faulty logic regardless
  12. instagibbs force-pushed on Oct 5, 2018
  13. instagibbs commented at 5:20 am on October 5, 2018: member
    Pushed a fix in line with @gmaxwell suggestions. It’s pretty hardcoded, so I’d rather make this more programmatic, perhaps giving a “hint” to the dummy signer of what type it should be?
  14. in src/wallet/wallet.cpp:1521 in 9bde981dd0 outdated 
    1513@@ -1514,13 +1514,19 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
1514     return GetVirtualTransactionSize(txNew);
1515 }
1516 
1517+/* Prevout(32*4+4*4) + sequence(4*4) + scriptsig len(1*4) + scriptsig(22*4)
1518+* witness signature(1*71) + pubkey(1*33) + witness stack extra bytes(3)
1519+* converted to virtual size. This is used when estimating change spend size that
1520+* the wallet doesn't own or know how to sign for. */
1521+int EstimateP2SHP2WPKHInputSize() {
    practicalswift commented at 6:55 am on October 5, 2018:

    Make this constexpr :-)

    Opt-in to internal linkage using static or use the unnamed namespace.

  15. in src/wallet/wallet.cpp:1522 in 9bde981dd0 outdated 
    1513@@ -1514,13 +1514,19 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
1514     return GetVirtualTransactionSize(txNew);
1515 }
1516 
1517+/* Prevout(32*4+4*4) + sequence(4*4) + scriptsig len(1*4) + scriptsig(22*4)
1518+* witness signature(1*71) + pubkey(1*33) + witness stack extra bytes(3)
1519+* converted to virtual size. This is used when estimating change spend size that
1520+* the wallet doesn't own or know how to sign for. */
1521+int EstimateP2SHP2WPKHInputSize() {
1522+    return (128+16+16+4+88+71+33+3)/4;
    practicalswift commented at 6:57 am on October 5, 2018:
    Add whitespace to improve readability: return (128 + 16 + 16 + 4 + 88 + 71 + 33 + 3) / 4;
  16. in src/wallet/wallet.cpp:2742 in 9bde981dd0 outdated 
    2734@@ -2729,7 +2735,14 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
2735                 if (pick_new_inputs) {
2736                     nValueIn = 0;
2737                     setCoins.clear();
2738-                    coin_selection_params.change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
2739+                    int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
2740+                    // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh
2741+                    // as lower-bound to allow BnB to do it's thing
2742+                    if (change_spend_size == -1) {
2743+                        coin_selection_params.change_spend_size = EstimateP2SHP2WPKHInputSize();
    practicalswift commented at 6:57 am on October 5, 2018:
    Make this implicit conversion explicit :-)
  17. in src/wallet/wallet.cpp:2744 in 9bde981dd0 outdated 
    2740+                    // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh
2741+                    // as lower-bound to allow BnB to do it's thing
2742+                    if (change_spend_size == -1) {
2743+                        coin_selection_params.change_spend_size = EstimateP2SHP2WPKHInputSize();
2744+                    } else {
2745+                        coin_selection_params.change_spend_size = change_spend_size;
    practicalswift commented at 6:57 am on October 5, 2018:
    Same here :-)
  18. DrahtBot commented at 10:47 am on October 5, 2018: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14711 (Remove uses of chainActive and mapBlockIndex in wallet code by ryanofsky)
    • #10973 (Refactor: separate wallet from node by ryanofsky)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    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.

  19. achow101 commented at 10:42 pm on October 5, 2018: member
    Concept ACK
  20. instagibbs force-pushed on Oct 6, 2018
  21. instagibbs commented at 5:23 am on October 6, 2018: member
    Updated the PR to use a function that uses DummySigner flow with an arbitrary valid pubkey as the P2SH-P2WPKH target to estimate the size more programatically.
  22. in src/wallet/wallet.cpp:2769 in 5db87a1a14 outdated 
    2761@@ -2729,7 +2762,14 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
2762                 if (pick_new_inputs) {
2763                     nValueIn = 0;
2764                     setCoins.clear();
2765-                    coin_selection_params.change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
2766+                    int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, this);
2767+                    // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh
2768+                    // as lower-bound to allow BnB to do it's thing
2769+                    if (change_spend_size == -1) {
2770+                        coin_selection_params.change_spend_size = (size_t)CalculateNestedKeyhashInputSize(false);
    practicalswift commented at 9:31 am on October 7, 2018:
    We should handle the case of -1 here too, or if CalculateNestedKeyhashInputSize is guaranteed to never return negative values it should return an unsigned type? :-)
    instagibbs commented at 11:58 pm on October 7, 2018:
    Changed the signature to return size_t directly since there’s no reason these things need to be signed…
    practicalswift commented at 6:04 am on October 8, 2018:
    Then the cast can be removed? :-)
    instagibbs commented at 7:24 am on October 8, 2018:
    done
  23. instagibbs force-pushed on Oct 7, 2018
  24. instagibbs force-pushed on Oct 8, 2018
  25. in test/functional/rpc_psbt.py:210 in 198830e06b outdated 
    167@@ -168,6 +168,9 @@ def run_test(self):
168             assert tx_in["sequence"] > MAX_BIP125_RBF_SEQUENCE
169         assert_equal(decoded_psbt["tx"]["locktime"], 0)
170 
171+        # Make sure change address wallet does not have P2SH innerscript access to results in success
    meshcollider commented at 8:35 am on October 8, 2018:
    nit: typo in comment
    jnewbery commented at 10:56 pm on November 28, 2018:
    not fixed. Suggest something like “Make sure that the wallet can successfully create a funded psbt when it isn’t able to sign for the change output” or similar.
  27. achow101 commented at 0:49 am on October 9, 2018: member
    utACK 198830e06bf4560cfb962e29c73c5b1357d90b37
  28. Empact commented at 2:36 am on October 9, 2018: member
    How about a higher-level test exhibiting the original failure/assert condition? Absent that we don’t have regression coverage or direct failure exhibition. Never mind! The test/functional/rpc_psbt.py change covers this.
  29. in src/wallet/wallet.h:1209 in 198830e06b outdated 
    1199@@ -1200,4 +1200,7 @@ class WalletRescanReserver
1200 // be IsAllFromMe).
1201 int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false);
1202 int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig = false);
1203+
1204+// Calculate the size of a single input of type P2SH-P2WPKH
1205+size_t CalculateNestedKeyhashInputSize(bool use_max_sig = false);
    Empact commented at 2:41 am on October 9, 2018:
    nit: If this is exposed here for testing, declaring it extern in the test file instead will reduce the internal surface area
    Empact commented at 2:37 am on October 20, 2018:
    As of now, it can be removed as the method is now in the wallet_tests file.
  30. instagibbs commented at 2:45 am on October 9, 2018: member
    The functional test could be better explained: We need it to try BnB coin selection with a change output that is a script that it doesn’t have knowledge of.
  31. meshcollider commented at 7:45 am on October 13, 2018: contributor
    Does this fix #14401 ?
  32. instagibbs commented at 12:17 pm on October 15, 2018: member
    @MeshCollider yes I believe so
  33. in src/wallet/wallet.cpp:1517 in 198830e06b outdated 
    1513@@ -1514,13 +1514,46 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
1514     return GetVirtualTransactionSize(txNew);
1515 }
1516 
1517+size_t CalculateNestedKeyhashInputSize(bool use_max_sig)
    sipa commented at 7:52 am on October 17, 2018:

    Is this function just returning one of two integers that could be precomputed? If so, this seems a bit excessive.

    You could leave the function as a comment to show it was computed.

    instagibbs commented at 5:59 pm on October 17, 2018:

    Longer-term I was hoping to have utility functions for various sorts of standard input types.

    You could leave the function as a comment to show it was computed.

    Sorry I’m not quite parsing what you mean here.

    Empact commented at 2:58 am on October 18, 2018:
    A related suggestion: you could directly return the expected literal in this method, then move this implementation code to a test which asserts that the production code returns the same value produced by the current implementation here. Would be good then to reference the test here in a comment to explain the literal result. That way the association between complete implementation and literal optimization is self-enforcing going forward.
    instagibbs commented at 8:17 pm on October 18, 2018:
    That’s actually quite reasonable, thank you.
  34. instagibbs force-pushed on Oct 18, 2018
  35. instagibbs commented at 8:50 pm on October 18, 2018: member
    updated with @Empact suggestion to move the explicit calculation to unit tests instead.
  36. in src/wallet/wallet.h:69 in f038b637c7 outdated 
    63@@ -64,6 +64,10 @@ static const bool DEFAULT_WALLET_RBF = false;
64 static const bool DEFAULT_WALLETBROADCAST = true;
65 static const bool DEFAULT_DISABLE_WALLET = false;
66 
67+//! Pre-calculated constants for input size estimation in *virtual size*
68+static constexpr size_t DUMMY_NESTED_P2WPKH_INPUT_SIZE = 91;
69+static constexpr size_t DUMMY_NESTED_P2WPKH_INPUT_SIZE_MAX = 91;
    sipa commented at 2:10 am on October 20, 2018:
    This constant isn’t used anywhere, is that intentional?
    Empact commented at 2:44 am on October 20, 2018:
    As of now ‘_MAX’ is used in wallet_tests… could maybe do without it (given equality), with further commenting.
  37. in src/wallet/test/wallet_tests.cpp:427 in f038b637c7 outdated 
    422+}
423+
424+BOOST_FIXTURE_TEST_CASE(dummy_input_size_test, TestChain100Setup)
425+{
426+    BOOST_CHECK_EQUAL(CalculateNestedKeyhashInputSize(false), DUMMY_NESTED_P2WPKH_INPUT_SIZE);
427+    // Same virtual size due to rounding
    Empact commented at 2:32 am on October 20, 2018:
    Could you elaborate on this rounding in the comment? I found the equality surprising, but then I’m not well-versed in the protocol.
    Empact commented at 2:33 am on October 20, 2018:
    The declaration could be a better place to provide the explanation.
  38. in src/wallet/test/wallet_tests.cpp:389 in f038b637c7 outdated 
    384@@ -384,4 +385,47 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup)
385     BOOST_CHECK(!wallet->GetKeyFromPool(pubkey, false));
386 }
387 
388+// Explicit calculation which is used to test the wallet constant
389+size_t CalculateNestedKeyhashInputSize(bool use_max_sig)
    Empact commented at 2:37 am on October 20, 2018:
    nit: could be static
  39. Empact changes_requested
  40. instagibbs force-pushed on Oct 22, 2018
  41. instagibbs commented at 1:45 pm on October 22, 2018: member
    Tried pushing update touching all comments
  42. DrahtBot added the label Needs rebase on Nov 10, 2018
  43. Remove stale comment in CalculateMaximumSignedInputSize b06483c96a
  44. CreateTransaction: Assume minimum p2sh-p2wpkh spend size for unknown change 0fb2e69815
  45. instagibbs force-pushed on Nov 12, 2018
  46. instagibbs commented at 6:12 pm on November 12, 2018: member
    rebased, just a test conflict
  47. DrahtBot removed the label Needs rebase on Nov 12, 2018
  48. in src/wallet/wallet.h:88 in 0fb2e69815 
    84@@ -85,6 +85,9 @@ static const bool DEFAULT_WALLET_RBF = false;
85 static const bool DEFAULT_WALLETBROADCAST = true;
86 static const bool DEFAULT_DISABLE_WALLET = false;
87 
88+//! Pre-calculated constants for input size estimation in *virtual size*
    Empact commented at 8:34 pm on November 12, 2018:
    nit: how about Estimated nested input size in *virtual size*, see CalculateNestedKeyhashInputSize
  49. Empact commented at 8:35 pm on November 12, 2018: member
    utACK 0fb2e69
  50. sipa commented at 11:07 pm on November 14, 2018: member
    utACK 0fb2e69815bd5146e601a7fd3585f21a1fdd6f5d
  51. MarcoFalke added this to the milestone 0.17.1 on Nov 28, 2018
  52. MarcoFalke added the label Needs backport on Nov 28, 2018
  53. TheBlueMatt commented at 8:27 pm on November 28, 2018: member
    utACK 0fb2e69815bd5146e601a7fd3585f21a1fdd6f5d
  54. ryanofsky approved
  55. ryanofsky commented at 9:06 pm on November 28, 2018: member
    Slightly tested ACK 0fb2e69815bd5146e601a7fd3585f21a1fdd6f5d. Just confirmed python test fails before and succeeds after the fix.
  56. jnewbery commented at 10:57 pm on November 28, 2018: member

    utACK 0fb2e69815bd5146e601a7fd3585f21a1fdd6f5d.

    Minor comment nit inline, but I suggest you don’t change it in order not to invalidate previous ACKs.

  57. gmaxwell commented at 3:29 am on November 29, 2018: contributor
    utACK
  58. laanwj added this to the "Blockers" column in a project
  59. MarcoFalke merged this on Nov 30, 2018
  60. MarcoFalke closed this on Nov 30, 2018
  61. MarcoFalke referenced this in commit 13a7454fbd on Nov 30, 2018
  62. MarcoFalke commented at 3:53 pm on November 30, 2018: member
    @instagibbs Would you mind creating a backport for 0.17 of this?
  63. instagibbs commented at 4:07 pm on November 30, 2018: member
    @MarcoFalke done
  64. MarcoFalke referenced this in commit 924cf794e1 on Nov 30, 2018
  65. fanquake commented at 10:27 pm on November 30, 2018: member
    Backported in #14851.
  66. fanquake removed the label Needs backport on Nov 30, 2018
  67. jnewbery removed this from the "Blockers" column in a project
  68. luke-jr referenced this in commit 5bbb4f57d5 on Dec 24, 2018
  69. deadalnix referenced this in commit 9e07ed780c on Nov 23, 2020
  70. MarcoFalke locked this on Sep 8, 2021


instagibbs practicalswift gmaxwell DrahtBot achow101 meshcollider jnewbery Empact sipa TheBlueMatt ryanofsky MarcoFalke fanquake

Labels
Wallet

Milestone
0.17.1

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-18 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me