wallet: unify max signature logic #25481

pull S3RK wants to merge 2 commits into bitcoin:master from S3RK:wallet_unify_max_sig changing 6 files +27 −36
  1. S3RK commented at 8:00 AM on June 27, 2022: contributor

    Currently DummySignTx and DummySignInput use different ways to determine signature size. This PR unifies the way wallet estimates signature size for various inputs. Instead of passing boolean flags from calling code the use_max_sig is now calculated at the place of signature creation using information available in CCoinControl

  2. fanquake added the label Wallet on Jun 27, 2022
  3. in src/bench/coin_selection.cpp:62 in 164414a979 outdated 
      57 | @@ -58,7 +58,8 @@ static void CoinSelection(benchmark::Bench& bench)
  58 |      // Create coins
  59 |      std::vector<COutput> coins;
  60 |      for (const auto& wtx : wtxs) {
  61 | -        coins.emplace_back(COutPoint(wtx->GetHash(), 0), wtx->tx->vout.at(0), /*depth=*/6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
  62 | +        const auto txout = wtx->tx->vout.at(0);
  63 | +        coins.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, false), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
    furszy commented at 1:00 PM on June 27, 2022: 
            coins.emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*use_max_sig=*/false), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0);
    S3RK commented at 3:25 PM on June 27, 2022:

    This is removed in the next commit

  4. in src/wallet/spend.cpp:196 in 164414a979 outdated 
     192 | @@ -198,7 +193,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
 193 |              // Filter by spendable outputs only
 194 |              if (!spendable && only_spendable) continue;
 195 |  
 196 | -            int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly));
 197 | +            int input_bytes = CalculateMaximumSignedInputSize(output, &wallet, false);
    furszy commented at 1:02 PM on June 27, 2022:

    why not use_max_sig=coinControl && coinControl->fAllowWatchOnly ?

    S3RK commented at 3:25 PM on June 27, 2022:

    I'd like to remove the flag

  5. in src/wallet/spend.cpp:289 in 164414a979 outdated 
     283 | @@ -289,8 +284,9 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
 284 |              ) {
 285 |                  CTxDestination address;
 286 |                  if (ExtractDestination(FindNonChangeParentOutput(wallet, *wtx.tx, output.n).scriptPubKey, address)) {
 287 | +                    const auto out = wtx.tx->vout.at(output.n);
 288 |                      result[address].emplace_back(
 289 | -                        COutPoint(wtx.GetHash(), output.n), wtx.tx->vout.at(output.n), depth, GetTxSpendSize(wallet, wtx, output.n), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), CachedTxIsFromMe(wallet, wtx, ISMINE_ALL));
 290 | +                            COutPoint(wtx.GetHash(), output.n), out, depth, CalculateMaximumSignedInputSize(out, &wallet, false), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), CachedTxIsFromMe(wallet, wtx, ISMINE_ALL));
    furszy commented at 1:03 PM on June 27, 2022:

    same as above, let's document the use_max_sig flag.

    S3RK commented at 3:26 PM on June 27, 2022:

    It's removed in the next commit

  6. in src/wallet/spend.cpp:446 in 164414a979 outdated 
     442 | @@ -447,7 +443,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
 443 |              if (ptr_wtx->tx->vout.size() <= outpoint.n) {
 444 |                  return std::nullopt;
 445 |              }
 446 | -            input_bytes = GetTxSpendSize(wallet, *ptr_wtx, outpoint.n, false);
 447 | +            input_bytes = CalculateMaximumSignedInputSize(ptr_wtx->tx->vout[outpoint.n], &wallet, false);
    furszy commented at 1:04 PM on June 27, 2022:

    same as above, let's document the use_max_sig flag.

    S3RK commented at 3:26 PM on June 27, 2022:

    Same as above

  7. in src/wallet/test/coinselector_tests.cpp:89 in 164414a979 outdated 
      85 | @@ -86,7 +86,8 @@ static void add_coin(std::vector<COutput>& coins, CWallet& wallet, const CAmount
  86 |      auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{}));
  87 |      assert(ret.second);
  88 |      CWalletTx& wtx = (*ret.first).second;
  89 | -    coins.emplace_back(COutPoint(wtx.GetHash(), nInput), wtx.tx->vout.at(nInput), nAge, GetTxSpendSize(wallet, wtx, nInput), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate);
  90 | +    const auto txout = wtx.tx->vout.at(nInput);
    furszy commented at 1:05 PM on June 27, 2022: 
        const auto& output = wtx.tx->vout.at(nInput);
    S3RK commented at 7:29 AM on June 28, 2022:

    Done

  8. in src/wallet/test/coinselector_tests.cpp:90 in 164414a979 outdated 
      85 | @@ -86,7 +86,8 @@ static void add_coin(std::vector<COutput>& coins, CWallet& wallet, const CAmount
  86 |      auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{}));
  87 |      assert(ret.second);
  88 |      CWalletTx& wtx = (*ret.first).second;
  89 | -    coins.emplace_back(COutPoint(wtx.GetHash(), nInput), wtx.tx->vout.at(nInput), nAge, GetTxSpendSize(wallet, wtx, nInput), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate);
  90 | +    const auto txout = wtx.tx->vout.at(nInput);
  91 | +    coins.emplace_back(COutPoint(wtx.GetHash(), nInput), txout, nAge, CalculateMaximumSignedInputSize(txout, &wallet, false), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate);
    furszy commented at 1:06 PM on June 27, 2022: 
        coins.emplace_back(COutPoint(wtx.GetHash(), nInput), txout, nAge, CalculateMaximumSignedInputSize(txout, &wallet, /*use_max_sig=*/false), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate);
    S3RK commented at 3:27 PM on June 27, 2022:

    Can't spot the difference

    furszy commented at 4:54 PM on June 27, 2022:

    It it the same as this one #25481 (review)

  9. in src/bench/coin_selection.cpp:59 in ae074640fa outdated 
      55 | @@ -56,10 +56,11 @@ static void CoinSelection(benchmark::Bench& bench)
  56 |      addCoin(3 * COIN, wallet, wtxs);
  57 |  
  58 |      // Create coins
  59 | +    CCoinControl coin_control:
    furszy commented at 1:12 PM on June 27, 2022: 
        wallet::CCoinControl coin_control;
    S3RK commented at 3:27 PM on June 27, 2022:

    Thanks. Totally missed it

    achow101 commented at 6:26 PM on June 27, 2022:

    Or pass a nullptr instead of an empty CCoinControl.

    S3RK commented at 7:29 AM on June 28, 2022:

    Done. Replaced with nullptr

  10. furszy commented at 1:14 PM on June 27, 2022: member

    Aside from the quick code review that made, want to point you in #24699 direction.

    The first two commits there are implementing part of what you did here. Would be nice to get that one reviewed first.

  11. DrahtBot commented at 4:16 PM on June 27, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24699 (wallet: Improve AvailableCoins performance by reducing duplicated operations by achow101)
    • #24584 (wallet: avoid mixing different OutputTypes during coin selection by josibake)

    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.

  12. wallet: replace GetTxSpendSize with CalculateMaximumSignedInputSize a94659c84e
  13. S3RK force-pushed on Jun 28, 2022
  14. wallet: use CCoinControl to estimate signature size d54c5c8b1b
  15. S3RK force-pushed on Jun 28, 2022
  16. S3RK commented at 7:33 AM on June 28, 2022: contributor

    @furszy thanks for the review and the pointer. I don't want to add hints for the flag, because it's removed in the next commit. I made this PR compatible with #24699 and now they nicely compliment each other. 008d1178c8b09aa9190b188f399fc0f5818c7499 Is fully covered by the first commit here and it does even more. c685bf545416abe719d2821e8156ff1d0bb2d749 Is taking a slightly different direction, but it's compatible with the code here It seems either PR can go first.

  17. achow101 commented at 6:50 PM on June 30, 2022: member

    ACK d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9

  18. theStack approved
  19. theStack commented at 1:44 PM on July 6, 2022: contributor

    Code-review ACK d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9

  20. fanquake requested review from murchandamus on Jul 6, 2022
  21. achow101 merged this on Jul 8, 2022
  22. achow101 closed this on Jul 8, 2022
  23. murchandamus commented at 7:35 PM on July 12, 2022: contributor

    Post-merge ACK d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9

  24. bitcoin locked this on Jul 12, 2023
Contributors
S3RKfurszyachow101DrahtBottheStackmurchandamus
Labels
Wallet
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: 2026-04-21 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me