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

achow101 · 2022-06-03T18:58:15Z

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 · 2022-06-04T00:47:56Z

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.

src/wallet/rpc/spend.cpp

luke-jr · 2022-06-18T15:48:20Z

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?

src/wallet/coincontrol.h

luke-jr · 2022-06-18T16:00:58Z

src/wallet/spend.cpp

+        uint32_t sequence = default_sequence;
+        if (coin_control.HasSequence(coin.outpoint)) {
+            sequence = coin_control.GetSequence(coin.outpoint);
+            // If an input as a preset sequence, we can't do anti-fee-sniping


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)

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.

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?

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.

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

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

achow101 · 2022-06-20T17:38:52Z

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.

SUMMARY: 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.

josibake

ACK 1d1a6ff

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

src/wallet/coincontrol.cpp

josibake · 2023-12-08T11:24:44Z

src/wallet/coincontrol.cpp

-    const auto ext_it = m_external_txouts.find(outpoint);
-    if (ext_it == m_external_txouts.end()) {
+    const auto it = m_selected.find(outpoint);
+    if (it == m_selected.end() || !it->second.HasTxOut()) {


in 7b5d22f:

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 🤷

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

src/wallet/spend.cpp

src/wallet/coincontrol.h

src/wallet/coincontrol.cpp

src/wallet/spend.cpp

src/wallet/coincontrol.cpp

josibake · 2023-12-08T12:25:50Z

src/wallet/interfaces.cpp

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


in 3d1a5f2:

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.

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.

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.

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

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

…saction 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).

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

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.

josibake · 2023-12-09T13:24:09Z

ACK 0295b44

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.

S3RK · 2023-12-11T09:03:35Z

Code review ACK 0295b44

Still not a huge fan of 596642c 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?

josibake · 2023-12-11T09:11:33Z

Still not a huge fan of 596642c and I have same concerns as josibake

My concerns were addressed by #25273 (comment). I think the approach in 596642c 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.

josibake · 2023-12-11T17:35:33Z

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.

furszy · 2023-12-12T12:45:01Z

src/wallet/spend.cpp

@@ -1349,6 +1353,9 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
    // Set the user desired locktime
    coinControl.m_locktime = tx.nLockTime;

+    // Set the user desired version
+    coinControl.m_version = tx.nVersion;


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.

Since bitcoin#25273, the "insert change at a random position" behavior is instructed by passing std::nullopt, and not -1 anymore. Also, document what 'std::nullopt' means.

Since bitcoin#25273, the behavior of 'inserting change at a random position' is instructed by passing std::nullopt instead of -1. Additionally, added missing documentation about the meaning of 'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()'

Since bitcoin#25273, the behavior of 'inserting change at a random position' is instructed by passing std::nullopt instead of -1. Also, added missing documentation about the meaning of 'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()'

37c75c5 test: wallet, fix change position out of range error (furszy) Pull request description: Fixes #29061. Only the benchmark is affected. Since #25273, the behavior of 'inserting change at a random position' is instructed by passing ´std::nullopt´ instead of -1. Also, added missing documentation about the meaning of 'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()' ACKs for top commit: achow101: ACK 37c75c5 kevkevinpal: ACK [37c75c5](37c75c5) BrandonOdiwuor: ACK 37c75c5 Tree-SHA512: d9a8d8533540455716a5090fcf407573cad9f0d0018a05f903f89e51620302f9b256318db6f7338b85c047f7fab372d724e916b1721d7ed302dbf3d845b08734

DrahtBot added RPC/REST/ZMQ Wallet labels Jun 3, 2022

DrahtBot added the Needs rebase label Jun 16, 2022

achow101 force-pushed the use-preset-tx-things branch from 996f7f8 to 664f743 Compare June 17, 2022 03:28

DrahtBot removed the Needs rebase label Jun 17, 2022

luke-jr reviewed Jun 18, 2022

View reviewed changes

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

luke-jr reviewed Jun 18, 2022

View reviewed changes

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

luke-jr reviewed Jun 18, 2022

View reviewed changes

achow101 force-pushed the use-preset-tx-things branch from 664f743 to 5e19bce Compare June 20, 2022 17:37

DrahtBot added the Needs rebase label Jun 20, 2022

achow101 force-pushed the use-preset-tx-things branch from 5e19bce to 37a15c0 Compare June 20, 2022 22:04

DrahtBot removed the Needs rebase label Jun 21, 2022

achow101 force-pushed the use-preset-tx-things branch 2 times, most recently from 0135553 to 45fb4fb Compare June 21, 2022 20:02

DrahtBot added the Needs rebase label Jul 12, 2022

achow101 force-pushed the use-preset-tx-things branch from 45fb4fb to ed25666 Compare July 12, 2022 22:02

DrahtBot removed the Needs rebase label Jul 12, 2022

DrahtBot mentioned this pull request Jul 13, 2022

util: Make BResult error a generic type instead of only bilingual_str #25601

Closed

This was referenced Jul 20, 2022

wallet: return change from SelectionResult #25647

Merged

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

josibake reviewed Dec 8, 2023

View reviewed changes

achow101 and others added 6 commits December 8, 2023 14:54

wallet: Introduce and use PreselectedInput class in CCoinControl

e1abfb5

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.

coincontrol: Replace HasInputWeight with returning optional from Get

5321786

wallet: Replace SelectExternal with SetTxOut

596642c

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.

wallet: Set preset input sequence through coin control

4d335bb

wallet: Explicitly preserve transaction locktime in CreateTransaction

0fefcbb

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

wallet: Explicitly preserve transaction version in CreateTransaction

14e5074

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

achow101 force-pushed the use-preset-tx-things branch from 1d1a6ff to 9c7eb4c Compare December 8, 2023 20:25

achow101 added 3 commits December 8, 2023 17:12

wallet: Explicitly preserve scriptSig and scriptWitness in CreateTran…

2d39db7

…saction 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).

