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

Sjors commented at 9:38 AM on September 7, 2022: member

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

fanquake added the label Wallet on Sep 7, 2022

DrahtBot commented at 5:43 PM on September 8, 2022: contributor

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 commented at 7:14 AM on September 13, 2022: member

Added a release note.

Sjors force-pushed on Sep 13, 2022

luke-jr commented at 10:30 PM on September 20, 2022: member

A functional test would be nice

in src/psbt.cpp:323 in a58038ac5b outdated

 319 | @@ -320,7 +320,7 @@ PrecomputedTransactionData PrecomputePSBTData(const PartiallySignedTransaction&
 320 |      return txdata;
 321 |  }
 322 |  
 323 | -bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash,  SignatureData* out_sigdata, bool finalize)
 324 | +bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash,  SignatureData* out_sigdata, bool finalize, bool grind)

luke-jr commented at 10:37 PM on September 20, 2022:

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

in src/wallet/scriptpubkeyman.cpp:621 in a58038ac5b outdated

 617 | @@ -618,7 +618,7 @@ SigningResult LegacyScriptPubKeyMan::SignMessage(const std::string& message, con
 618 |      return SigningResult::SIGNING_FAILED;
 619 |  }
 620 |  
 621 | -TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, const PrecomputedTransactionData& txdata, int sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize) const
 622 | +TransactionError LegacyScriptPubKeyMan::FillPSBT(PartiallySignedTransaction& psbtx, const PrecomputedTransactionData& txdata, int sighash_type, bool sign, bool bip32derivs, int* n_signed, bool finalize, bool grind) const

luke-jr commented at 10:41 PM on September 20, 2022:

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

luke-jr commented at 10:41 PM on September 20, 2022: member

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

in src/psbt.cpp:364 in a58038ac5b outdated

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

ishaanam commented at 5:14 PM on September 21, 2022:

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

Sjors commented at 8:37 AM on September 22, 2022:

@achow101 I could use some hints here.

achow101 commented at 3:12 PM on September 22, 2022:

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

ishaanam commented at 5:20 PM on September 21, 2022: contributor

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

Sjors commented at 8:37 AM on September 22, 2022: member

A functional test would be nice

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

luke-jr commented at 5:17 PM on September 22, 2022: member

Worst case, use a hardcoded descriptor and signatures?

Sjors commented at 9:23 AM on September 23, 2022: member

@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 commented at 1:47 PM on September 23, 2022: member

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?

Sjors force-pushed on Sep 23, 2022

in src/wallet/coincontrol.h:64 in b133ab9b1e outdated

  59 | @@ -60,6 +60,8 @@ class CCoinControl
  60 |      int m_max_depth = DEFAULT_MAX_DEPTH;
  61 |      //! SigningProvider that has pubkeys and scripts to do spend size estimation for external inputs
  62 |      FlatSigningProvider m_external_provider;
  63 | +    //! Wallet inputs are signed by an external signer
  64 | +    bool m_external_signer = false;

luke-jr commented at 2:02 AM on September 24, 2022:

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?

Sjors commented at 8:44 AM on September 26, 2022:

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.

in src/wallet/spend.cpp:1064 in b133ab9b1e outdated

1061 |             res ? res->fee : 0, res ? res->change_pos : 0);
1062 |      if (!res) return res;
1063 |      const auto& txr_ungrouped = *res;
1064 |      // try with avoidpartialspends unless it's enabled already
1065 | -    if (txr_ungrouped.fee > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
1066 | +    if (txr_ungrouped.fee > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !tmp_cc.m_avoid_partial_spends) {

luke-jr commented at 2:04 AM on September 24, 2022:

No need to change this?

Sjors commented at 8:45 AM on September 26, 2022:

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

luke-jr changes_requested

luke-jr commented at 2:06 AM on September 24, 2022: member

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

Sjors commented at 8:50 AM on September 26, 2022: member

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 commented at 3:15 PM on September 26, 2022: member

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 commented at 8:19 AM on September 28, 2022: member

@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 commented at 3:01 PM on September 29, 2022: member

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 https://github.com/furszy/bitcoin-core/commit/dccb1205a581bc48e8c3e5230387c72843445587. Not that many changes.

Plus, could also include the unused CWallet::DummySignTx removal too: https://github.com/bitcoin/bitcoin/pull/25881/commits/ce81b9a4fa821e92813e62d15d0b1ce5131e0a2e

in src/wallet/wallet.cpp:1652 in b133ab9b1e outdated

1558 | @@ -1559,7 +1559,7 @@ bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut
1559 |  
1560 |      // Use max sig if watch only inputs were used or if this particular input is an external input
1561 |      // to ensure a sufficient fee is attained for the requested feerate.

ishaanam commented at 5:36 PM on October 5, 2022:

nit: this comment can be updated

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

Sjors commented at 9:06 AM on March 1, 2023:

Added to a followup PR #27180.

achow101 commented at 8:20 PM on October 24, 2022: member

ACK b133ab9b1ee3c824cfc2ca29e3cfc7054377a481

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 commented at 1:26 PM on October 27, 2022: member

I think that would be a layer violation.

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

S3RK commented at 8:37 AM on October 31, 2022: contributor

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@8f11705fccbace8534bbf5daf3306465f56c46aa

Sjors commented at 12:52 PM on February 22, 2023: member

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 commented at 8:03 AM on February 23, 2023: contributor

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.

wallet: annotate bools in descriptor SPKM FillPSBT() 72b763e452

Sjors force-pushed on Feb 23, 2023

Sjors commented at 11:05 AM on February 23, 2023: member

@S3RK done!

in src/wallet/spend.cpp:308 in 302f07ee5a outdated

 304 | @@ -305,7 +305,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
 305 |  
 306 |              std::unique_ptr<SigningProvider> provider = wallet.GetSolvingProvider(output.scriptPubKey);
 307 |  
 308 | -            int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), coinControl);
 309 | +            int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), wallet.CanGrindR(), coinControl);

furszy commented at 1:14 PM on February 23, 2023:

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

Sjors commented at 5:26 PM on February 23, 2023:

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

Sjors commented at 5:34 PM on February 23, 2023:

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

furszy commented at 6:56 PM on February 23, 2023:

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.

furszy approved

furszy commented at 1:17 PM on February 23, 2023: member

ACK 302f07ee Looking quite straightforward now.

DrahtBot requested review from achow101 on Feb 23, 2023

Sjors force-pushed on Feb 23, 2023

wallet: skip R-value grinding for external signers

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>

807de2cebd

Sjors force-pushed on Feb 23, 2023

furszy approved

furszy commented at 12:53 PM on February 24, 2023: member

ACK 807de2ce

ishaanam commented at 6:29 PM on February 26, 2023: contributor

utACK 807de2cebdad960c2b52185528ca8960ec694f49

S3RK commented at 8:13 AM on February 27, 2023: contributor

ACK 807de2cebdad960c2b52185528ca8960ec694f49

Thanks your patience and addressing the feedback.

achow101 commented at 4:34 PM on February 27, 2023: member

ACK 807de2cebdad960c2b52185528ca8960ec694f49

achow101 merged this on Feb 27, 2023

achow101 closed this on Feb 27, 2023

sidhujag referenced this in commit a33432a148 on Feb 28, 2023

Sjors deleted the branch on Mar 1, 2023

fanquake referenced this in commit 69ba5727d5 on Mar 8, 2023

sidhujag referenced this in commit b0d71056f2 on Mar 8, 2023

bitcoin locked this on Feb 29, 2024