I'm pretty sure that this is actually incorrect.

internal and keypool.fInternal will not always be the same. For wallets before HD chain split, internal=true will still result in keypool.fInternal=false. Later, ReserveDestination.fInternal is used by ReturnDestination in order to put the kepool data back into the correct keypool set, and the correct set is determined by the value of ReserveDestionation.fInternal.

With a pre-split wallet or wallets with a pre-split keypool, with this change we will end up placing returned destinations to the internal keypool where they should go into the external keypool or the pre-split keypool. Since we decide which set to fetch new keypool keys from by using the feature flags, for such wallets we will not choose the internal keypool as the flags indicate that is not supported. Thus with such wallets, we are just throwing away change addresses rather than correctly returning them to the keypool.

hmm true, good catch. The code is quite entangled there.

You know, the legacy version of ReturnDestination should be checking whether the wallet is pre-split or not to return the key to the proper place as ReserveKeyFromKeyPool is doing.. and not just place it into the internal pool directly if the internal flag is true.

Another thing that looks like an existent bug is what happens when the wallet takes the last key from the pre-split keypool:

1. ReserveKeyFromKeyPool will erase the last key index from set_pre_split_keypool. Leaving the set empty.
2. As set_pre_split_keypool is empty, ReturnDestination will not return the index to set_pre_split_keypool and wrongly push it inside setExternalKeyPool (because before return the index into the pre-split set we are checking that the set is not empty).