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

instagibbs · 2021-05-24T11:27:46Z

For clarity of return values of size estimation functions.

jonatack

ACK 79e2a27

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

theStack

Code review ACK 79e2a27

achow101 · 2021-05-24T19:54:12Z

ACK 79e2a27

ryanofsky

Code review ACK 79e2a27. 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 · 2021-05-24T19:56:54Z

Concept ACK

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

DrahtBot · 2021-05-24T22:05:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

maflcko

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

src/wallet/wallet.h

src/wallet/feebumper.cpp

src/wallet/wallet.h

maflcko

ACK

src/wallet/wallet.h

maflcko

ACK

jonatack · 2021-05-25T10:01:59Z

src/wallet/wallet.cpp

@@ -1628,14 +1628,14 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
 }

 // Returns pair of vsize and weight
-std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)
+TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)


clang-format

jonatack · 2021-05-25T10:02:13Z

src/wallet/wallet.cpp

@@ -1644,16 +1644,16 @@ std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx,
 }

 // txouts needs to be in the order of tx.vin
-std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)
+TxSize CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)


jonatack · 2021-05-25T10:04:22Z

utACK 299e85c

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

src/wallet/wallet.cpp

instagibbs · 2021-05-25T23:39:22Z

rebased on master

maflcko · 2021-05-26T05:27:19Z

review ACK 881a3e2

practicalswift · 2021-05-26T05:37:17Z

cr ACK 881a3e2

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();

promag

Code review ACK 881a3e2.

…or name…

fanquake added the Wallet label May 24, 2021

jonatack reviewed May 24, 2021

View reviewed changes

theStack approved these changes May 24, 2021

View reviewed changes

ryanofsky approved these changes May 24, 2021

View reviewed changes

maflcko reviewed May 25, 2021

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

src/wallet/feebumper.cpp Outdated Show resolved Hide resolved

maflcko reviewed May 25, 2021

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

instagibbs force-pushed the named_size_tuple branch from 88fea85 to 858dad7 Compare May 25, 2021 09:16

maflcko approved these changes May 25, 2021

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

instagibbs force-pushed the named_size_tuple branch from 525fbed to 299e85c Compare May 25, 2021 09:32

maflcko approved these changes May 25, 2021

View reviewed changes

jonatack reviewed May 25, 2021

View reviewed changes

maflcko reviewed May 25, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label May 25, 2021

instagibbs force-pushed the named_size_tuple branch from 299e85c to 72906cb Compare May 25, 2021 23:19

Replace size/weight estimate tuple with struct for named fields

881a3e2

instagibbs force-pushed the named_size_tuple branch from 72906cb to 881a3e2 Compare May 25, 2021 23:33

DrahtBot removed the Needs rebase label May 26, 2021

fanquake merged commit 7aa41fc into bitcoin:master May 26, 2021

promag reviewed May 26, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 27, 2021

Merge bitcoin#22042: Replace size/weight estimate tuple with struct f…

ddb2d59

…or name…

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 7aa41fc into merged_master (Bitcoin PR bitcoin/bitcoin#22042)

81eb03b

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

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

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

Uh oh!

Conversation

instagibbs commented May 24, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 24, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented May 24, 2021

Uh oh!

DrahtBot commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack May 25, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack May 25, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack commented May 25, 2021

Uh oh!

Uh oh!

instagibbs commented May 25, 2021

Uh oh!

maflcko commented May 26, 2021

Uh oh!

practicalswift commented May 26, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented May 24, 2021 •

edited

Loading