wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction

achow101 commented at 6:58 pm on June 3, 2022: member

Currently FundTransaction handles transaction locktime and preset input data by extracting the selected inputs and change output from CreateTransaction’s results. This means that CreateTransaction is actually unaware of any user desired locktime or sequence numbers. This can have an effect on whether and how anti-fee-sniping works.

This PR makes CreateTransaction aware of the locktime and preset input data by providing them to CCoinControl. CreateTransasction will then set the sequences, scriptSigs, scriptWItnesses, and locktime as appropriate if they are specified. This allows FundTransaction to actually use CreateTransaction’s result directly instead of having to extract the parts of it that it wants.

Additionally FundTransaction will return a CreateTransactionResult as CreateTransaction does instead of having several output parameters. Lastly, instead of using -1 as a magic number for the change output position, the change position is changed to be an optional with no value set indicating no desired change output position (when provided as an input parameter) or no change output present (in the result).

DrahtBot added the label RPC/REST/ZMQ on Jun 3, 2022

DrahtBot added the label Wallet on Jun 3, 2022

DrahtBot commented at 0:47 am on June 4, 2022: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	josibake, S3RK
Concept ACK	darosior
Stale ACK	furszy, ishaanam, theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/733 (Deniability - a tool to automatically improve coin ownership privacy by denavila)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27792 (wallet: Deniability API (Unilateral Transaction Meta-Privacy) by denavila)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#26902 (wallet: do not backdate locktime if it may lead to fingerprinting by rodentrabies)
#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
#24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by maflcko)
#21283 (Implement BIP 370 PSBTv2 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/coincontrol.h:155 in 99bd2a643d outdated

121             return false;
122         }
123-        txout = ext_it->second;
124+        if (!it->second.HasTxOut()) {
125+            return false;
126+        }

furszy commented at 6:00 pm on June 8, 2022:

maybe inline?

0if (it == m_selected.end() || !it->second.HasTxOut())) {
1    return false;
2}

achow101 commented at 10:20 pm on January 23, 2023:

Done

DrahtBot added the label Needs rebase on Jun 16, 2022

achow101 force-pushed on Jun 17, 2022

DrahtBot removed the label Needs rebase on Jun 17, 2022

in src/wallet/rpc/spend.cpp:490 in 664f743047 outdated

486@@ -488,13 +487,13 @@ static std::vector<RPCArg> FundTxDoc(bool solving_data = true)
487     return args;
488 }
489 
490-void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl, bool override_min_fee)
491+std::optional<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const UniValue& options, CCoinControl& coinControl, bool override_min_fee)

luke-jr commented at 3:43 pm on June 18, 2022:

Since this can never return a nullopt (errors throw), maybe just return CreatedTransactionResult?

achow101 commented at 5:37 pm on June 20, 2022:

Done

luke-jr commented at 3:48 pm on June 18, 2022: member

Lastly, instead of using -1 as a magic number for the change output position, the change position is changed to be an optional with no value set indicating no desired change output position (when provided as an input parameter) or no change output present (in the result).

I’m not sure this is an improvement. It just adds complexity, for no benefit in this particular instance?

in src/wallet/coincontrol.h:170 in 664f743047 outdated

172+        txout = it->second.GetTxOut();
173         return true;
174     }
175 
176-    void Select(const COutPoint& output)
177+    void Select(const COutPoint& output, std::optional<uint32_t> sequence = std::nullopt, std::optional<CScript> script_sig = std::nullopt, std::optional<CScriptWitness> script_witness = std::nullopt)

luke-jr commented at 3:55 pm on June 18, 2022:

I think it would be better to return the PreselectedInput and have the caller use SetSequence etc directly.

achow101 commented at 5:37 pm on June 20, 2022:

Done

in src/wallet/spend.cpp:1134 in 664f743047 outdated

