wallet: skip R-value signature grinding for external signers #26032

Sjors · 2022-09-07T09:38:01Z

When producing a dummy signature for the purpose of estimating the transaction fee, do not assume an external signer performs R-value grinding on the signature.

In particular, this avoids a scenario where the fee rate is 1 sat / vbyte and a transaction with a 72 byte signature is not accepted into our mempool.

Suggested testing:

On master, launch with -signet and create an external signer wallet using e.g. a Trezor and HWI, see guide (with the GUI it should "just work" once you have the HWI path configured).
Create a few addresses and fund them from the faucet: https://signet.bc-2.jp/ (wait for confirmation)
Create another address, and now send the entire wallet to it, set the fee to 1 sat/byte
Most likely this transaction never gets broadcast and you won't see it on the signet explorer
With this PR, try again.
Check the explorer and inspect the transaction. Each input witness starts with either 30440220 (R has 32 bytes) or 30440221 (R has 33 bytes). See this explainer for DER encoding.

Fixes #26030

DrahtBot · 2022-09-08T17:43:27Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, ishaanam, S3RK, achow101

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:

#26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
#26152 (Bump unconfirmed ancestor transactions to target feerate by Xekyo)
#26066 (wallet: Refactor and document CoinControl by aureleoules)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#22341 (rpc: add getxpub by Sjors)

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.

Sjors · 2022-09-13T07:14:51Z

Added a release note.

luke-jr · 2022-09-20T22:30:02Z

A functional test would be nice

luke-jr

I wonder if it would make sense to move grind to be an attribute of (or at least method of) the SigningProvider?

luke-jr · 2022-09-20T22:37:11Z

src/psbt.cpp

@@ -320,7 +320,7 @@ PrecomputedTransactionData PrecomputePSBTData(const PartiallySignedTransaction&
    return txdata;
 }

-bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash,  SignatureData* out_sigdata, bool finalize)
+bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash,  SignatureData* out_sigdata, bool finalize, bool grind)


nit: grind is a bit unclear, maybe require_optimal_signature_size or !use_max_sig (like in wallet_tests)?

luke-jr · 2022-09-20T22:41:15Z

src/wallet/scriptpubkeyman.cpp

@@ -618,7 +618,7 @@ SigningResult LegacyScriptPubKeyMan::SignMessage(const std::string& message, con
    return SigningResult::SIGNING_FAILED;
 }

-TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, const PrecomputedTransactionData& txdata, int sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize) const
+TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, const PrecomputedTransactionData& txdata, int sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize, bool grind) const


grind gets ignored here. What if a watch-only input is used, and the signer for it can't grind?

ishaanam

Concept ACK, but I am a bit confused about the approach so I've left a question below.

ishaanam · 2022-09-21T17:14:00Z

src/psbt.cpp

@@ -361,7 +361,7 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
    sigdata.witness = false;
    bool sig_complete;
    if (txdata == nullptr) {
-        sig_complete = ProduceSignature(provider, DUMMY_SIGNATURE_CREATOR, utxo.scriptPubKey, sigdata);
+        sig_complete = ProduceSignature(provider, grind ? DUMMY_SIGNATURE_CREATOR : DUMMY_MAXIMUM_SIGNATURE_CREATOR, utxo.scriptPubKey, sigdata);


It seems like the dummy signature produced here isn't used for fee estimation, but for figuring out what signatures that the psbt is missing (for use when analyzing a psbt). It looks like this behavior was introduced in cb40b3a.

@achow101 I could use some hints here.

Size estimation occurs in CreateTransaction, inside of SelectCoins and CalculateMaximumSignedInputSize. FillPSBT and the functions it calls shouldn't be touched.

Sjors · 2022-09-22T08:37:31Z

A functional test would be nice

Indeed, but I'm not sure how. Can our test suite produce ungrinded signatures?

luke-jr · 2022-09-22T17:17:23Z

Worst case, use a hardcoded descriptor and signatures?

Sjors · 2022-09-23T09:23:50Z

@luke-jr in that case the entire regtest chain needs to be deterministic, otherwise you're spending non-existing coins. But IIRC it is, so I'll take another look at that. Does seem a bit brittle.

Sjors · 2022-09-23T13:47:57Z

I changed the approach. DummySignInput now checks (the new) coin_control->m_external_signer to determine use_max_sig. This is much simpler.

Since it piggybacks on the logic for external inputs I think a functional test is less necessary?

luke-jr · 2022-09-24T02:02:50Z

src/wallet/coincontrol.h

+    //! Wallet inputs are signed by an external signer
+    bool m_external_signer = false;


will be*

But I don't think this is a good name for the flag. It's quite possible external signers might support grinding, and will want a way to optimise fees too?

I could call it m_can_grind_r and default to true? And then for external signers we set it to false until we have a way (e.g. via HWI) to communicate this capability. That said, I don't know if anyone will implement it, since Taproot doesn't have this problem.

luke-jr · 2022-09-24T02:04:29Z

src/wallet/spend.cpp

    TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res),
           res ? res->fee : 0, res ? res->change_pos : 0);
    if (!res) return res;
    const auto& txr_ungrouped = *res;
    // try with avoidpartialspends unless it's enabled already
