Merge a6fc293c0a into merged_master (Bitcoin PR bitcoin/bitcoin#25656)

ReserveDestination::SetBlindingPubKey was removed since it was only used
in one place (in Wallet::GetNewChangeDestination), and after this change
it was silently breaking blinded change addresses by somehow replacing
the destination with CNoDestination...
This commit is contained in:
Byron Hambly 2024-10-31 09:26:04 +02:00
commit a33b20f0ff
5 changed files with 36 additions and 64 deletions

View file

@ -298,26 +298,22 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat
return true;
}
bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error)
util::Result<CTxDestination> LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
{
if (LEGACY_OUTPUT_TYPES.count(type) == 0) {
error = _("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types");
return false;
return util::Error{_("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types")};
}
assert(type != OutputType::BECH32M);
LOCK(cs_KeyStore);
if (!CanGetAddresses(internal)) {
error = _("Error: Keypool ran out, please call keypoolrefill first");
return false;
return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};
}
if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
error = _("Error: Keypool ran out, please call keypoolrefill first");
return false;
return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};
}
address = GetDestinationForKey(keypool.vchPubKey, type);
return true;
return GetDestinationForKey(keypool.vchPubKey, type);
}
bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal)
@ -1765,17 +1761,12 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle
return true;
}
bool DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error)
util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
{
LOCK(cs_desc_man);
auto op_dest = GetNewDestination(type);
index = m_wallet_descriptor.next_index - 1;
if (op_dest) {
address = *op_dest;
} else {
error = util::ErrorString(op_dest);
}
return bool(op_dest);
return op_dest;
}
void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CTxDestination& addr)

View file

@ -179,7 +179,7 @@ public:
virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; }
virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; }
virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) { return false; }
virtual util::Result<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return util::Error{Untranslated("Not supported")}; }
virtual void KeepDestination(int64_t index, const OutputType& type) {}
virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {}
@ -366,7 +366,7 @@ public:
bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override;
bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override;
bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) override;
util::Result<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override;
void KeepDestination(int64_t index, const OutputType& type) override;
void ReturnDestination(int64_t index, bool internal, const CTxDestination&) override;
@ -582,7 +582,7 @@ public:
bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override;
bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override;
bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) override;
util::Result<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) override;
void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) override;
// Tops up the descriptor cache and m_map_script_pub_keys. The cache is stored in the wallet file

View file

@ -1154,13 +1154,9 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
for (const auto& value : map_recipients_sum) {
// Reserve a new key pair from key pool. If it fails, provide a dummy
// destination in case we don't need change.
CTxDestination dest;
bilingual_str dest_err;
if (index >= reservedest.size() || !reservedest[index]->GetReservedDestination(dest, true, dest_err)) {
if (dest_err.empty()) {
dest_err = _("Please call keypoolrefill first");
}
error = _("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + dest_err;
auto op_dest = reservedest[index]->GetReservedDestination(true);
if (index >= reservedest.size() || !op_dest) {
error = _("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + util::ErrorString(op_dest);
// ELEMENTS: We need to put a dummy destination here. Core uses an empty script
// but we can't because empty scripts indicate fees (which trigger assertion
// failures in `BlindTransaction`). We also set the index to -1, indicating
@ -1168,7 +1164,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// returned by the `ReturnDestination` loop below.
mapScriptChange[value.first] = std::pair<int, CScript>(-1, dummy_script);
} else {
mapScriptChange[value.first] = std::pair<int, CScript>(index, GetScriptForDestination(dest));
mapScriptChange[value.first] = std::pair<int, CScript>(index, GetScriptForDestination(*op_dest));
++index;
}
}
@ -1194,20 +1190,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
continue;
}
CTxDestination dest;
bilingual_str dest_err;
if (index >= reservedest.size() || !reservedest[index]->GetReservedDestination(dest, true, dest_err)) {
if (dest_err.empty()) {
dest_err = _("Keypool ran out, please call keypoolrefill first");
}
return util::Error{_("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + dest_err};
auto op_dest = reservedest[index]->GetReservedDestination(true);
if (index >= reservedest.size() || !op_dest) {
return util::Error{_("Transaction needs a change address, but we can't generate it.") + Untranslated(" ") + util::ErrorString(op_dest)};
}
CScript scriptChange = GetScriptForDestination(dest);
CScript scriptChange = GetScriptForDestination(*op_dest);
// A valid destination implies a change script (and
// vice-versa). An empty change script will abort later, if the
// change keypool ran out, but change is required.
CHECK_NONFATAL(IsValidDestination(dest) != (scriptChange == dummy_script));
CHECK_NONFATAL(IsValidDestination(*op_dest) != (scriptChange == dummy_script));
mapScriptChange[asset] = std::pair<int, CScript>(index, scriptChange);
++index;
}

View file

@ -2583,19 +2583,19 @@ util::Result<CTxDestination> CWallet::GetNewChangeDestination(const OutputType t
{
LOCK(cs_wallet);
CTxDestination dest;
bilingual_str error;
ReserveDestination reservedest(this, type);
if (!reservedest.GetReservedDestination(dest, true, error)) {
return util::Error{error};
}
if (add_blinding_key) {
CPubKey blinding_pubkey = GetBlindingPubKey(GetScriptForDestination(dest));
reservedest.SetBlindingPubKey(blinding_pubkey, dest);
auto op_dest = reservedest.GetReservedDestination(true);
if (op_dest) {
reservedest.KeepDestination();
if (add_blinding_key) {
CTxDestination dest{*op_dest};
CPubKey blinding_pubkey = GetBlindingPubKey(GetScriptForDestination(dest));
std::visit(SetBlindingPubKeyVisitor(blinding_pubkey), dest);
return dest;
}
}
reservedest.KeepDestination();
return dest;
return op_dest;
}
std::optional<int64_t> CWallet::GetOldestKeyPoolTime() const
@ -2665,33 +2665,24 @@ std::set<std::string> CWallet::ListAddrBookLabels(const std::string& purpose) co
return label_set;
}
bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool internal, bilingual_str& error)
util::Result<CTxDestination> ReserveDestination::GetReservedDestination(bool internal)
{
m_spk_man = pwallet->GetScriptPubKeyMan(type, internal);
if (!m_spk_man) {
error = strprintf(_("Error: No %s addresses available."), FormatOutputType(type));
return false;
return util::Error{strprintf(_("Error: No %s addresses available."), FormatOutputType(type))};
}
if (nIndex == -1)
{
m_spk_man->TopUp();
CKeyPool keypool;
if (!m_spk_man->GetReservedDestination(type, internal, address, nIndex, keypool, error)) {
return false;
}
auto op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool);
if (!op_address) return op_address;
address = *op_address;
fInternal = keypool.fInternal;
}
dest = address;
return true;
}
void ReserveDestination::SetBlindingPubKey(const CPubKey& blinding_pubkey, CTxDestination& dest)
{
std::visit(SetBlindingPubKeyVisitor(blinding_pubkey), address);
dest = address;
return address;
}
void ReserveDestination::KeepDestination()

View file

@ -231,9 +231,7 @@ public:
}
//! Reserve an address
bool GetReservedDestination(CTxDestination& pubkey, bool internal, bilingual_str& error);
//! Attach a blinding pubkey to a reserved address
void SetBlindingPubKey(const CPubKey& blinding_pubkey, CTxDestination& dest);
util::Result<CTxDestination> GetReservedDestination(bool internal);
//! Return reserved address
void ReturnDestination();
//! Keep the address. Do not return it's key to the keypool when this object goes out of scope