851     for (const auto& coin : selected_coins) {
852-        txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence));
853+        uint32_t sequence = default_sequence;
854+        if (coin_control.HasSequence(coin.outpoint)) {
855+            sequence = coin_control.GetSequence(coin.outpoint);
856+            // If an input as a preset sequence, we can't do anti-fee-sniping

luke-jr commented at 4:00 pm on June 18, 2022:

Why not? As long as one input has a non-final sequence, it should still work.

(DiscourageFeeSniping would need its sanity checks adjusted to tolerate it, though)

achow101 commented at 3:13 pm on June 20, 2022:

This is to retain the current behavior. The next step in a followup is to allow anti-fee-sniping and modify DiscourageFeeSniping to consider the preset sequences.

luke-jr commented at 8:48 pm on June 20, 2022:

Where is this behaviour in the current logic? I thought presently it would do the anti-sniping, and then possibly (at worst) have that partially “broken” by subsequent changes?

achow101 commented at 9:29 pm on June 20, 2022:

It’s more of a side effect of FundTransaction rather than explicit behavior. Currently, having preset sequences inherently means a preset locktime which would disable anti-fee-sniping. This is just due to how FundTransaction works, and FundTransaction is the only way that preset sequences can even be provided.

Until we update DiscourageFeeSniping to handle preset sequences and locktime, I would prefer that we have this check explicitly just to avoid any problems in the future with how and when preset sequences and locktimes can be set.

darosior commented at 11:00 am on March 23, 2023:

Making sure i got this one right. use_anti_fee_sniping may only be false if this is called from FundTransaction. And FundTransaction diregards the value of the nLockTime that may be set here anyways. (Well not anymore in the following commits, but at this stage in the commit series it does.)

achow101 commented at 3:36 pm on March 23, 2023:

Yes, FundTransaction currently throws away nLockTime thereby disabling anti-fee-sniping, and this preserves that behavior.

achow101 force-pushed on Jun 20, 2022

achow101 commented at 5:38 pm on June 20, 2022: member

Lastly, instead of using -1 as a magic number for the change output position, the change position is changed to be an optional with no value set indicating no desired change output position (when provided as an input parameter) or no change output present (in the result).

I’m not sure this is an improvement. It just adds complexity, for no benefit in this particular instance?

IMO it makes it easier to understand.

0SUMMARY: MemorySanitizer: use-of-uninitialized-value src/wallet/spend.cpp:1025:5 in wallet::CreateTransaction(wallet::CWallet&, std::__1::vector<wallet::CRecipient, std::__1::allocator<wallet::CRecipient> > const&, std::__1::optional<unsigned int>, bilingual_str&, wallet::CCoinControl const&, FeeCalculation&, bool)

I think I’ve fixed this.

DrahtBot added the label Needs rebase on Jun 20, 2022

achow101 force-pushed on Jun 20, 2022

DrahtBot removed the label Needs rebase on Jun 21, 2022

achow101 force-pushed on Jun 21, 2022

DrahtBot added the label Needs rebase on Jul 12, 2022

achow101 force-pushed on Jul 12, 2022

DrahtBot removed the label Needs rebase on Jul 12, 2022

DrahtBot added the label Needs rebase on Aug 5, 2022

achow101 force-pushed on Aug 5, 2022

DrahtBot removed the label Needs rebase on Aug 5, 2022

DrahtBot added the label Needs rebase on Aug 19, 2022

achow101 force-pushed on Aug 19, 2022

DrahtBot removed the label Needs rebase on Aug 19, 2022

DrahtBot added the label Needs rebase on Aug 22, 2022

achow101 force-pushed on Aug 22, 2022

DrahtBot removed the label Needs rebase on Aug 22, 2022

achow101 force-pushed on Aug 24, 2022

achow101 force-pushed on Oct 21, 2022

in src/wallet/coincontrol.h:206 in bd979e2b14 outdated

183+    }
184+
185+    uint32_t GetSequence(const COutPoint& outpoint) const
186+    {
187+        return m_selected.at(outpoint).GetSequence();
188+    }

furszy commented at 6:57 pm on November 17, 2022:

In 9aab806b:

sounds like this could be unified and return an optional<uint32_t> so double lookups aren’t needed (e.g. force callers to always call HasSequence first then GetSequence).

achow101 commented at 10:20 pm on January 23, 2023:

Done, also for input weight and position.

DrahtBot added the label Needs rebase on Dec 6, 2022

achow101 force-pushed on Dec 6, 2022

DrahtBot removed the label Needs rebase on Dec 6, 2022

DrahtBot added the label Needs rebase on Dec 10, 2022

achow101 force-pushed on Dec 12, 2022

DrahtBot removed the label Needs rebase on Dec 12, 2022

DrahtBot added the label Needs rebase on Dec 13, 2022

achow101 force-pushed on Dec 13, 2022

DrahtBot removed the label Needs rebase on Dec 13, 2022

DrahtBot added the label Needs rebase on Jan 3, 2023

achow101 force-pushed on Jan 3, 2023

DrahtBot removed the label Needs rebase on Jan 3, 2023

in src/wallet/spend.cpp:1400 in 65550f8ac4 outdated

1151+                // The input was not in the wallet, but is in the UTXO set, so select as external
1152+                preset_txin.SetTxOut(coins[outPoint].out);
1153+            } else {
1154+                error = _("Unable to find UTXO for external input");
1155+                return false;
1156+            }

furszy commented at 2:06 pm on January 22, 2023:

In 65550f8a:

nit: could replace the inner if/else for

0if (!wallet.IsMine(outPoint)) {
1    if (coins[outPoint].out.IsNull()) {
2        error = _("Unable to find UTXO for external input");
3        return false;
4    }
5
6    // The input was not in the wallet, but is in the UTXO set, so select as external
7    preset_txin.SetTxOut(coins[outPoint].out);
8}

achow101 commented at 10:20 pm on January 23, 2023:

Done as suggested

in src/wallet/spend.cpp:1187 in 187f2168a7 outdated

968+            auto [ss, sw] = coin_control.GetScripts(coin.outpoint);
969+            script_sig = ss;
970+            script_witness = sw;
971+        }
972+        txNew.vin.push_back(CTxIn(coin.outpoint, script_sig, sequence));
973+        txNew.vin.back().scriptWitness = script_witness;

furszy commented at 2:25 pm on January 22, 2023:

what about using this information inside FetchSelectedInputs, in the input_bytes size calculation too.

achow101 commented at 10:22 pm on January 23, 2023:

Right now, I don’t think that’s a good idea. Since we are always say that the preset inputs have scriptSigs and witnesses when they come from FundTransaction, any input weight calculation using them may be incorrect because those inputs are not necessarily complete or valid, so the final weight may actually differ.

in src/wallet/spend.cpp:1378 in 6fb622183a outdated

