wallet: fix crash on importdescriptors with a range ending at 2^31-1 #35989

shuv-amp commented at 8:15 PM on August 16, 2026: contributor

WalletDescriptor stores an exclusive range end in int32_t. Importing [2147483647, 2147483647] produces an end of 2^31, which narrows to a negative value and aborts the node during keypool top-up. An import without a range can hit the same issue with an oversized -keypool.

Check the exclusive end before constructing the wallet descriptor, covering both explicit and default ranges. The bound stays wallet-specific: scanning RPCs still accept INT32_MAX as an inclusive endpoint.

Also calculate the keypool top-up endpoint in int64_t before checking its bound, and reject reversed ranges before computing high - low in CheckDescriptorRangeBounds. These fix the overflows reached by keypoolrefill INT32_MAX after advancing a descriptor, and by [INT64_MAX, 0], respectively. Regression tests cover all three fixes.

I tested on macOS arm64 with Clang 22 and UBSan passed: 34 targeted unit cases and the importdescriptors, keypool, deriveaddresses (RPC and CLI), scantxoutset, and scanblocks functional tests.

DrahtBot added the label Wallet on Aug 16, 2026

DrahtBot commented at 8:15 PM on August 16, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35989.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	jeanpablojp, vicjuma
Stale ACK	kriss39, molnard, achow101

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36245 (wallet: start descriptor top up at range_start instead of index 0 by kriss39)
#36236 (wallet, rpc: add verify_balance option to importdescriptors by musaHaruna)
#35444 (wallet: make descriptor SPKM mutex non-recursive by w0xlt)

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.

jeanpablojp commented at 10:47 AM on August 23, 2026: contributor

Concept ACK

I reproduced the crash both ways on master.

in src/wallet/scriptpubkeyman.cpp:1067 in 16b31cae72

