Remove wallet global nTimeFirstKey

[jnewbery]

Taken from #15032 (review):

I think nTimeFirstKey is only read when the wallet is being loaded from file here:

bitcoin/src/wallet/wallet.cpp

Line 4217 in 3b19d8e

if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height)) {

It's set in LoadKeyMetadata(), LoadScriptMetadata(), and GenerateNewKey() (through UpdateTimeFirstKey()) before then, but any time that nTimeFirstKey is set after wallet load is unnecessary, since it doesn't get read again, and isn't persisted to disk. Notably, any rpc calls that import keys don't need to update the nTimeFirstKey.

I think nTimeFirstKey should be removed as a wallet global entirely. I think -rescan as a wallet command-line argument can also be removed since we now have the rescan() rpc method (removing -rescan has been suggested here: #7061 (comment) and here: #13044 (comment)). If we want to retain the ability to rescan from the wallet's birthday (ie the earliest key birthday in the wallet), then there should be a function GetWalletBirthday() to iterate through the wallet keys and return the wallet's birthday instead of having a wallet global.

[jnewbery]

Requesting @jonasschnelli and @ryanofsky input, since they were the last to look at nTimeFirstKey in #9034 and #9108. Also @meshcollider since he touched nTimeFirstKey in #15032.

[promag]

I think nTimeFirstKey is only read when the wallet is being loaded from file

No, UpdateTimeFirstKey reads it to keep track of the minimum.

Edit: ok, there is no use case for the always updated nTimeFirstKey.

there should be a function GetWalletBirthday() to iterate through the wallet keys

Honestly the tradeoff looks worst - keep track of the minimum timestamp vs iterate over all keys..

Edit: if it turns to be used then I think it's preferable to keep track of nTimeFirstKey.

I think -rescan as a wallet command-line argument can also be removed.

👍

[jnewbery]

the tradeoff looks worst - keep track of the minimum timestamp vs iterate over all keys..

I expect the 'iterate over all keys in memory' operation is very short compared to the other events that are happening at wallet load/startup, such as parsing all key-values from the wallet file.

The reason that I prefer this to storing a wallet member variable is that the current nTimeFirstKey is confusing. People think that it needs to be updated when any key is updated in the wallet. I spent quite a long time digging into the use of nTimeFirstKey in my review of #15032, and you believed that it needed to be updated even when reviewing this issue, after I told you that it doesn't need to be updated! The fact that we were both tripped up by this tells me that it's confusing and should be changed 🙂

[promag]

Right, it's not clear. I just think it's preferable constant time getter than a linear search, and in that case it could have better comments explaining it.

[ryanofsky]

I just think it's preferable constant time getter than a linear search

FWIW, I agree with jnewbery here. The nTimeFirstKey logic seems fragile and difficult to understand. I wouldn't think its worth keeping around to avoid a linear search in hypothetical future code, so personally I'd be happy to see a PR removing it.

[maflcko]

Concept ACK. This seems to be triggering a rescan on a fresh wallet, which makes no sense. On an existing wallet, we should have a proper locator, so overwriting the block height of the locator with nTimeFirstKey seems fragile and unnecessary. Question: Would we unnecessarily rescan the whole chain when loading an existing wallet that has one of the key's metadata.nTime == 1? I hope not.

[ryanofsky]

On an existing wallet, we should have a proper locator, so overwriting the block height of the locator with nTimeFirstKey seems fragile and unnecessary

I figure it's supposed to be an optimization in case the locator gets zapped or something. But in the normal case agree it should be unnecessary. I think the proposal here is only to get rid of nTimeFirstKey without changing this behavior, though.

Question: Would we unnecessarily rescan the whole chain when loading an existing wallet that has one of the key's metadata.nTime == 1?

Not unless the locator forked at the beginning of the chain. The findFirstBlockWithTimeAndHeight call treats rescan_height as a minimum block height.

bitcoin/src/wallet/wallet.cpp

Lines 4511 to 4513 in e6e99d4

	if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
	rescan_height = *first_block;
	}

bitcoin/src/interfaces/chain.cpp

Line 96 in e6e99d4

CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);

bitcoin/src/chain.h

Lines 468 to 469 in e6e99d4

	/** Find the earliest block with timestamp equal or greater than the given time and height equal or greater than the given height. */
	CBlockIndex* FindEarliestAtLeast(int64_t nTime, int height) const;