1214-    }
1215-
1216-    // Copy output sizes from new transaction; they may have had the fee
1217-    // subtracted from them.
1218-    for (unsigned int idx = 0; idx < tx.vout.size(); idx++) {
1219-        tx.vout[idx].nValue = tx_new->vout[idx].nValue;

furszy commented at 2:52 pm on January 22, 2023:

In 6fb62218:

This removal doesn’t seems to be related to the changes scope (the goal of the PR is keep inputs data). Would be good to add some context for it.

In a first glance, seems that keeping the original outputs value might be useful when you want to discard the fee calculation. Basically enable SFFO on all the outputs to not create a change output, then overwrite the returned outputs amounts with the original ones (maybe you are calling fundrawtransaction on different wallets and want to only calculate the fee on the last call).

achow101 commented at 9:51 pm on January 23, 2023:

I think you misunderstand what the original code was doing. It isn’t preserving the original values, it’s replacing the original values with the values that CreateTransaction produces. Originally FundTransaction is calling CreateTranasction and copying specific parts of the result into the original transaction and returning that modified original. The change I’ve made here is to just return the result of CreateTransaction directly, which matches the previous behavior because the earlier commits are preserving all of the components of the transaction that FundTranasction was preserving.

furszy commented at 2:17 pm on January 26, 2023:

oops, yeah.. nvm then.

furszy commented at 3:08 pm on January 22, 2023: member

Code reviewed, left few nits and topics.

How is enforced that SignTransaction doesn’t overwrite the provided inputs’ scriptsig and witness data?. It’s called after setting the data and, as far as can see, it isn’t skipping not empty inputs. Would be good to add test coverage for it.

achow101 commented at 9:23 pm on January 23, 2023: member

How is enforced that SignTransaction doesn’t overwrite the provided inputs’ scriptsig and witness data?. It’s called after setting the data and, as far as can see, it isn’t skipping not empty inputs.

It will ignore inputs that have a valid scriptSig and witness. SignTransaction calls DataFromTransaction which will perform a VerifyScript of the existing data. If this is successful, it will mark that input as complete (sets SignatureData.complete) which ProduceSignature checks before it does anything else.

achow101 force-pushed on Jan 23, 2023

furszy approved

furszy commented at 3:11 pm on January 26, 2023: member

Code review ACK 91e6fad8

DrahtBot added the label Needs rebase on Mar 7, 2023

achow101 force-pushed on Mar 15, 2023

DrahtBot removed the label Needs rebase on Mar 15, 2023

furszy approved

furszy commented at 10:22 pm on March 17, 2023: member

diff re-ACK 35fe41f

darosior commented at 11:33 am on March 23, 2023: member

Concept and approach ACK. The code also looks good to me but i’m not too familiar with it so i may be missing some subtleties.

Since we now may have timelocked coins, another motivation for making CreateTransaction aware of a user chosen nLockTime would be to only select coins that are actually spendable with this nLockTime value.

glozow requested review from theStack on Apr 25, 2023

in src/wallet/interfaces.cpp:261 in c158ad118a outdated

257@@ -258,12 +258,12 @@ class WalletImpl : public Wallet
258         CAmount& fee) override
259     {
260         LOCK(m_wallet->cs_wallet);
261-        auto res = CreateTransaction(*m_wallet, recipients, change_pos,
262+        auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::optional<unsigned int>((unsigned int)change_pos),

furszy commented at 7:07 pm on April 30, 2023:

tiny nit: just because I’m using this in another PR:

0change_pos == -1 ? std::nullopt : std::make_optional(change_pos)

in src/wallet/coincontrol.h:76 in bf275f358a outdated

71+        }
72+        CScriptWitness script_witness;
73+        if (m_script_witness.has_value()) {
74+            script_witness = m_script_witness.value();
75+        }
76+        return {script_sig, script_witness};

theStack commented at 4:42 pm on May 3, 2023:

(in commit bf275f358a5e816cf9a8ab6e7dbd39f6dcdf80d7) nit: this could just be a one-liner using std::optional::value_or

0        return {m_script_sig.value_or(CScript{}), m_script_witness.value_or(CScriptWitness{})};

in src/wallet/spend.cpp:990 in bf275f358a outdated

986+        CScript script_sig;
987+        CScriptWitness script_witness;
988+        if (coin_control.HasScripts(coin->outpoint)) {
989+            auto [ss, sw] = coin_control.GetScripts(coin->outpoint);
990+            script_sig = ss;
991+            script_witness = sw;

theStack commented at 4:57 pm on May 3, 2023:

(in commit bf275f358a5e816cf9a8ab6e7dbd39f6dcdf80d7): nit: could avoid introducing temporary variables by directly assigning to script_sig/script_witness using std::tie

0            std::tie(script_sig, script_witness) = coin_control.GetScripts(coin->outpoint);

DrahtBot added the label Needs rebase on May 3, 2023

theStack commented at 5:00 pm on May 3, 2023: contributor

Concept ACK

achow101 force-pushed on May 10, 2023

DrahtBot removed the label Needs rebase on May 11, 2023

achow101 force-pushed on May 24, 2023

DrahtBot added the label CI failed on May 25, 2023

achow101 commented at 2:58 pm on May 25, 2023: member

Rebased

achow101 force-pushed on May 25, 2023

DrahtBot removed the label CI failed on May 25, 2023

DrahtBot added the label CI failed on May 30, 2023

DrahtBot removed the label CI failed on May 31, 2023

DrahtBot added the label CI failed on Jun 27, 2023

maflcko commented at 10:56 am on June 28, 2023: member

Needs rebase if still relevant

achow101 force-pushed on Jun 28, 2023

DrahtBot removed the label CI failed on Jun 29, 2023

in src/wallet/coincontrol.cpp:31 in 2de8e8309a outdated

29-    return m_external_txouts.count(output) > 0;
30+    const auto it = m_selected.find(output);
31+    if (it == m_selected.end()) {
32+        return false;
33+    }
34+    return it->second.HasTxOut();

theStack commented at 11:50 am on August 6, 2023:

(in commit 2de8e8309ac0ee60742e45b8cc1342e61c725566) nit: could be written as one-liner

0    return (it != m_selected.end() && it->second.HasTxOut());

(same for HasInputWeight, though that gets replaced anyway in the next commit)

achow101 commented at 8:49 pm on September 15, 2023:

Done

in src/wallet/spend.cpp:265 in 077783d24b outdated

160@@ -161,15 +161,17 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
161     PreSelectedInputs result;
162     const bool can_grind_r = wallet.CanGrindR();
163     for (const COutPoint& outpoint : coin_control.ListSelected()) {
164-        int input_bytes = -1;
165+        int32_t input_bytes = coin_control.GetInputWeight(outpoint).value_or(-1);

theStack commented at 12:17 pm on August 6, 2023:

(in commit 077783d24b7f1e3d3312e6cc12aee3f178400725) Looking at the “override calculated size with coin control specified size” branch removed below, seems like a GetVirtualTransactionSize call is missing here (in the case that GetInputWeight doesn’t return std::nullopt), as we want vbytes as input_bytes unit, not WU?

achow101 commented at 9:38 pm on September 15, 2023:

Good catch, fixed.

Tried to add a test but the effects of this mistake are not readily apparent enough to be tested in our functional test framework.

in src/wallet/coincontrol.cpp:90 in 705568146d outdated

83+{
84+    const auto it = m_selected.find(outpoint);
85+    if (it == m_selected.end()) {
86+        return std::nullopt;
87+    }
88+    return it->second.GetSequence();

theStack commented at 12:27 pm on August 6, 2023:

(in commit 705568146d4399ad8089adc4943786e2a0c0a62e) nit: could be written as a one-liner

0    return (it != m_selected.end()) ? it->second.GetSequence() : std::nullopt;

achow101 commented at 9:06 pm on September 15, 2023:

Done

in src/wallet/spend.cpp:1134 in 705568146d outdated

1009+    const uint32_t default_sequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL};
1010     for (const auto& coin : selected_coins) {
1011-        txNew.vin.push_back(CTxIn(coin->outpoint, CScript(), nSequence));
1012+        std::optional<uint32_t> sequence = coin_control.GetSequence(coin->outpoint);
1013+        if (sequence) {
1014+            // If an input as a preset sequence, we can't do anti-fee-sniping

theStack commented at 12:33 pm on August 6, 2023:

typo, I guess

0            // If an input has a preset sequence, we can't do anti-fee-sniping

achow101 commented at 9:06 pm on September 15, 2023:

Done

in src/wallet/coincontrol.cpp:99 in 6fcbf9f443 outdated

92+{
93+    const auto it = m_selected.find(outpoint);
94+    if (it == m_selected.end()) {
95+        return false;
96+    }
97+    return it->second.HasScripts();

theStack commented at 12:40 pm on August 6, 2023:

(in commit 6fcbf9f44358f85aab4c0b6617c113b4c37da5a8) nit: could be written as a one-liner

0    return (it != m_selected.end() && it->second.HasScripts());

achow101 commented at 9:06 pm on September 15, 2023:

Done

DrahtBot added the label Needs rebase on Sep 6, 2023

achow101 force-pushed on Sep 7, 2023

DrahtBot removed the label Needs rebase on Sep 7, 2023

achow101 force-pushed on Sep 15, 2023

DrahtBot added the label Needs rebase on Sep 19, 2023

achow101 force-pushed on Sep 27, 2023

DrahtBot removed the label Needs rebase on Sep 27, 2023

josibake commented at 4:51 pm on October 2, 2023: member

big Concept ACK

Still in the process of reviewing this, but it looks good so far. Building off of this PR in #28560

DrahtBot added the label Needs rebase on Oct 16, 2023

achow101 force-pushed on Oct 16, 2023

DrahtBot removed the label Needs rebase on Oct 16, 2023

theStack approved

theStack commented at 9:04 pm on October 29, 2023: contributor

Code-review ACK 59c363ec7245521e388b4d21abb44d7881907757

DrahtBot requested review from furszy on Oct 29, 2023

DrahtBot requested review from josibake on Oct 29, 2023

DrahtBot requested review from darosior on Oct 29, 2023

in src/wallet/spend.cpp:978 in c8544398fb outdated

976@@ -977,6 +977,10 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
977     FastRandomContext rng_fast;
978     CMutableTransaction txNew; // The resulting transaction that we make
979 
980+    if (coin_control.m_version) {
981+        txNew.nVersion = coin_control.m_version.value();

S3RK commented at 8:22 am on November 1, 2023:

Can’t figure out how is it set currently?

achow101 commented at 11:03 pm on November 13, 2023:

It isn’t. When FundTransaction extracts the inputs and outputs from CreateTransction’s result and puts them into the tx it got, so the version ends up being preserved. But CreateTransaction isn’t aware of the version selected by the user.

in src/wallet/coincontrol.h:115 in 03420020b6 outdated

90@@ -91,6 +91,8 @@ class CCoinControl
91     int m_max_depth = DEFAULT_MAX_DEPTH;
92     //! SigningProvider that has pubkeys and scripts to do spend size estimation for external inputs
93     FlatSigningProvider m_external_provider;
94+    //! Locktime
95+    std::optional<uint32_t> m_locktime;

S3RK commented at 5:04 pm on November 5, 2023:

in 03420020b6b4958ee6765be718724b89cd174d5c

locktime doesn’t belong to CoinControl because it has nothing to do with coins being spent. I’d prefer if we keep CoinControl scoped down to only containing data necessary to select and spend coins.

Same is true for some other fields in CoinControl. Maybe we should extract parameters not related to coins into a separate struct?

achow101 commented at 11:04 pm on November 13, 2023:

With miniscript, it does matter since scripts can contain locktimes.

in src/wallet/coincontrol.h:117 in c8544398fb outdated

92@@ -93,6 +93,8 @@ class CCoinControl
93     FlatSigningProvider m_external_provider;
94     //! Locktime
95     std::optional<uint32_t> m_locktime;
96+    //! Version
97+    std::optional<uint32_t> m_version;

S3RK commented at 5:04 pm on November 5, 2023:

in c8544398fb945b2402274981b22c70fdd8a67b9e

Same comment as for locktime

achow101 commented at 11:04 pm on November 13, 2023:

With miniscript, it does matter, as it controls whether something with OP_CSV is spendable.

in src/wallet/spend.cpp:1403 in 49ce0fe1e2 outdated

1361+                return false;
1362+            }
1363+
1364             // The input was not in the wallet, but is in the UTXO set, so select as external
1365-            coinControl.SelectExternal(outPoint, coins[outPoint].out);
1366+            preset_txin.SetTxOut(coins[outPoint].out);

S3RK commented at 5:06 pm on November 5, 2023:

I’m not sure about 49ce0fe1e2ec87508aea537d17e6bb13791be1aa and probably would prefer to drop it.

It makes interface less clear. It’s not obvious that I need to call SetTxOut to make input as external.

achow101 commented at 11:12 pm on November 13, 2023:

I’d prefer to leave it as is since it follows the pattern established by the following commits where various things are Set.

in src/wallet/coincontrol.h:27 in 59a7c74163 outdated

23@@ -24,6 +24,33 @@ const int DEFAULT_MAX_DEPTH = 9999999;
24 //! Default for -avoidpartialspends
25 static constexpr bool DEFAULT_AVOIDPARTIALSPENDS = false;
26 
27+class PreselectedInput

S3RK commented at 5:13 pm on November 5, 2023:

in 59a7c7416313693a3c8219363d9ffee1e2b86d4d

Naming is hard. We already have struct PreSelectedInputs which is quite confusing as it refers to different data.

PreSelectedInputs is basically set<COutput>. It seems we should find a better name for PreSelectedInputs. Maybe rename it to PreSelectedCoins?

It’d be nice to have consistent terms to refer to different data. How about we use input when we talk about tx.in and related info. And we use coin when its enriched data in context of coin selection.

Perhaps @murchandamus have thought about terminology.

achow101 commented at 11:16 pm on November 13, 2023:

:shrug:

in src/wallet/coincontrol.h:37 in 53cb734591 outdated

32@@ -33,6 +33,10 @@ class PreselectedInput
33     std::optional<int64_t> m_weight;
34     //! The sequence number for this input
35     std::optional<uint32_t> m_sequence;
36+    //! The scriptSig for this input
37+    std::optional<CScript> m_script_sig;

S3RK commented at 5:16 pm on November 5, 2023:

in 53cb734591cabb37fc6e3d147a4715203cf15ddb

There is redundancy between m_script_sig + m_script_witness and m_weight. One can set all of them and reach and inconsistent state, which will lead to error.

Can we make the interface better?

achow101 commented at 11:16 pm on November 13, 2023:

It is actually expected that both can be provided, and not match. We may be funding a transaction with other inputs that already have some signatures (e.g. a multisig where one of the parties is adding another input for fees, and one other party has already signed with sighash_single), so m_script_sig or m_script_witness may already exist, but are incomplete. m_weight may then also be provided, and it wouldn’t match since it would account for the signatures that have not yet been added.

S3RK commented at 5:22 pm on November 5, 2023: contributor

Approach ACK.

Left inline comments with more details.

It also would be nice to update PR description to explain what defects it fixes, i.e. expand “This can have an effect on whether and how anti-fee-sniping works.” by describing potential bugs or negative effects.

DrahtBot added the label Needs rebase on Nov 16, 2023

achow101 force-pushed on Nov 16, 2023

DrahtBot removed the label Needs rebase on Nov 16, 2023

in src/wallet/spend.cpp:1405 in 00d6669ce1 outdated

1406+
1407             // The input was not in the wallet, but is in the UTXO set, so select as external
1408-            coinControl.SelectExternal(outPoint, coins[outPoint].out);
1409+            preset_txin.SetTxOut(coins[outPoint].out);
1410         }
1411+        preset_txin.SetSequence(txin.nSequence);

ishaanam commented at 1:48 am on November 17, 2023:

This is done for every pre-selected input, even the ones that don’t specify the sequence. This seems confusing and also means that a transaction with any pre-selected inputs won’t do any anti-fee-sniping.

achow101 commented at 3:40 pm on November 28, 2023:

This is what currently happens. The behavior can be improved in future PRs.

ishaanam commented at 1:59 am on November 17, 2023: contributor

Concept ACK

in src/wallet/spend.cpp:1377 in ebc39da8e5 outdated

1345@@ -1341,6 +1346,9 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
1346         vecSend.push_back(recipient);
1347     }
1348 
1349+    // Set the user desired locktime
1350+    coinControl.m_locktime = tx.nLockTime;

ishaanam commented at 6:33 pm on November 18, 2023:

In ebc39da8e5f72521fb4889bc1adc0a80136ce3da “wallet: Explicitly preserve transaction locktime in CreateTransaction”:

The user-provided locktime and sequence values should also be set in the sendall coin_control. In the future if we want to take locktime into account when running AvailbleCoins, this would be very useful.

achow101 commented at 3:42 pm on November 28, 2023:

That can be done in such future PRs.

ishaanam commented at 4:13 pm on November 29, 2023: contributor

Looks like there is a silent merge conflict in src/wallet/test/fuzz/notifications.cpp

DrahtBot added the label CI failed on Nov 29, 2023

achow101 force-pushed on Nov 29, 2023

achow101 force-pushed on Dec 7, 2023

DrahtBot removed the label CI failed on Dec 7, 2023

ishaanam commented at 10:37 pm on December 7, 2023: contributor

ACK 1d1a6ff03215cf3aadd5fd1118f354b1ba8ffa5a

DrahtBot requested review from S3RK on Dec 7, 2023

DrahtBot requested review from theStack on Dec 7, 2023

theStack approved

theStack commented at 0:36 am on December 8, 2023: contributor

re-ACK 1d1a6ff03215cf3aadd5fd1118f354b1ba8ffa5a

fanquake commented at 10:35 am on December 8, 2023: member

@josibake @furszy want to take another look here.

in src/wallet/coincontrol.cpp:20 in 7b5d22f44f outdated

13@@ -14,69 +14,104 @@ CCoinControl::CCoinControl()
14 
15 bool CCoinControl::HasSelected() const
16 {
17-    return !m_selected_inputs.empty();
18+    return !m_selected.empty();
19 }
20 
21 bool CCoinControl::IsSelected(const COutPoint& output) const

josibake commented at 10:55 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: kind of annoying that we call this an output but below in GetExternalOutput we (correctly) call it an outpoint, would be nice to use consistent naming if this ever gets retouched.

achow101 commented at 8:25 pm on December 8, 2023:

Renamed to outpoint here and elsewhere.

in src/wallet/coincontrol.cpp:40 in 7b5d22f44f outdated

43-    return std::make_optional(ext_it->second);
44+    return it->second.GetTxOut();
45 }
46 
47-void CCoinControl::Select(const COutPoint& output)
48+PreselectedInput& CCoinControl::Select(const COutPoint& output)

josibake commented at 10:58 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: s/output/outpoint/

in src/wallet/coincontrol.cpp:62 in 7b5d22f44f outdated

71 }
72 
73 std::vector<COutPoint> CCoinControl::ListSelected() const
74 {
75-    return {m_selected_inputs.begin(), m_selected_inputs.end()};
76+    std::vector<COutPoint> out;

josibake commented at 11:11 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: s/out/outpoints/, in this case I think it helps make it more clear why you are doing the the transform: we are returning just the outpoints from the m_selected vector

achow101 commented at 8:25 pm on December 8, 2023:

Done

in src/wallet/coincontrol.cpp:34 in 7b5d22f44f outdated

34 std::optional<CTxOut> CCoinControl::GetExternalOutput(const COutPoint& outpoint) const
35 {
36-    const auto ext_it = m_external_txouts.find(outpoint);
37-    if (ext_it == m_external_txouts.end()) {
38+    const auto it = m_selected.find(outpoint);
39+    if (it == m_selected.end() || !it->second.HasTxOut()) {

josibake commented at 11:24 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

Is there a situation where an internal input would have a value for TxOut, accidentally or purposefully? Only asking because it feels slightly more fragile to rely only on this to determine if something is external or internal vs having two separate vectors, but I also don’t have a good sense if what I’m worried about is even possible (internal having TxOut set) or if its even that bad if we confuse external and internal inputs :shrug:

achow101 commented at 7:49 pm on December 8, 2023:

The consequence is that size estimation will overestimate that input’s size by 1 byte, which is up to 4 weight units of overestimation and therefore fees. However it should not be possible to accidentally set an internal input to be external as all calls to SelectExternal are guarded by a !IsMine().

in src/wallet/spend.cpp:120 in 5ce3008d46 outdated

116@@ -117,8 +117,9 @@ static std::optional<int64_t> GetSignedTxinWeight(const CWallet* wallet, const C
117                                                   const bool can_grind_r)
118 {
119     // If weight was provided, use that.
120-    if (coin_control && coin_control->HasInputWeight(txin.prevout)) {
121-        return coin_control->GetInputWeight(txin.prevout);
122+    std::optional<int32_t> weight;

josibake commented at 11:34 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

GetInputWeight is currently returning int64_t but I think int32_t is the more appropriate choice given that this represents weight.

achow101 commented at 8:27 pm on December 8, 2023:

GetTransactionInputWeight returns an int64_t, so I’ve unified these to use int64_t as well.

in src/wallet/spend.cpp:265 in 5ce3008d46 outdated

261@@ -261,15 +262,20 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
262     const bool can_grind_r = wallet.CanGrindR();
263     std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpFees(coin_control.ListSelected(), coin_selection_params.m_effective_feerate);
264     for (const COutPoint& outpoint : coin_control.ListSelected()) {
265-        int input_bytes = -1;
266+        int32_t input_bytes = coin_control.GetInputWeight(outpoint).value_or(-1);

josibake commented at 11:36 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

same as above, type mismatch between int32_t and what’s returned, int64_t

in src/wallet/coincontrol.cpp:72 in 5ce3008d46 outdated

71@@ -72,15 +72,10 @@ void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight)
72     m_selected[outpoint].SetInputWeight(weight);
73 }
74 
75-bool CCoinControl::HasInputWeight(const COutPoint& outpoint) const
76+std::optional<int64_t> CCoinControl::GetInputWeight(const COutPoint& outpoint) const

josibake commented at 11:51 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

shouldn’t this be int32_t?

in src/wallet/coincontrol.h:57 in 5ce3008d46 outdated

45@@ -46,9 +46,7 @@ class PreselectedInput
46     /** Set the weight for this input. */
47     void SetInputWeight(int64_t weight);
48     /** Retrieve the input weight for this input. */
49-    int64_t GetInputWeight() const;
50-    /** Return whether the input weight is set. */
51-    bool HasInputWeight() const;
52+    std::optional<int64_t> GetInputWeight() const;

josibake commented at 11:51 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

int32_t

in src/wallet/coincontrol.h:161 in 5ce3008d46 outdated

135-    bool HasInputWeight(const COutPoint& outpoint) const;
136     /**
137      * Returns the input weight.
138      */
139-    int64_t GetInputWeight(const COutPoint& outpoint) const;
140+    std::optional<int64_t> GetInputWeight(const COutPoint& outpoint) const;

josibake commented at 11:51 am on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

int32_t

in src/wallet/coincontrol.cpp:136 in 2a06c4c2b3 outdated

134+void PreselectedInput::SetScriptWitness(const CScriptWitness& script_wit)
135+{
136+    m_script_witness = script_wit;
137+}
138+
139+bool PreselectedInput::HasScripts() const

josibake commented at 12:06 pm on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/2a06c4c2b33c614cc3f877bdc013c8df9e847ca9:

in earlier commits, you were replacing the “Has -> Get” pattern with a “Get” that returned a std::optional. Why can’t we also do that here?

achow101 commented at 8:27 pm on December 8, 2023:

Good point. changed to use optionals.

in src/wallet/spend.cpp:1189 in 2a06c4c2b3 outdated

1161@@ -1162,7 +1162,13 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1162             // If an input has a preset sequence, we can't do anti-fee-sniping
1163             use_anti_fee_sniping = false;
1164         }
1165-        txNew.vin.emplace_back(coin->outpoint, CScript(), sequence.value_or(default_sequence));
1166+        CScript script_sig;
1167+        CScriptWitness script_witness;
1168+        if (coin_control.HasScripts(coin->outpoint)) {
1169+            std::tie(script_sig, script_witness) = coin_control.GetScripts(coin->outpoint);
1170+        }

josibake commented at 12:09 pm on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/2a06c4c2b33c614cc3f877bdc013c8df9e847ca9:

could be something like:

0        auto scripts = GetScripts(coin->outpoint);
1        if (scripts) {
2            std::tie(script_sig, script_witness) = *scripts;
3        }

if you decide to drop the “Has” method.

in src/wallet/coincontrol.cpp:43 in e53789afe6 outdated

38@@ -39,7 +39,9 @@ std::optional<CTxOut> CCoinControl::GetExternalOutput(const COutPoint& outpoint)
39 
40 PreselectedInput& CCoinControl::Select(const COutPoint& output)
41 {
42-    return m_selected[output];
43+    auto& input = m_selected[output];
44+    input.SetPosition(m_selection_pos++);

josibake commented at 12:17 pm on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/e53789afe60ddbcc014931467ae6b4cab15d0c67:

TIL m_var++ passes the value, then increments.

nit: I think it’s considered more idiomatic (and more readable imo) to do something like:

0    input.SetPosition(m_selection_pos);
1    m_selection_pos++;

achow101 commented at 8:27 pm on December 8, 2023:

Done

in src/wallet/interfaces.cpp:284 in 3d1a5f273c outdated

280@@ -281,12 +281,12 @@ class WalletImpl : public Wallet
281         CAmount& fee) override
282     {
283         LOCK(m_wallet->cs_wallet);
284-        auto res = CreateTransaction(*m_wallet, recipients, change_pos,
285+        auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos),

josibake commented at 12:25 pm on December 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/25273/commits/3d1a5f273c3c01b47ea4c74503c8353b2632dce8:

note for a follow-up: would be nice if we could use this convention throughout the codebase instead of switching the magic number into a std::optional, like we are doing here.

josibake commented at 12:35 pm on December 8, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/25273/commits/1d1a6ff03215cf3aadd5fd1118f354b1ba8ffa5a

Left some nits that can be ignored or addressed in a follow-up, but nothing major. There is a type mismatch with GetInputWeight returning int64_t but being assigned to int32_t. Would be great to fix that but also not a super big deal since weight will never be greater than int32_t anyway (but should def be fixed in a followup).

wallet: Introduce and use PreselectedInput class in CCoinControl

Instead of having different maps for selected inputs, external inputs,
and input weight in CCoinControl, have a class PreselectedInput which
tracks stores that information for each input.

e1abfb5b20

coincontrol: Replace HasInputWeight with returning optional from Get 5321786b9d

wallet: Replace SelectExternal with SetTxOut

Instead of having a separate CCoinControl::SelectExternal function, we
can use the normal CCoinControl::Select function and explicitly use
PreselectedInput::SetTxOut in the caller. The semantics of what an
external input is remains.

596642c5a9

wallet: Set preset input sequence through coin control 4d335bb1e0

wallet: Explicitly preserve transaction locktime in CreateTransaction

We provide the preset nLockTime to CCoinControl so that
CreateTransactionInternal can be aware of it and set it in the produced
transaction.

0fefcbb776

wallet: Explicitly preserve transaction version in CreateTransaction

We provide the preset nVersion to CCoinControl so that
CreateTransactionInternal can be aware of it and set it in the produced
transaction.

14e50746f6

achow101 force-pushed on Dec 8, 2023

wallet: Explicitly preserve scriptSig and scriptWitness in CreateTransaction

When creating a transaction with preset inputs, also preserve the
scriptSig and scriptWitness for those preset inputs if they are provided
(e.g. in fundrawtransaction).

2d39db7aa1

wallet: use optional for change position as an optional in CreateTransaction

Instead of making -1 a magic number meaning no change or random change
position, use an optional to have that meaning.

758501b713

wallet: return CreatedTransactionResult from FundTransaction

Instead of using the output parameters, return CreatedTransactionResult
from FundTransaction in the same way that CreateTransaction does.
Additionally, instead of modifying the original CMutableTransaction, the
result from CreateTransactionInternal is used.

0295b44c25

achow101 force-pushed on Dec 8, 2023

DrahtBot added the label CI failed on Dec 9, 2023

josibake commented at 1:24 pm on December 9, 2023: member

ACK https://github.com/bitcoin/bitcoin/commit/0295b44c257fe23f1ad344aff11372d67864f0db

Thanks for taking the suggestions! Looks like the CI failure is a transient failure, I ran the same test locally and it passed. Probably just needs to be re-run.

DrahtBot requested review from theStack on Dec 9, 2023

DrahtBot requested review from ishaanam on Dec 9, 2023

S3RK commented at 9:03 am on December 11, 2023: contributor

Code review ACK 0295b44c257fe23f1ad344aff11372d67864f0db

Still not a huge fan of 596642c5a9 and I have same concerns as josibake. I prefer explicit defitino of external inputs rather than indirectly through SetTxOut. Though I don’t want this to block the PR.

Also it seems two commits were squashed “Explicitly preserve scriptSig and scriptWitness in CreateTransaction” and “Preserve preset input order”. Was that intentional?

DrahtBot removed review request from S3RK on Dec 11, 2023

josibake commented at 9:11 am on December 11, 2023: member

Still not a huge fan of https://github.com/bitcoin/bitcoin/commit/596642c5a9f52dda2599b0bde424366bb22b3c6e and I have same concerns as josibake

My concerns were addressed by #25273 (review). I think the approach in https://github.com/bitcoin/bitcoin/commit/596642c5a9f52dda2599b0bde424366bb22b3c6e is much simpler and as the comment points out, the worst-case scenario if inputs are labeled incorrectly is overestimating the fee by 1 byte.

DrahtBot removed the label CI failed on Dec 11, 2023

fanquake merged this on Dec 11, 2023

fanquake closed this on Dec 11, 2023

josibake commented at 5:35 pm on December 11, 2023: member

For the reviewers of this PR, #28560 is a follow-up that further refactors FundTransaction by cleaning up the SFFO parsing and removing the SFFO logic from FundTransaction. It does this by passing a CRecipient vector to FundTransaction, which also sets us up to be able to pass silent payment addresses to FundTransaction in a later PR.

in src/wallet/spend.cpp:1357 in 14e50746f6 outdated

1352@@ -1349,6 +1353,9 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
1353     // Set the user desired locktime
1354     coinControl.m_locktime = tx.nLockTime;
1355 
1356+    // Set the user desired version
1357+    coinControl.m_version = tx.nVersion;

furszy commented at 12:45 pm on December 12, 2023:

it seems that the version field is not sanitized. The user provided tx.nVersion is a int32_t while coinControl.m_version is a uint32_t.

furszy referenced this in commit 7da0ccc683 on Dec 12, 2023

furszy referenced this in commit 6255c483fc on Dec 12, 2023

furszy referenced this in commit 37c75c5820 on Dec 12, 2023

achow101 referenced this in commit 9f0f83d650 on Dec 13, 2023

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Code Coverage

Reviews

Conflicts