1064 | -    int32_t new_range_end = std::max(m_wallet_descriptor.next_index + (int32_t)target_size, m_wallet_descriptor.range_end);
1065 | +    // Calculate the new range_end. next_index is user controlled for imported descriptors
1066 | +    // and target_size comes from the user configurable -keypool, so this addition can
1067 | +    // overflow. There is no representable end in that case, so leave the range unchanged.
1068 | +    int32_t new_range_end = m_wallet_descriptor.range_end;
1069 | +    if (auto target_end{CheckedAdd(m_wallet_descriptor.next_index, static_cast<int32_t>(target_size))}) {

jeanpablojp commented at 10:47 AM on August 23, 2026:

Only the overflow case changes behaviour here, and I couldn't find a test for it. A keypoolrefill 2147483647 reaches this addition without going through an import, as long as every active descriptor has already advanced one index. On master, UBSan flags it. Worth a case so it doesn't come back?

shuv-amp commented at 7:48 PM on August 25, 2026:

Added. It only fails under UBSan though: the wrapped sum is negative, so std::max() discarded it anyway and range_end comes out the same either way.

DrahtBot added the label Needs rebase on Aug 24, 2026

shuv-amp force-pushed on Aug 25, 2026

DrahtBot removed the label Needs rebase on Aug 25, 2026

in src/wallet/scriptpubkeyman.cpp:1104 in def4fb0101 outdated

1101 | +    // descriptor and target_size is user controlled, so compute the end in a wider type.
1102 | +    // The range is left unchanged if that end does not fit in the int32_t it is stored in.
1103 | +    const int64_t target_end{int64_t{m_wallet_descriptor.GetNext()} + target_size};
1104 | +    int32_t new_range_end = m_wallet_descriptor.GetEnd();
1105 | +    if (target_end <= std::numeric_limits<int32_t>::max()) {
1106 | +        new_range_end = std::max(static_cast<int32_t>(target_end), m_wallet_descriptor.GetEnd());

molnard commented at 5:18 PM on September 2, 2026:

nit , readability choice:

        new_range_end = std::max(static_cast<int32_t>(target_end), new_range_end);

shuv-amp commented at 6:17 AM on September 5, 2026:

I'd keep GetEnd() here, it's explicit about the lower bound.

molnard commented at 6:03 PM on September 2, 2026: none

ACK def4fb0101b73f825db7f20ff6d6dd7170b5d5ca

I reviewed the code. The changes are straightforward and focused on fixing the reported issues.

Testing

The following results are from local test runs.

Compared master b811aeabad94ef48cd0f0fb1d2fcc456594aeedb with the PR tip:

Importing [2147483647,2147483647] terminated master. The PR returned error -8 and remained responsive.
Importing without a range under -keypool=3000000000 terminated master. The PR returned error -8 and remained responsive.
Calling keypoolrefill 2147483647 after advancing every active descriptor to index 1 terminated master. The PR returned error -4 and remained responsive.

Non-blocking follow-ups

Possbile follow-ups, worth considering separately from this PR:

Detecting/recovering invalid descriptor ranges already persisted by the old bug.
Centralizing validation of oversized -keypool values. For example: accepted as a wallet configuration option without an upper-bound check (wallet.cpp)
Clarifying whether TopUpWithDB should return failure when the requested endpoint is unrepresentable, instead of leaving the range unchanged and returning true (plus writes the unchanged descriptor). This is separate from the tested keypoolrefill RPC, which correctly returns an error.

DrahtBot requested review from jeanpablojp on Sep 2, 2026

shuv-amp commented at 6:47 AM on September 5, 2026: contributor

Detecting/recovering invalid descriptor ranges already persisted by the old bug.

Follow-up, agreed. On the reported import the descriptor write happens inside AddWalletDescriptor with the assert after it, and activation is back in ProcessDescriptorImport once that call returns, so the record persists unactivated even with active=true. On reload Load() runs zero iterations over the inverted range and TopUpKeyPool skips it for being inactive. The wallet reopens, but none of that descriptor's scripts are registered.

I thought UpdateWalletDescriptor might put the assert back in play since it resets m_max_cached_index, but it also assigns the incoming descriptor over m_wallet_descriptor, so the old range is gone before the top up. Haven't tested recovery on an affected wallet.

Centralizing validation of oversized -keypool values.

There's narrowing before the addition too: TopUpWithDB assigns m_keypool_size to an unsigned int, so -keypool=4294968296 gives a target_size of 1000. The member keeps the full value, so the no-range import path still rejects it with -8 here. A bound of INT32_MAX would be representable and still ask for two billion derivations, so the limit has to be about work, not just the type.

Clarifying whether TopUpWithDB should return failure when the requested endpoint is unrepresentable, instead of leaving the range unchanged and returning true (plus writes the unchanged descriptor).

I'd keep it as is here and look at the return value with its callers separately. keypoolrefill discards the bool, the -4 you saw is GetKeyPoolSize() < kpSize, which sums across active spkms rather than reporting the descriptor that failed. UpdateWithSigningProvider throws std::runtime_error("Could not top up scriptPubKeys") on false and MarkUnusedAddresses logs "Topping up keypool failed (locked wallet)", so changing it means touching those too. The unchanged write is pre-existing, keypoolrefill 1 on a full keypool does the same, though that only says it isn't new, not that true is the right answer.

molnard commented at 7:10 PM on September 7, 2026: none

I agree with these points and think they can be handled in follow-up PRs. They don't block this fix, so this PR can be merged as is.

kriss39 commented at 11:29 AM on September 14, 2026: contributor

tACK def4fb0101

Built and ran both tests. Also tried it by hand: importing [2147483647, 2147483647] into a watch-only wallet is rejected with "End of range is too high", the node stays up and the wallet is still empty after a restart. Without the first two commits the new importdescriptors case takes the node down as expected.

One note on the wallet_keypool.py case: it also passes on master in a non-sanitizer build. The wrapped sum ends up below GetEnd(), so std::max just keeps the old end and keypoolrefill fails with the same -4. So it only catches the overflow under UBSan. Still worth having, maybe with a comment in the test saying that.

Not something this PR needs to solve, but [2147482648, 2147482648] is accepted both with and without this PR (range_end 2147482649 still fits in int32_t), and the top up then derives every index from 0, so the RPC doesn't come back for a very long time. That's the existing O(range_start) cost in TopUpWithDB, #36245 addresses it. The only thing that changes here is that the next_index + target_size addition no longer overflows, so a UBSan build stops aborting on it.

vicjuma commented at 11:45 AM on September 14, 2026: contributor

Concept ACK

Before <img width="2134" height="1218" alt="Image" src="https://github.com/user-attachments/assets/afd72087-d598-421d-8485-e1950fd317aa" />

After

DrahtBot added the label Needs rebase on Sep 17, 2026

polespinasa commented at 6:10 AM on September 17, 2026: member

There is another (unraelated) overflow that this PR could fix, see #34861 (review)

DrahtBot requested review from vicjuma on Sep 18, 2026

wallet: reject unrepresentable descriptor import ranges

WalletDescriptor stores the range in int32_t with an exclusive end,
but ImportDescriptor computes that end in int64_t and passes it straight
to the constructor. An inclusive endpoint of INT32_MAX, which
CheckDescriptorRangeBounds accepts, becomes INT32_MIN when narrowed and
leaves an inverted range that aborts the node in TopUpWithDB. The same
happens with no range given when -keypool is larger than INT32_MAX.

Bound the end after both branches, before passing the range to the
WalletDescriptor constructor. Keep this restriction in the wallet:
an inclusive endpoint of INT32_MAX is valid for the scanning RPCs.

1889440f82

wallet: fix integer overflow in descriptor keypool top up

TopUpWithDB adds next_index and target_size in int32_t. After an address
has been used, keypoolrefill INT32_MAX overflows this sum.

Calculate the sum in int64_t and check that it fits in int32_t before
extending the descriptor range.

867ad9bffb

test: add coverage for descriptor range end bounds

Cover imports whose exclusive range end does not fit in int32_t, both
with an explicit range and with an oversized -keypool.

Also cover keypoolrefill INT32_MAX after advancing every active
descriptor past index zero. This detects the addition overflow under
UBSan and checks that a rejected refill leaves the keypool unchanged.

ab3bb55a67

script: avoid overflow when checking descriptor range bounds

CheckDescriptorRangeBounds overflows low + 1000000 for [INT64_MAX, 0].
Reject reversed ranges first and compare high - low instead. At that
point 0 <= low <= high <= INT32_MAX, so the subtraction is safe.

Add the reversed range to reject_invalid_descriptor_ranges. It fails
with signed integer overflow under UBSan before this change.

Suggested-by: Rob1Ham

457c9e8999

shuv-amp force-pushed on Sep 19, 2026

shuv-amp commented at 3:48 PM on September 19, 2026: contributor

Rebased onto d48e76e689, adapting the import guard to wallet/imports.cpp. Added a separate commit for the range-validation overflow reported by @Rob1Ham and linked by @polespinasa, including the suggested regression test. The new test fails before the fix and passes after it under UBSan. The targeted unit and functional tests passed.

DrahtBot removed the label Needs rebase on Sep 19, 2026

molnard commented at 9:29 PM on September 21, 2026: none

tACK 457c9e8999fbee1d37f41eaede681f787e657395

Tested locally. The code hasn't changed since my previous review, and I still consider it good.

achow101 commented at 12:04 AM on September 26, 2026: member

ACK 457c9e8999fbee1d37f41eaede681f787e657395

polespinasa commented at 4:50 PM on September 28, 2026: member

This is a behaviour change as now descriptors with a previously accepted range will now fail. Probably worth a release note.

in src/wallet/scriptpubkeyman.cpp:1169 in 867ad9bffb

polespinasa commented at 5:14 PM on September 28, 2026:

in 867ad9bffbd3ab26c9899207ddd11380e0c1a1bd wallet: fix integer overflow in descriptor keypool top up

This can now be a db writte no-op as maybe new_range_end did not change. Maybe:

$ git diff
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 5eab5d505e..9e5e6a51de 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -1165,8 +1165,15 @@ bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int siz
         }
         m_max_cached_index++;
     }