-    if (txr_ungrouped.fee > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
+    if (txr_ungrouped.fee > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !tmp_cc.m_avoid_partial_spends) {


No need to change this?

Not really, but accessing both the original coin_control and tmp_cc seems like asking for future regressions.

luke-jr · 2022-09-24T02:06:17Z

Also, please don't rebase when there's no reason to. (If you could rebase back, it'd be nice.)

Sjors · 2022-09-26T08:50:24Z

please don't rebase when there's no reason to

Given how much refactoring is (still) going on in the wallet, not rebasing increases the risk of a silent merge conflict. Although in this case back-porting might be very useful, so I'll see if I can move it back to something a bit older.

furszy

Instead of adding the m_external_signer flag to coin control, what about passing wallet.IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) to the DummySignInput function? (or pass the wallet ref and call IsWalletFlagSet internally).

I think that we should keep the coin control object as a container for user modifiable parameters. And not start using it to pass wallet internal flags across functions.

Sjors · 2022-09-28T08:19:13Z

@furszy there's three call sites to DummySignInput. Two are in CWallet::DummySignTx which has direct access to IsWalletFlagSet(). The third is in CalculateMaximumSignedInputSize (wallet/spend.cpp) which only has access to a SigningProvider.

I could expand SigningProvider with m_can_grind_r and CanGrindR(). That seems the right place for it, but that might touch a lot of code (haven't tried).

The easier way is to add an can_grind_r argument to CalculateMaximumSignedInputSize. All it's call sites (outside of tests & bench at least) are in coin_selection.cpp and spend.cpp and have access to a CWallet.

furszy · 2022-09-29T15:01:33Z

I could expand SigningProvider with m_can_grind_r and CanGrindR(). That seems the right place for it, but that might touch a lot of code (haven't tried).

yeah, SigningProvider seems to be the right place for it.

The easier way is to add an can_grind_r argument to CalculateMaximumSignedInputSize. All it's call sites (outside of tests & bench at least) are in coin_selection.cpp and spend.cpp and have access to a CWallet.

Would look like this furszy@dccb120. Not that many changes.

Plus, could also include the unused CWallet::DummySignTx removal too: ce81b9a

achow101 · 2022-10-24T20:20:02Z

ACK b133ab9

I could expand SigningProvider with m_can_grind_r and CanGrindR(). That seems the right place for it, but that might touch a lot of code (haven't tried).

I think that would be a layer violation. The SigningProvider does not actually do any signing at all, it just provides information that is useful for signing. A SigningProvider could be providing information that pertains to both internal and external outputs, in which case the method of signing differs for each input.

Sjors · 2022-10-27T13:26:20Z

I think that would be a layer violation.

I'll leave this PR as-is then. We can always refactor things.

S3RK · 2022-10-31T08:37:47Z

Concept ACK. This PR is a clearly an improvement from users' perspective.

Approach wise, I don't like that CCoinControl is becoming bloated collection of loosely related parameters. We already have 15 public members there. And from my point of view, ability to grind signatures has nothing to do with coin control.

I reviewed both this PR and the alternative approach and I like the second option better. You can make it even simpler like this S3RK/bitcoin@8f11705

Sjors · 2023-02-22T12:52:58Z

I considered switching to @S3RK's approach of adding extra argument to DummySignInput rather than expanding CCoinControl. The result is here: https://github.com/Sjors/bitcoin/tree/2023/02/external-signer-feerate-leave-ccoincontrol-alone

The downside of this is approach is that I had to sprinkle const bool can_grind_r = !wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); in several different places. @furszy's commit does it in three places. This is fine right now, but at some point we might have some external signers that do support grinding. We'd then have to update the check everywhere. So although CCoinControl is not really meant to keep track of this, it's the most convenient place.

So I'm keeping the original commit for now.

S3RK · 2023-02-23T08:03:57Z

The downside of this is approach is that I had to sprinkle const bool can_grind_r = !wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); in several different places. @furszy's commit does it in three places. This is fine right now, but at some point we might have some external signers that do support grinding. We'd then have to update the check everywhere.

What if we encapsulate the logic in Wallet::CanGrindR()?

The ability to grind signatures is a characteristic of a wallet IIUC. So that should be pretty future proof.

Sjors · 2023-02-23T11:05:49Z

@S3RK done!

furszy

ACK 302f07e
Looking quite straightforward now.

furszy · 2023-02-23T13:14:24Z

src/wallet/spend.cpp

@@ -305,7 +305,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,

            std::unique_ptr<SigningProvider> provider = wallet.GetSolvingProvider(output.scriptPubKey);

-            int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), coinControl);
+            int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), wallet.CanGrindR(), coinControl);


