Replace size/weight estimate tuple with struct for named fields

instagibbs commented at 11:27 am on May 24, 2021: member

For clarity of return values of size estimation functions.

fanquake added the label Wallet on May 24, 2021

jonatack commented at 11:54 am on May 24, 2021: member

ACK 79e2a276e4817a25c8c8a757dc6ce8e52c1b37f6

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

theStack approved

theStack commented at 3:39 pm on May 24, 2021: member

Code review ACK 79e2a276e4817a25c8c8a757dc6ce8e52c1b37f6

achow101 commented at 7:54 pm on May 24, 2021: member

ACK 79e2a276e4817a25c8c8a757dc6ce8e52c1b37f6

ryanofsky approved

ryanofsky commented at 7:56 pm on May 24, 2021: member

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.

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?

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

#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.

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)

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?

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:

0        const int64_t maxTxSize{CalculateMaximumSignedTxSize(*wtx.tx, &wallet).vsize};

MarcoFalke commented at 8:20 am on May 25, 2021: member

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

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:

0    const int64_t vsize{-1};
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

instagibbs force-pushed on May 25, 2021

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:

0TxSize 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 :)

MarcoFalke approved

MarcoFalke commented at 9:18 am on May 25, 2021: member

ACK

instagibbs force-pushed on May 25, 2021

MarcoFalke approved

MarcoFalke commented at 9:53 am on May 25, 2021: member

ACK

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

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

jonatack commented at 10:04 am on May 25, 2021: member

utACK 299e85c8188510528f1e49a0b8d2a99f45916898

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

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

DrahtBot added the label Needs rebase on May 25, 2021

instagibbs force-pushed on May 25, 2021

Replace size/weight estimate tuple with struct for named fields 881a3e2e17

instagibbs force-pushed on May 25, 2021

instagibbs commented at 11:39 pm on May 25, 2021: member

rebased on master

DrahtBot removed the label Needs rebase on May 26, 2021

MarcoFalke commented at 5:27 am on May 26, 2021: member

review ACK 881a3e2e17c5a6fdffb16a47a2eaff9029f261e5

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):

0$ git grep -E '^( |std::optional<)*std::pair.*\(' -- "*.h"
1src/consensus/tx_verify.h:std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>& prevHeights, const CBlockIndex& block);
2src/httpserver.h:    std::pair<bool, std::string> GetHeader(const std::string& hdr) const;
3src/indirectmap.h:    std::pair<iterator, bool> insert(const value_type& value) { return m.insert(value); }
4src/pubkey.h:    std::optional<std::pair<XOnlyPubKey, bool>> CreateTapTweak(const uint256* merkle_root) const;
5src/rpc/util.h:std::pair<int64_t, int64_t> ParseDescriptorRange(const UniValue& value);
6src/txorphanage.h:    std::pair<CTransactionRef, NodeId> GetTx(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
7src/util/readwritefile.h:std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxsize=std::numeric_limits<size_t>::max());
8src/util/system.h:    std::pair<int, char**> get();

fanquake merged this on May 26, 2021

fanquake closed this on May 26, 2021

promag commented at 8:25 am on May 26, 2021: member

Code review ACK 881a3e2e17c5a6fdffb16a47a2eaff9029f261e5.

sidhujag referenced this in commit ddb2d59885 on May 27, 2021

gwillen referenced this in commit 81eb03b40c on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

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

Conflicts