-    SetRangeEnd(new_range_end);
-    batch.WriteDescriptor(GetID(), m_wallet_descriptor);
+    if (new_range_end > m_wallet_descriptor.GetEnd()) {
+        SetRangeEnd(new_range_end);
+        if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) {
+            throw std::runtime_error(std::string(__func__) + ": writing descriptor failed");
+        }
+    }
 
     // By this point, the cache size should be the size of the entire range
     assert(m_wallet_descriptor.GetEnd() - 1 == m_max_cached_index);

shuv-amp commented at 7:44 PM on September 29, 2026:

I'd keep the write unconditional. It persists the whole record, not just range_end, and two callers rely on it when the end doesn't move.

With the diff, wallet_migration.py fails in test_basic with 8 descriptors instead of 11: CreateFromMigration uses a keypool size of 0, so the end never moves and this is the only write of the migrated descriptors. MarkUnusedAddresses also relies on it to persist next_index, so when the range already covers next_index + keypool, it's lost on restart and getnewaddress can hand out an already paid address again.

Master writes unconditionally too, and in the overflow case it also keeps GetEnd(), so this PR doesn't add a new no-op write. Skipping the write would need those two callers to persist their own changes, and together with the return value check that I'd do separately.

fanquake added the label Needs release note on Sep 29, 2026

doc: add release note for #35989 2b87954093

shuv-amp commented at 7:49 PM on September 29, 2026: contributor

Added a release note in 2b87954093 as suggested by @polespinasa.

maflcko removed the label Needs release note on Sep 30, 2026