tiny nit:
could cache this result into a variable outside of the loop (after the only_safe variable)

Good point, I introduced a const bool in ~~both~~ all three places where it's used in a loop.

Though perhaps it's overkill, because IsWalletFlagSet is just m_wallet_flags & flag and not a database lookup.

yeah, I think that the only place that worth is inside AvailableCoins. And not only because of the overhead that introduces (it's one "AND" operation on an atomic variable per wallet UTXO), I was more thinking about the code structure and what we should do if we want to decrease the cs_wallet lock contention there.

still, this is a tiny nit from a person that is thinking about future stuff.

When producing a dummy signature for the purpose of estimating the transaction fee, do not assume an external signer performs R-value grinding on the signature. In particular, this avoids a scenario where the fee rate is 1 sat / vbyte and a transaction with a 72 byte signature is not accepted into our mempool. This commit also drops the nullptr default for CCoinControl arguments for functions that it touches. This is because having a boolean argument right next to an optional pointer is error prone. Co-Authored-By: S3RK <1466284+S3RK@users.noreply.github.com>

furszy

ACK 807de2c

ishaanam

utACK 807de2c

ishaanam · 2022-10-05T17:36:23Z

src/wallet/wallet.cpp

    // Use max sig if watch only inputs were used or if this particular input is an external input
    // to ensure a sufficient fee is attained for the requested feerate.


nit: this comment can be updated

Suggested change

// Use max sig if watch only inputs were used or if this particular input is an external input

// to ensure a sufficient fee is attained for the requested feerate.

// Use max sig if watch only inputs were used, if this particular input is an external input,

// or if this wallet uses an external signer, to ensure a sufficient fee is attained for the requested feerate.

Added to a followup PR #27180.

S3RK · 2023-02-27T08:13:11Z

ACK 807de2c

Thanks your patience and addressing the feedback.

achow101 · 2023-02-27T16:34:36Z

ACK 807de2c

…rnal signers 807de2c wallet: skip R-value grinding for external signers (Sjors Provoost) 72b763e wallet: annotate bools in descriptor SPKM FillPSBT() (Sjors Provoost) Pull request description: When producing a dummy signature for the purpose of estimating the transaction fee, do not assume an external signer performs R-value grinding on the signature. In particular, this avoids a scenario where the fee rate is 1 sat / vbyte and a transaction with a 72 byte signature is not accepted into our mempool. Suggested testing: 1. On master, launch with `-signet` and create an external signer wallet using e.g. a Trezor and HWI, see [guide](https://github.com/bitcoin/bitcoin/blob/master/doc/external-signer.md#example-usage) (with the GUI it should "just work" once you have the HWI path configured). 2. Create a few addresses and fund them from the faucet: https://signet.bc-2.jp/ (wait for confirmation) 3. Create another address, and now send the entire wallet to it, set the fee to 1 sat/byte 4. Most likely this transaction never gets broadcast and you won't see it on the [signet explorer](https://explorer.bc-2.jp) 5. With this PR, try again. 6. Check the explorer and inspect the transaction. Each input witness starts with either `30440220` (R has 32 bytes) or `30440221` (R has 33 bytes). See this explainer for [DER encoding](https://bitcoin.stackexchange.com/questions/92680/what-are-the-der-signature-and-sec-format). Fixes bitcoin#26030 ACKs for top commit: S3RK: ACK 807de2c achow101: ACK 807de2c furszy: ACK 807de2c ishaanam: utACK 807de2c Tree-SHA512: 64f626a3030ef0ab1e43af86d8fba113151512561baf425e6e5182af53df3a64fa9e85c7f67bf4ed15b5ad6e5d5afc7fbba8b6e1f3bad388e48db51cb9446074

6fc5f4f doc: DummySignInput mention external signer (Sjors Provoost) Pull request description: Followups for #26032. So far nothing major. ACKs for top commit: ishaanam: ACK 6fc5f4f S3RK: ACK 6fc5f4f Tree-SHA512: e27edde9853487fe3eef8213f991aae3724f318bbbe0b11da23759879adaf9a31771e6ea0c30baaebca149032780b89b32aa540ff456ca3d5ec6adb0371749c6

6fc5f4f doc: DummySignInput mention external signer (Sjors Provoost) Pull request description: Followups for bitcoin#26032. So far nothing major. ACKs for top commit: ishaanam: ACK 6fc5f4f S3RK: ACK 6fc5f4f Tree-SHA512: e27edde9853487fe3eef8213f991aae3724f318bbbe0b11da23759879adaf9a31771e6ea0c30baaebca149032780b89b32aa540ff456ca3d5ec6adb0371749c6

fanquake added the Wallet label Sep 7, 2022

Sjors force-pushed the 2022/09/external-signer-feerate branch from 9558c8a to a58038a Compare September 13, 2022 07:15

DrahtBot mentioned this pull request Sep 14, 2022

BIP-322 basic support #24058

Closed

luke-jr reviewed Sep 20, 2022

View reviewed changes

ishaanam reviewed Sep 21, 2022

View reviewed changes

Sjors force-pushed the 2022/09/external-signer-feerate branch 3 times, most recently from bb5b170 to b133ab9 Compare September 23, 2022 14:54

luke-jr suggested changes Sep 24, 2022

View reviewed changes

furszy reviewed Sep 26, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 18, 2022

Multiprocess bitcoin #10102

Draft

DrahtBot mentioned this pull request Nov 25, 2022

Wallet: estimate the size of signed inputs using descriptors #26567

Merged

DrahtBot mentioned this pull request Nov 29, 2022

tracing: Only prepare tracepoint arguments when actually tracing #26593

Open

wallet: annotate bools in descriptor SPKM FillPSBT()

72b763e

Sjors force-pushed the 2022/09/external-signer-feerate branch from b133ab9 to 302f07e Compare February 23, 2023 11:05

DrahtBot mentioned this pull request Feb 23, 2023

wallet: Have the wallet store the key for automatically generated descriptors #26728

Closed

furszy approved these changes Feb 23, 2023

View reviewed changes

DrahtBot requested a review from achow101 February 23, 2023 13:18

This was referenced Feb 23, 2023

wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets #26008

Merged

wallet: rpc to add automatically generated descriptors #25907

Closed

rpc: add path to gethdkey #22341

Closed

Sjors force-pushed the 2022/09/external-signer-feerate branch 2 times, most recently from 540ec59 to 98a4dad Compare February 23, 2023 17:29

Sjors force-pushed the 2022/09/external-signer-feerate branch from 98a4dad to 807de2c Compare February 23, 2023 17:30

This was referenced Feb 23, 2023

Bump unconfirmed ancestor transactions to target feerate #26152

Merged

wallet: Refactor and document CoinControl #26066

Merged

furszy approved these changes Feb 24, 2023

View reviewed changes

ishaanam reviewed Feb 26, 2023

View reviewed changes

achow101 merged commit 710cab1 into bitcoin:master Feb 27, 2023

Sjors deleted the 2022/09/external-signer-feerate branch March 1, 2023 09:03

Sjors mentioned this pull request Mar 1, 2023

doc: DummySignInput mention external signer #27180

Merged

ryanofsky mentioned this pull request Apr 13, 2023

refactor: Use util::Result class for wallet loading #25722

Draft

bitcoin locked and limited conversation to collaborators Feb 29, 2024

		//! Wallet inputs are signed by an external signer
		bool m_external_signer = false;

		// Use max sig if watch only inputs were used or if this particular input is an external input
		// to ensure a sufficient fee is attained for the requested feerate.

wallet: skip R-value signature grinding for external signers #26032

wallet: skip R-value signature grinding for external signers #26032

Conversation

Sjors commented Sep 7, 2022 • edited Loading

DrahtBot commented Sep 8, 2022 • edited Loading

Reviews

Conflicts

Sjors commented Sep 13, 2022

luke-jr commented Sep 20, 2022

luke-jr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishaanam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Sep 22, 2022

luke-jr commented Sep 22, 2022

Sjors commented Sep 23, 2022 • edited Loading

Sjors commented Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luke-jr commented Sep 24, 2022

Sjors commented Sep 26, 2022

furszy left a comment

Choose a reason for hiding this comment

Sjors commented Sep 28, 2022

furszy commented Sep 29, 2022

achow101 commented Oct 24, 2022

Sjors commented Oct 27, 2022

S3RK commented Oct 31, 2022

Sjors commented Feb 22, 2023 • edited Loading

S3RK commented Feb 23, 2023

Sjors commented Feb 23, 2023

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Sjors Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

furszy Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

ishaanam left a comment

Choose a reason for hiding this comment

ishaanam Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S3RK commented Feb 27, 2023

achow101 commented Feb 27, 2023

Sjors commented Sep 7, 2022 •

edited

Loading

DrahtBot commented Sep 8, 2022 •

edited

Loading

Sjors commented Sep 23, 2022 •

edited

Loading

Sjors commented Sep 23, 2022 •

edited

Loading

Sjors commented Feb 22, 2023 •

edited

Loading

Sjors Feb 23, 2023 •

edited

Loading

Sjors Feb 23, 2023 •

edited

Loading

furszy Feb 23, 2023 •

edited

Loading

ishaanam Oct 5, 2022 •

edited

Loading