Replace size/weight estimate tuple with struct for named fields #22042

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:named_size_tuple changing 3 files +17 −13
  1. instagibbs commented at 11:27 AM on May 24, 2021: member

    For clarity of return values of size estimation functions.

  2. fanquake added the label Wallet on May 24, 2021
  3. jonatack commented at 11:54 AM on May 24, 2021: contributor

    ACK 79e2a276e4817a25c8c8a757dc6ce8e52c1b37f6

    Good idea; the code is much clearer with the struct. It might be nice to clang-format the touched function signatures.

  4. theStack approved
  5. theStack commented at 3:39 PM on May 24, 2021: contributor

    Code review ACK 79e2a276e4817a25c8c8a757dc6ce8e52c1b37f6

  6. achow101 commented at 7:54 PM on May 24, 2021: member

    ACK 79e2a276e4817a25c8c8a757dc6ce8e52c1b37f6

  7. ryanofsky approved
  8. ryanofsky commented at 7:56 PM on May 24, 2021: contributor

    Code review ACK 79e2a276e4817a25c8c8a757dc6ce8e52c1b37f6. I was going to suggest returning std::optional<TxSize> instead of TxSize to get rid of magic return TxSize{-1, -1}; negative sizes, but this would be a bigger change and -1 seems to be used outside of this anyway.

  9. practicalswift commented at 7:56 PM on May 24, 2021: contributor

    Concept ACK

    Non-blocking suggestion: Could in-class initialize vsize and weight to reasonable defaults?

  10. DrahtBot commented at 10:05 PM on May 24, 2021: 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:

    • #22019 (wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101)
    • #22008 (wallet: Cleanup and refactor CreateTransactionInternal by achow101)
    • #21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
    • #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
    • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

    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.

  11. DrahtBot cross-referenced this on May 24, 2021 from issue wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101
  12. DrahtBot cross-referenced this on May 24, 2021 from issue wallet: Decide which coin selection solution to use based on waste metric by achow101
  13. DrahtBot cross-referenced this on May 24, 2021 from issue wallet: Cleanup and refactor CreateTransactionInternal by achow101
  14. DrahtBot cross-referenced this on May 25, 2021 from issue MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky
  15. DrahtBot cross-referenced this on May 25, 2021 from issue refactor: Make CWalletTx sync state type-safe by ryanofsky
  16. DrahtBot cross-referenced this on May 25, 2021 from issue Add Single Random Draw as an additional coin selection algorithm by achow101
  17. DrahtBot cross-referenced this on May 25, 2021 from issue Use effective values throughout coin selection by achow101
  18. DrahtBot cross-referenced this on May 25, 2021 from issue Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101
  19. in src/wallet/wallet.h:1393 in 79e2a276e4 outdated 
    1389 | @@ -1390,8 +1390,13 @@ class WalletRescanReserver
1390 |  * Use DummySignatureCreator, which inserts 71 byte signatures everywhere.
1391 |  * NOTE: this requires that all inputs must be in mapWallet (eg the tx should
1392 |  * be IsAllFromMe). */
1393 | -std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet);
1394 | -std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig = false);
1395 | +struct TxSize {
    MarcoFalke commented at 8:19 AM on May 25, 2021:

    The doxygen comment is attached wrong

    instagibbs commented at 9:16 AM on May 25, 2021:

    fixed(I think)

  20. in src/wallet/wallet.h:1399 in 79e2a276e4 outdated 
    1396 | +    int64_t vsize;
1397 | +    int64_t weight;
1398 | +};
1399 | +
1400 | +TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet);
1401 | +TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig = false);
    MarcoFalke commented at 8:19 AM on May 25, 2021:

    could clang-format-diff?

  21. in src/wallet/feebumper.cpp:193 in 79e2a276e4 outdated 
     189 | @@ -190,7 +190,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
 190 |      if (coin_control.m_feerate) {
 191 |          // The user provided a feeRate argument.
 192 |          // We calculate this here to avoid compiler warning on the cs_wallet lock
 193 | -        const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, &wallet).first;
 194 | +        const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, &wallet).vsize;
    MarcoFalke commented at 8:20 AM on May 25, 2021: 
            const int64_t maxTxSize{CalculateMaximumSignedTxSize(*wtx.tx, &wallet).vsize};
  22. MarcoFalke commented at 8:20 AM on May 25, 2021: member

    left some nits and pointed out that doxygen will be messed up

  23. in src/wallet/wallet.h:1395 in 79e2a276e4 outdated 
    1389 | @@ -1390,8 +1390,13 @@ class WalletRescanReserver
