-upgradewallet from pre-HD Split not working as expected

[instagibbs]

I expected the internal keypool to be filled, and for change addresses to immediately be drawn from that pool. Looking for replication/explanation in case I missed something. Just tested this on master.

1. Create wallet using 0.14.3(or anything pre-split, maybe?)
2. Stop
3. Load wallet using master with -upgradewallet
4. Query wallet for details

getwalletinfo
{
  "walletname": "test.dat",
  "walletversion": 169900,
  "balance": 0.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 0,
  "keypoololdest": 1560344352,
  "keypoolsize": 2000,
  "keypoolsize_hd_internal": 0,
  "paytxfee": 0.00000000,
  "hdseedid": "a9e2788e2871531d9b54b1fc85ff822dffcc2707",
  "private_keys_enabled": true,
  "scanning": false
}

As you can see, there are 2000 keys now, and knowledge that internal exists, but it's empty. I ask for a change key:

getrawchangeaddress
2N1ZX1ZLgRABqcec4XDVMJmMFRNqnJZBfpA

which I save for later, and check keypools again:

getwalletinfo
{
  "walletname": "test.dat",
  "walletversion": 169900,
  "balance": 0.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 0,
  "keypoololdest": 1560344352,
  "keypoolsize": 1999,
  "keypoolsize_hd_internal": 1,
  "paytxfee": 0.00000000,
  "hdseedid": "a9e2788e2871531d9b54b1fc85ff822dffcc2707",
  "private_keys_enabled": true,
  "scanning": false
}

The internal keypool grew by one, and external shrunk by 1.

getaddressinfo 2N1ZX1ZLgRABqcec4XDVMJmMFRNqnJZBfpA
{
  "address": "2N1ZX1ZLgRABqcec4XDVMJmMFRNqnJZBfpA",
  "scriptPubKey": "a9145b36b3eb5ae046fcaae77a25d6cd93155f33c9ba87",
  "ismine": true,
  "solvable": true,
  "desc": "sh(wpkh([510ad933/0'/0'/1']022a1576b06ca44fbfd3956afe0b5a4933fbeda8a66c308de8074b7b186fa5b4b8))#936fhdgy",
  "iswatchonly": false,
  "isscript": true,
  "iswitness": false,
  "script": "witness_v0_keyhash",
  "hex": "001458aed9491c976d7bce0d0323ff8d1bc139a338bb",
  "pubkey": "022a1576b06ca44fbfd3956afe0b5a4933fbeda8a66c308de8074b7b186fa5b4b8",
  "embedded": {
    "isscript": false,
    "iswitness": true,
    "witness_version": 0,
    "witness_program": "58aed9491c976d7bce0d0323ff8d1bc139a338bb",
    "pubkey": "022a1576b06ca44fbfd3956afe0b5a4933fbeda8a66c308de8074b7b186fa5b4b8",
    "address": "bcrt1qtzhdjjgujakhhnsdqv3llrgmcyu6xw9mvwxxwh",
    "scriptPubKey": "001458aed9491c976d7bce0d0323ff8d1bc139a338bb"
  },
  "ischange": true,
  "timestamp": 1560344273,
  "hdkeypath": "m/0'/0'/1'",
  "hdseedid": "a9e2788e2871531d9b54b1fc85ff822dffcc2707",
  "hdmasterfingerprint": "510ad933",
  "labels": [
  ]
}

That returned key appears to be an external key based on keypath, though it is marked as ischange.

Grabbing more keys seems to continue this pattern:

getwalletinfo
{
  "walletname": "test.dat",
  "walletversion": 169900,
  "balance": 0.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 0,
  "keypoololdest": 1560344352,
  "keypoolsize": 1998,
  "keypoolsize_hd_internal": 2,
  "paytxfee": 0.00000000,
  "hdseedid": "a9e2788e2871531d9b54b1fc85ff822dffcc2707",
  "private_keys_enabled": true,
  "scanning": false
}

[instagibbs]

Hmm, after digging, it looks like it intentionally uses the pre-split pool first before returning internal keypool keys. Non-obvious. A test case demonstrating this behavior would be helpful.

[maflcko]

Is this related to any of these?

• ischange / listsinceblock/listtransactions bug ischange / listsinceblock/listtransactions bug #16160

[instagibbs]

I think those versions listed are all post-split, is ischange is detected purely based on lack of address stored in wallet, not due to where it came from in keypool or hd derivation.

[achow101]

This is intended behavior. The pre-split keypool needs to be consumed before hd keys are derived.

As for a test case, we can only do that once we get old wallet versions in the test suite.

[instagibbs]

What's the blocker on that?

[instagibbs]

I read some background on what it would take. Closing this for now, but I'd really like a comment-based explanation of this logic. It really makes little sense to me.

[darosior]

Maybe related #16091