From 807de2cebdad960c2b52185528ca8960ec694f49 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 23 Feb 2023 18:30:12 +0100 Subject: [PATCH] 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> --- src/wallet/spend.cpp | 14 ++++++++------ src/wallet/spend.h | 4 ++-- src/wallet/wallet.cpp | 14 ++++++++++---- src/wallet/wallet.h | 5 ++++- 4 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 1a79b59d12..865f6e41a6 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -30,11 +30,11 @@ using interfaces::FoundBlock; namespace wallet { static constexpr size_t OUTPUT_GROUP_MAX_ENTRIES{100}; -int CalculateMaximumSignedInputSize(const CTxOut& txout, const COutPoint outpoint, const SigningProvider* provider, const CCoinControl* coin_control) +int CalculateMaximumSignedInputSize(const CTxOut& txout, const COutPoint outpoint, const SigningProvider* provider, bool can_grind_r, const CCoinControl* coin_control) { CMutableTransaction txn; txn.vin.push_back(CTxIn(outpoint)); - if (!provider || !DummySignInput(*provider, txn.vin[0], txout, coin_control)) { + if (!provider || !DummySignInput(*provider, txn.vin[0], txout, can_grind_r, coin_control)) { return -1; } return GetVirtualTransactionInputSize(txn.vin[0]); @@ -43,7 +43,7 @@ int CalculateMaximumSignedInputSize(const CTxOut& txout, const COutPoint outpoin int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet, const CCoinControl* coin_control) { const std::unique_ptr provider = wallet->GetSolvingProvider(txout.scriptPubKey); - return CalculateMaximumSignedInputSize(txout, COutPoint(), provider.get(), coin_control); + return CalculateMaximumSignedInputSize(txout, COutPoint(), provider.get(), wallet->CanGrindR(), coin_control); } // txouts needs to be in the order of tx.vin @@ -163,6 +163,7 @@ util::Result FetchSelectedInputs(const CWallet& wallet, const PreSelectedInputs result; std::vector vPresetInputs; coin_control.ListSelected(vPresetInputs); + const bool can_grind_r = wallet.CanGrindR(); for (const COutPoint& outpoint : vPresetInputs) { int input_bytes = -1; CTxOut txout; @@ -181,7 +182,7 @@ util::Result FetchSelectedInputs(const CWallet& wallet, const } if (input_bytes == -1) { - input_bytes = CalculateMaximumSignedInputSize(txout, outpoint, &coin_control.m_external_provider, &coin_control); + input_bytes = CalculateMaximumSignedInputSize(txout, outpoint, &coin_control.m_external_provider, can_grind_r, &coin_control); } // If available, override calculated size with coin control specified size @@ -214,6 +215,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, const int min_depth = {coinControl ? coinControl->m_min_depth : DEFAULT_MIN_DEPTH}; const int max_depth = {coinControl ? coinControl->m_max_depth : DEFAULT_MAX_DEPTH}; const bool only_safe = {coinControl ? !coinControl->m_include_unsafe_inputs : true}; + const bool can_grind_r = wallet.CanGrindR(); std::set trusted_parents; for (const auto& entry : wallet.mapWallet) @@ -305,7 +307,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, std::unique_ptr provider = wallet.GetSolvingProvider(output.scriptPubKey); - int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), coinControl); + int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), can_grind_r, coinControl); bool solvable = provider ? InferDescriptor(output.scriptPubKey, *provider)->IsSolvable() : false; bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); @@ -828,7 +830,7 @@ static util::Result CreateTransactionInternal( coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); // Get size of spending the change output - int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, &wallet); + int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, &wallet, /*coin_control=*/nullptr); // If the wallet doesn't know how to sign change output, assume p2sh-p2wpkh // as lower-bound to allow BnB to do it's thing if (change_spend_size == -1) { diff --git a/src/wallet/spend.h b/src/wallet/spend.h index d8da556d29..ad122b7ef3 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -17,8 +17,8 @@ namespace wallet { /** Get the marginal bytes if spending the specified output from this transaction. * Use CoinControl to determine whether to expect signature grinding when calculating the size of the input spend. */ -int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet, const CCoinControl* coin_control = nullptr); -int CalculateMaximumSignedInputSize(const CTxOut& txout, const COutPoint outpoint, const SigningProvider* pwallet, const CCoinControl* coin_control = nullptr); +int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet, const CCoinControl* coin_control); +int CalculateMaximumSignedInputSize(const CTxOut& txout, const COutPoint outpoint, const SigningProvider* pwallet, bool can_grind_r, const CCoinControl* coin_control); struct TxSize { int64_t vsize{-1}; int64_t weight{-1}; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index daddd6446d..e5c03849af 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1642,7 +1642,7 @@ void CWallet::InitWalletFlags(uint64_t flags) // Helper for producing a max-sized low-S low-R signature (eg 71 bytes) // or a max-sized low-S signature (e.g. 72 bytes) depending on coin_control -bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut &txout, const CCoinControl* coin_control) +bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut &txout, bool can_grind_r, const CCoinControl* coin_control) { // Fill in dummy signatures for fee calculation. const CScript& scriptPubKey = txout.scriptPubKey; @@ -1650,7 +1650,7 @@ bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut // 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. - const bool use_max_sig = coin_control && (coin_control->fAllowWatchOnly || coin_control->IsExternalSelected(tx_in.prevout)); + const bool use_max_sig = coin_control && (coin_control->fAllowWatchOnly || coin_control->IsExternalSelected(tx_in.prevout) || !can_grind_r); if (!ProduceSignature(provider, use_max_sig ? DUMMY_MAXIMUM_SIGNATURE_CREATOR : DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigdata)) { return false; } @@ -1706,6 +1706,7 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const std::vector { // Fill in dummy signatures for fee calculation. int nIn = 0; + const bool can_grind_r = CanGrindR(); for (const auto& txout : txouts) { CTxIn& txin = txNew.vin[nIn]; @@ -1718,8 +1719,8 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const std::vector continue; } const std::unique_ptr provider = GetSolvingProvider(txout.scriptPubKey); - if (!provider || !DummySignInput(*provider, txin, txout, coin_control)) { - if (!coin_control || !DummySignInput(coin_control->m_external_provider, txin, txout, coin_control)) { + if (!provider || !DummySignInput(*provider, txin, txout, can_grind_r, coin_control)) { + if (!coin_control || !DummySignInput(coin_control->m_external_provider, txin, txout, can_grind_r, coin_control)) { return false; } } @@ -4081,6 +4082,11 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return true; } +bool CWallet::CanGrindR() const +{ + return !IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER); +} + bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 32cb3e3f59..16eee1e050 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -947,6 +947,9 @@ public: //! Adds the ScriptPubKeyMans given in MigrationData to this wallet, removes LegacyScriptPubKeyMan, //! and where needed, moves tx and address book entries to watchonly_wallet or solvable_wallet bool ApplyMigrationData(MigrationData& data, bilingual_str& error) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + //! Whether the (external) signer performs R-value signature grinding + bool CanGrindR() const; }; /** @@ -1004,7 +1007,7 @@ bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name); //! Remove wallet name from persistent configuration so it will not be loaded on startup. bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name); -bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut &txout, const CCoinControl* coin_control = nullptr); +bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut &txout, bool can_grind_r, const CCoinControl* coin_control); bool FillInputToWeight(CTxIn& txin, int64_t target_weight);