1390 |  * Use DummySignatureCreator, which inserts 71 byte signatures everywhere.
1391 |  * NOTE: this requires that all inputs must be in mapWallet (eg the tx should
1392 |  * be IsAllFromMe). */
1393 | -std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet);
1394 | -std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig = false);
1395 | +struct TxSize {
1396 | +    int64_t vsize;
1397 | +    int64_t weight;
    MarcoFalke commented at 8:24 AM on May 25, 2021: 
        const int64_t vsize{-1};
    const int64_t weight{-1};
    instagibbs commented at 9:32 AM on May 25, 2021:

    const makes it unhappy due to default copy constructor being ill-formed, removed

  24. instagibbs force-pushed on May 25, 2021
  25. in src/wallet/wallet.h:1399 in 858dad7dbc outdated 
    1396 |  * NOTE: this requires that all inputs must be in mapWallet (eg the tx should
1397 |  * be IsAllFromMe). */
1398 | -std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet);
1399 | -std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig = false);
1400 | +TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet);
1401 | +TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig = false);
    MarcoFalke commented at 9:18 AM on May 25, 2021: 
    TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* wallet, const std::vector<CTxOut>& txouts, bool use_max_sig = false);

    ;)

    instagibbs commented at 9:19 AM on May 25, 2021:

    hmmm didn't see any output running locally. Probably using it wrong :)

  26. MarcoFalke approved
  27. MarcoFalke commented at 9:18 AM on May 25, 2021: member

    ACK

  28. instagibbs force-pushed on May 25, 2021
  29. MarcoFalke approved
  30. MarcoFalke commented at 9:53 AM on May 25, 2021: member

    ACK

  31. in src/wallet/wallet.cpp:1630 in 299e85c818 outdated 
    1627 | @@ -1628,14 +1628,14 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
1628 |  }
1629 |  
1630 |  // Returns pair of vsize and weight
1631 | -std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)
1632 | +TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)
    jonatack commented at 10:01 AM on May 25, 2021:

    clang-format

  32. in src/wallet/wallet.cpp:1646 in 299e85c818 outdated 
    1643 | @@ -1644,16 +1644,16 @@ std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx,
1644 |  }
1645 |  
1646 |  // txouts needs to be in the order of tx.vin
1647 | -std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)
1648 | +TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)
    jonatack commented at 10:02 AM on May 25, 2021:

    same

  33. jonatack commented at 10:04 AM on May 25, 2021: contributor

    utACK 299e85c8188510528f1e49a0b8d2a99f45916898

    good changes in git diff 79e2a27 299e85c, modulo clang format nits

  34. in src/wallet/wallet.cpp:1630 in 299e85c818 outdated 
    1627 | @@ -1628,14 +1628,14 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
1628 |  }
1629 |  
1630 |  // Returns pair of vsize and weight
    MarcoFalke commented at 10:05 AM on May 25, 2021:

    Can remove this comment?

    instagibbs commented at 11:19 PM on May 25, 2021:

    removed

  35. DrahtBot added the label Needs rebase on May 25, 2021
  36. instagibbs force-pushed on May 25, 2021
  37. Replace size/weight estimate tuple with struct for named fields 881a3e2e17
  38. instagibbs force-pushed on May 25, 2021
  39. instagibbs commented at 11:39 PM on May 25, 2021: member

    rebased on master

  40. DrahtBot removed the label Needs rebase on May 26, 2021
  41. MarcoFalke commented at 5:27 AM on May 26, 2021: member

    review ACK 881a3e2e17c5a6fdffb16a47a2eaff9029f261e5

  42. practicalswift commented at 5:37 AM on May 26, 2021: contributor

    cr ACK 881a3e2e17c5a6fdffb16a47a2eaff9029f261e5

    Much more readable!

    Possible candidates for similar treatment (out of scope for this PR of course):

    $ git grep -E '^( |std::optional<)*std::pair.*\(' -- "*.h"
src/consensus/tx_verify.h:std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>& prevHeights, const CBlockIndex& block);
src/httpserver.h:    std::pair<bool, std::string> GetHeader(const std::string& hdr) const;
src/indirectmap.h:    std::pair<iterator, bool> insert(const value_type& value) { return m.insert(value); }
src/pubkey.h:    std::optional<std::pair<XOnlyPubKey, bool>> CreateTapTweak(const uint256* merkle_root) const;
src/rpc/util.h:std::pair<int64_t, int64_t> ParseDescriptorRange(const UniValue& value);
src/txorphanage.h:    std::pair<CTransactionRef, NodeId> GetTx(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
src/util/readwritefile.h:std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxsize=std::numeric_limits<size_t>::max());
src/util/system.h:    std::pair<int, char**> get();
  43. fanquake merged this on May 26, 2021
  44. fanquake closed this on May 26, 2021
  45. promag commented at 8:25 AM on May 26, 2021: member

    Code review ACK 881a3e2e17c5a6fdffb16a47a2eaff9029f261e5.

  46. sidhujag referenced this in commit ddb2d59885 on May 27, 2021
  47. gwillen referenced this in commit 81eb03b40c on Jun 1, 2022
  48. bitcoin locked this on Aug 16, 2022
Contributors
instagibbsjonatacktheStackachow101ryanofskypracticalswiftDrahtBotMarcoFalkepromag
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-05-19 06:53 UTC