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 the descriptor range in int32_t fields, with range_end exclusive. ProcessDescriptorImport computes that end in int64_t and passes it to the constructor with no bound in between. ParseDescriptorRange accepts an inclusive endpoint of 2^31 - 1, so the exclusive end can be 2^31. That does not fit in int32_t and truncates to INT32_MIN, which leaves the descriptor with an inverted range. The node then aborts while filling the keypool:

$ bitcoin-cli -named createwallet wallet_name=w disable_private_keys=true
$ bitcoin-cli -rpcwallet=w importdescriptors '[{"desc":"wpkh([728986fc/84h/1h/0h]tpubDDLEJ5Q3Tu8hn4BBEn5bnd2cYecWFwPpse1o9rx8uds47cP2bfr62sZM9LaYuNYPGnWmSKmiPeXinV2YxjHC4kFf3UUShMziLazstZbGRXU/0/*)#0gv2ddmk","range":[2147483647,2147483647],"timestamp":"now"}]'
error: Error while attempting to communicate with server 127.0.0.1:8332 (EOF)

Assertion failed: (m_wallet_descriptor.range_end - 1 == m_max_cached_index),
function TopUpWithDB, file scriptpubkeyman.cpp, line 1109.

The same crash happens with no range argument at all when -keypool is set above INT32_MAX, because that branch takes the end from m_keypool_size, which is only bounded from below (wallet.cpp:3092). Guarding the range argument alone is not enough.

By the time the assert fires the descriptor has already been written to disk. CreateFromImport calls TopUpWithDB directly and skips the transaction wrapper that TopUp has, so the record survives the abort. The wallet still loads afterwards, but listdescriptors reports the inverted range:

"range": [2147483647, -2147482650]

The first commit bounds the end after both branches, so every field that reaches the WalletDescriptor constructor is representable. I did not put the check in ParseDescriptorRange. #35872 made an endpoint of 2^31 - 1 valid for the scanning RPCs and added a test asserting it succeeds, so tightening the shared parser would regress that. This limit comes from how the wallet stores the range, not from descriptor ranges in general.

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
ACK	molnard
Concept ACK	jeanpablojp

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:

#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#34861 (wallet: Add importdescriptors interface by polespinasa)

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

wallet: reject unrepresentable descriptor import ranges

`WalletDescriptor` stores the range in `int32_t` with an exclusive end,
but `ProcessDescriptorImport` computes that end in `int64_t` and passes
it straight to the constructor. An inclusive endpoint of `INT32_MAX`,
which `ParseDescriptorRange` accepts, truncates to `INT32_MIN` 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. Every field that reaches the
`WalletDescriptor` constructor is then representable.
`ParseDescriptorRange` is left alone since an endpoint of `INT32_MAX` is
valid for the scanning RPCs.

e65a151678

wallet: fix integer overflow in descriptor keypool top up

`TopUpWithDB` adds `target_size` to `next_index` in `int32_t`.
`next_index` is user controlled for imported descriptors and
`target_size` comes from `-keypool` or the `keypoolrefill` argument, so
the two can add up to more than `INT32_MAX`:

    wallet/scriptpubkeyman.cpp: runtime error: signed integer overflow:
    1 + 2147483647 cannot be represented in type 'int32_t' (aka 'int')

Compute the end in `int64_t` and leave the range alone when it does not
fit in the `int32_t` the range end is stored in. The wrapped sum was
negative, so `std::max()` discarded it anyway and the resulting range is
unchanged. Only the undefined behavior goes away.

5a8d4749ef

test: add coverage for descriptor range end bounds

Each case fails without the previous commits.

wallet_importdescriptors.py covers the two import paths that abort the
node: a range ending at `INT32_MAX`, and an import with no range under a
`-keypool` larger than `INT32_MAX`. The first uses an xpub because the
watch-only wallet rejects the xpriv the surrounding cases use, before
the descriptor is added at all.

wallet_keypool.py covers the overflow that `keypoolrefill` reaches
without any import. It needs every active descriptor to have advanced
past index 0 first, since at index 0 the sum is exactly `INT32_MAX`.
That case is only visible under UBSan, where the node aborts on the
addition.

def4fb0101

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

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.