Merge bitcoin/bitcoin#34993: wallet: `NotifyCanGetAddressesChanged` when advancing `next_index`
What changed, and why it matters
This Bitcoin Core change fixes a wallet notification bug. In the GUI, the 'Receive' button could stay clickable even when the wallet had actually run out of addresses it could generate. The patch makes sure the wallet emits a 'CanGetAddressesChanged' signal whenever the next available address index advances, so the UI can disable the Receive button at the right time. It is a correctness/UI fix, not a remote code execution or theft vulnerability.
No urgent security action required. This is a low-severity UI-correctness fix. Users and downstream maintainers should include it in regular updates to avoid confusing GUI behavior where the Receive button appears active when no address can be generated.
Security signals we found
UI state desynchronization leading to user-action failure
Missing event notification after state mutation
Refactor to centralize state-change notifications
Watch-only wallet edge case with hardened derivation and exhausted keypool
Evidence from the diff
The commit refactors WalletDescriptor so that range_start, range_end, and next_index are private and only modifiable through accessor methods (GetStart/GetEnd/GetNext, IncNext/DecNext/SetEnd). It introduces IncIndex(), DecIndex(), and SetRangeEnd() in DescriptorScriptPubKeyMan that compare CanGetAddresses() before and after the mutation and fire NotifyCanGetAddressesChanged() only when the result changes. Previously, advancing next_index after TopUp() did not always notify listeners, so GUI state could lag behind the actual inability to produce further addresses in edge cases such as watch-only wallets with hardened derivation paths that exhaust the keypool.
Changed components
src/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/walletutil.hsrc/wallet/export.cppBitcoin Core wallet descriptor keypool managementBitcoin Core GUI Receive button stateInspect captured patch +97 / −30
### src/wallet/export.cpp
@@ -36,8 +36,8 @@ util::Expected<std::vector<WalletDescInfo>, std::string> ExportDescriptors(const
wallet_descriptor.creation_time,
wallet.IsActiveScriptPubKeyMan(*desc_spk_man),
wallet.IsInternalScriptPubKeyMan(desc_spk_man),
- is_range ? std::optional(std::make_pair(wallet_descriptor.range_start, wallet_descriptor.range_end)) : std::nullopt,
- wallet_descriptor.next_index
+ is_range ? std::optional(std::make_pair(wallet_descriptor.GetStart(), wallet_descriptor.GetEnd())) : std::nullopt,
+ wallet_descriptor.GetNext()
);
}
return wallet_descriptors;
### src/wallet/scriptpubkeyman.cpp
@@ -871,6 +871,42 @@ std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::GenerateNe
return spkm;
}
+void DescriptorScriptPubKeyMan::IncIndex()
+{
+ AssertLockHeld(cs_desc_man);
+
+ const auto old_can = CanGetAddresses();
+ m_wallet_descriptor.IncNext();
+ const auto new_can = CanGetAddresses();
+ if (old_can != new_can) {
+ NotifyCanGetAddressesChanged();
+ }
+}
+
+void DescriptorScriptPubKeyMan::DecIndex()
+{
+ AssertLockHeld(cs_desc_man);
+
+ const auto old_can = CanGetAddresses();
+ m_wallet_descriptor.DecNext();
+ const auto new_can = CanGetAddresses();
+ if (old_can != new_can) {
+ NotifyCanGetAddressesChanged();
+ }
+}
+
+void DescriptorScriptPubKeyMan::SetRangeEnd(int32_t end)
+{
+ AssertLockHeld(cs_desc_man);
+
+ const auto old_can = CanGetAddresses();
+ m_wallet_descriptor.SetEnd(end);
+ const auto new_can = CanGetAddresses();
+ if (old_can != new_can) {
+ NotifyCanGetAddressesChanged();
+ }
+}
+
util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type)
{
// Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later
@@ -891,11 +927,11 @@ util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetNewDestination(const
// Get the scriptPubKey from the descriptor
FlatSigningProvider out_keys;
std::vector<CScript> scripts_temp;
- if (m_wallet_descriptor.range_end <= m_max_cached_index && !TopUp(1)) {
+ if (m_wallet_descriptor.GetEnd() <= m_max_cached_index && !TopUp(1)) {
// We can't generate anymore keys
return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};
}
- if (!m_wallet_descriptor.descriptor->ExpandFromCache(m_wallet_descriptor.next_index, m_wallet_descriptor.cache, scripts_temp, out_keys)) {
+ if (!m_wallet_descriptor.descriptor->ExpandFromCache(m_wallet_descriptor.GetNext(), m_wallet_descriptor.cache, scripts_temp, out_keys)) {
// We can't generate anymore keys
return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};
}
@@ -904,7 +940,7 @@ util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetNewDestination(const
if (!ExtractDestination(scripts_temp[0], dest)) {
return util::Error{_("Error: Cannot extract destination from the generated scriptpubkey")}; // shouldn't happen
}
- m_wallet_descriptor.next_index++;
+ IncIndex();
WalletBatch(m_storage.GetDatabase()).WriteDescriptor(GetID(), m_wallet_descriptor);
return dest;
}
@@ -975,19 +1011,18 @@ util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetReservedDestination(c
{
LOCK(cs_desc_man);
auto op_dest = GetNewDestination(type);
- index = m_wallet_descriptor.next_index - 1;
+ index = m_wallet_descriptor.GetNext() - 1;
return op_dest;
}
void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CTxDestination& addr)
{
LOCK(cs_desc_man);
// Only return when the index was the most recent
- if (m_wallet_descriptor.next_index - 1 == index) {
- m_wallet_descriptor.next_index--;
+ if (m_wallet_descriptor.GetNext() - 1 == index) {
+ DecIndex();
}
WalletBatch(m_storage.GetDatabase()).WriteDescriptor(GetID(), m_wallet_descriptor);
- NotifyCanGetAddressesChanged();
}
std::map<CKeyID, CKey> DescriptorScriptPubKeyMan::GetKeys() const
@@ -1060,13 +1095,11 @@ bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int siz
}
// Calculate the new range_end
- int32_t new_range_end = std::max(m_wallet_descriptor.next_index + (int32_t)target_size, m_wallet_descriptor.range_end);
+ int32_t new_range_end = std::max(m_wallet_descriptor.GetNext() + (int32_t)target_size, m_wallet_descriptor.GetEnd());
// If the descriptor is not ranged, we actually just want to fill the first cache item
if (!m_wallet_descriptor.descriptor->IsRange()) {
new_range_end = 1;
- m_wallet_descriptor.range_end = 1;
- m_wallet_descriptor.range_start = 0;
}
FlatSigningProvider provider;
@@ -1102,14 +1135,13 @@ bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int siz
}
m_max_cached_index++;
}
- m_wallet_descriptor.range_end = new_range_end;
+ SetRangeEnd(new_range_end);
batch.WriteDescriptor(GetID(), m_wallet_descriptor);
// By this point, the cache size should be the size of the entire range
- assert(m_wallet_descriptor.range_end - 1 == m_max_cached_index);
+ assert(m_wallet_descriptor.GetEnd() - 1 == m_max_cached_index);
m_storage.TopUpCallback(new_spks, this);
- NotifyCanGetAddressesChanged();
return true;
}
@@ -1119,18 +1151,18 @@ std::vector<WalletDestination> DescriptorScriptPubKeyMan::MarkUnusedAddresses(co
std::vector<WalletDestination> result;
if (IsMine(script)) {
int32_t index = m_map_script_pub_keys[script];
- if (index >= m_wallet_descriptor.next_index) {
+ if (index >= m_wallet_descriptor.GetNext()) {
WalletLogPrintf("%s: Detected a used keypool item at index %d, mark all keypool items up to this item as used\n", __func__, index);
auto out_keys = std::make_unique<FlatSigningProvider>();
std::vector<CScript> scripts_temp;
- while (index >= m_wallet_descriptor.next_index) {
- if (!m_wallet_descriptor.descriptor->ExpandFromCache(m_wallet_descriptor.next_index, m_wallet_descriptor.cache, scripts_temp, *out_keys)) {
+ while (index >= m_wallet_descriptor.GetNext()) {
+ if (!m_wallet_descriptor.descriptor->ExpandFromCache(m_wallet_descriptor.GetNext(), m_wallet_descriptor.cache, scripts_temp, *out_keys)) {
throw std::runtime_error(std::string(__func__) + ": Unable to expand descriptor from cache");
}
CTxDestination dest;
ExtractDestination(scripts_temp[0], dest);
result.push_back({dest, std::nullopt});
- m_wallet_descriptor.next_index++;
+ IncIndex();
}
}
if (!TopUp()) {
@@ -1222,7 +1254,7 @@ bool DescriptorScriptPubKeyMan::CanGetAddresses(bool internal) const
LOCK(cs_desc_man);
return m_wallet_descriptor.descriptor->IsSingleType() &&
m_wallet_descriptor.descriptor->IsRange() &&
- (HavePrivateKeys() || m_wallet_descriptor.next_index < m_wallet_descriptor.range_end || m_wallet_descriptor.descriptor->CanSelfExpand());
+ (HavePrivateKeys() || m_wallet_descriptor.GetNext() < m_wallet_descriptor.GetEnd() || m_wallet_descriptor.descriptor->CanSelfExpand());
}
bool DescriptorScriptPubKeyMan::HavePrivateKeys() const
@@ -1240,7 +1272,7 @@ bool DescriptorScriptPubKeyMan::HaveCryptedKeys() const
unsigned int DescriptorScriptPubKeyMan::GetKeyPoolSize() const
{
LOCK(cs_desc_man);
- return m_wallet_descriptor.range_end - m_wallet_descriptor.next_index;
+ return m_wallet_descriptor.GetEnd() - m_wallet_descriptor.GetNext();
}
int64_t DescriptorScriptPubKeyMan::GetTimeFirstKey() const
@@ -1479,7 +1511,7 @@ void DescriptorScriptPubKeyMan::Load()
{
LOCK(cs_desc_man);
std::set<CScript> new_spks;
- for (int32_t i = m_wallet_descriptor.range_start; i < m_wallet_descriptor.range_end; ++i) {
+ for (int32_t i = m_wallet_descriptor.GetStart(); i < m_wallet_descriptor.GetEnd(); ++i) {
FlatSigningProvider out_keys;
std::vector<CScript> scripts_temp;
if (!m_wallet_descriptor.descriptor->ExpandFromCache(i, m_wallet_descriptor.cache, scripts_temp, out_keys)) {
@@ -1645,12 +1677,12 @@ bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescript
return true;
}
- if (descriptor.range_start > m_wallet_descriptor.range_start ||
- descriptor.range_end < m_wallet_descriptor.range_end) {
+ if (descriptor.GetStart() > m_wallet_descriptor.GetStart() ||
+ descriptor.GetEnd() < m_wallet_descriptor.GetEnd()) {
// Use inclusive range for error
error = strprintf("new range must include current range = [%d,%d]",
- m_wallet_descriptor.range_start,
- m_wallet_descriptor.range_end - 1);
+ m_wallet_descriptor.GetStart(),
+ m_wallet_descriptor.GetEnd() - 1);
return false;
}
### src/wallet/scriptpubkeyman.h
@@ -337,6 +337,9 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
{}
WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man);
+ void IncIndex() EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
+ void DecIndex() EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
+ void SetRangeEnd(int32_t end) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
//! Same as 'TopUp' but designed for use within a batch transaction context
bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0);
### src/wallet/walletutil.h
@@ -62,15 +62,41 @@ fs::path GetWalletDir();
/** Descriptor with some wallet metadata */
class WalletDescriptor
{
+private:
+ int32_t range_start = 0; // First item in range; start of range, inclusive, i.e. [range_start, range_end). This never changes.
+ int32_t next_index = 0; // Position of the next item to generate
+ int32_t range_end = 0; // Item after the last; end of range, exclusive, i.e. [range_start, range_end). This will increment with each TopUp()
public:
std::shared_ptr<Descriptor> descriptor;
uint256 id; // Descriptor ID (calculated once at descriptor initialization/deserialization)
uint64_t creation_time = 0;
- int32_t range_start = 0; // First item in range; start of range, inclusive, i.e. [range_start, range_end). This never changes.
- int32_t range_end = 0; // Item after the last; end of range, exclusive, i.e. [range_start, range_end). This will increment with each TopUp()
- int32_t next_index = 0; // Position of the next item to generate
DescriptorCache cache;
+ int32_t GetStart() const { return range_start; }
+ int32_t GetNext() const { return next_index; }
+ int32_t GetEnd() const { return range_end; }
+
+ //! Increments the next_index of the descriptor.
+ void IncNext()
+ {
+ next_index++;
+ }
+
+ //! Increments the next_index of the descriptor.
+ void DecNext()
+ {
+ next_index--;
+ }
+
+ //! Sets the range_end of the descriptor.
+ void SetEnd(int32_t end)
+ {
+ if (!descriptor->IsRange()) {
+ CHECK_NONFATAL(end == 1);
+ }
+ range_end = end;
+ }
+
void DeserializeDescriptor(const std::string& str)
{
std::string error;
@@ -95,7 +121,13 @@ class WalletDescriptor
}
WalletDescriptor() = default;
- WalletDescriptor(std::shared_ptr<Descriptor> descriptor, uint64_t creation_time, int32_t range_start, int32_t range_end, int32_t next_index) : descriptor(descriptor), id(DescriptorID(*descriptor)), creation_time(creation_time), range_start(range_start), range_end(range_end), next_index(next_index) { }
+ WalletDescriptor(std::shared_ptr<Descriptor> descriptor, uint64_t creation_time, int32_t range_start, int32_t range_end, int32_t next_index)
+ : range_start(descriptor->IsRange() ? range_start : 0),
+ next_index(next_index),
+ range_end(descriptor->IsRange() ? range_end : 1),
+ descriptor(descriptor),
+ id(DescriptorID(*descriptor)),
+ creation_time(creation_time) {}
};
WalletDescriptor GenerateWalletDescriptor(const CExtPubKey& master_key, const OutputType& output_type, bool internal);Why this scored 26/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.