wallet: use optional for change position as an optional in CreateTran…

758501b

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

achow101 force-pushed the use-preset-tx-things branch from 9c7eb4c to 0295b44 Compare December 8, 2023 22:12

DrahtBot added the CI failed label Dec 9, 2023

DrahtBot requested review from theStack and ishaanam December 9, 2023 13:24

DrahtBot mentioned this pull request Dec 9, 2023

fuzz: make FuzzedDataProvider usage deterministic #29043

Merged

DrahtBot removed the request for review from S3RK December 11, 2023 09:03

DrahtBot removed the CI failed label Dec 11, 2023

fanquake merged commit dabd704 into bitcoin:master Dec 11, 2023
16 checks passed

furszy reviewed Dec 12, 2023

View reviewed changes

aureleoules mentioned this pull request Dec 12, 2023

Benchmark WalletCreateTxUsePresetInputsAndCoinSelection crashes due to #25273 #29061

Closed

1 task

furszy mentioned this pull request Dec 12, 2023

bench: wallet, fix change position out of range error #29065

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achow101 commented Jun 3, 2022

DrahtBot commented Jun 4, 2022 •

edited

Loading

luke-jr commented Jun 18, 2022

luke-jr Jun 18, 2022

achow101 Jun 20, 2022

luke-jr Jun 20, 2022

achow101 Jun 20, 2022

darosior Mar 23, 2023 •

edited

Loading

achow101 Mar 23, 2023

achow101 commented Jun 20, 2022

josibake left a comment

josibake Dec 8, 2023 •

edited

Loading

achow101 Dec 8, 2023

josibake Dec 8, 2023 •

edited

Loading

josibake commented Dec 9, 2023

S3RK commented Dec 11, 2023

josibake commented Dec 11, 2023

josibake commented Dec 11, 2023

furszy Dec 12, 2023 •

edited

Loading

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

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

Conversation

achow101 commented Jun 3, 2022

DrahtBot commented Jun 4, 2022 • edited Loading

Code Coverage

Reviews

Conflicts

luke-jr commented Jun 18, 2022

luke-jr Jun 18, 2022

Choose a reason for hiding this comment

achow101 Jun 20, 2022

Choose a reason for hiding this comment

luke-jr Jun 20, 2022

Choose a reason for hiding this comment

achow101 Jun 20, 2022

Choose a reason for hiding this comment

darosior Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

achow101 Mar 23, 2023

Choose a reason for hiding this comment

achow101 commented Jun 20, 2022

josibake left a comment

Choose a reason for hiding this comment

josibake Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

achow101 Dec 8, 2023

Choose a reason for hiding this comment

josibake Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

josibake commented Dec 9, 2023

S3RK commented Dec 11, 2023

josibake commented Dec 11, 2023

josibake commented Dec 11, 2023

furszy Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

DrahtBot commented Jun 4, 2022 •

edited

Loading

darosior Mar 23, 2023 •

edited

Loading

josibake Dec 8, 2023 •

edited

Loading

josibake Dec 8, 2023 •

edited

Loading

furszy Dec 12, 2023 •

edited

Loading