mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-21 14:34:49 +01:00
Code Issues Projects Releases Wiki Activity

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

Browse source 
807de2cebd wallet: skip R-value grinding for external signers (Sjors Provoost)
72b763e452 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 #26030

ACKs for top commit:
  S3RK:
    ACK 807de2cebd
  achow101:
    ACK 807de2cebd
  furszy:
    ACK 807de2ce
  ishaanam:
    utACK 807de2cebd

Tree-SHA512: 64f626a3030ef0ab1e43af86d8fba113151512561baf425e6e5182af53df3a64fa9e85c7f67bf4ed15b5ad6e5d5afc7fbba8b6e1f3bad388e48db51cb9446074
This commit is contained in:
Andrew Chow 2023-02-27 12:37:30 -05:00
commit 710cab1d43
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
5 changed files with 27 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
src/wallet/scriptpubkeyman.cpp
View file

@ -2495,7 +2495,7 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction&
input.FillSignatureData(sigdata);
std::unique_ptr<FlatSigningProvider> keys = std::make_unique<FlatSigningProvider>();
std::unique_ptr<FlatSigningProvider> script_keys = GetSigningProvider(script, sign);
std::unique_ptr<FlatSigningProvider> script_keys = GetSigningProvider(script, /*include_private=*/sign);
if (script_keys) {
keys->Merge(std::move(*script_keys));
} else {
@ -2536,7 +2536,7 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction&
}
}
SignPSBTInput(HidingSigningProvider(keys.get(), !sign, !bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!sign, /*hide_origin=*/!bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
bool signed_one = PSBTInputSigned(input);
if (n_signed && (signed_one || !sign)) {
@ -2553,7 +2553,7 @@ TransactionError DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTransaction&
if (!keys) {
continue;
}
UpdatePSBTOutput(HidingSigningProvider(keys.get(), true, !bip32derivs), psbtx, i);
UpdatePSBTOutput(HidingSigningProvider(keys.get(), /*hide_secret=*/true, /*hide_origin=*/!bip32derivs), psbtx, i);
}
return TransactionError::OK;

14
src/wallet/spend.cpp
View file

@ -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<SigningProvider> 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<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
PreSelectedInputs result;
std::vector<COutPoint> 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<PreSelectedInputs> 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<uint256> trusted_parents;
for (const auto& entry : wallet.mapWallet)
@ -305,7 +307,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(), 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<CreatedTransactionResult> 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) {

4
src/wallet/spend.h
View file

@ -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};

14
src/wallet/wallet.cpp
View file

@ -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<CTxOut>
{
// 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<CTxOut>
continue;
}
const std::unique_ptr<SigningProvider> 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);

5
src/wallet/wallet.h
View file

@ -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);