Remove wallet global nTimeFirstKey

jnewbery commented at 9:47 pm on January 31, 2019: member

Taken from #15032#pullrequestreview-198677243:

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

https://github.com/bitcoin/bitcoin/blob/3b19d8e341a5234c3e41f59f7b3de8febfc51c21/src/wallet/wallet.cpp#L4217

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 commented at 9:49 pm on January 31, 2019: member

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.

jonasschnelli added the label Wallet on Feb 2, 2019

promag commented at 7:34 pm on February 4, 2019: member

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 commented at 4:10 pm on February 5, 2019: member

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 :slightly_smiling_face:

promag commented at 4:06 pm on February 15, 2019: member

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 commented at 9:19 pm on July 17, 2019: member

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.

MarcoFalke commented at 8:10 pm on July 23, 2019: member

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 commented at 8:31 pm on July 23, 2019: member

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.

https://github.com/bitcoin/bitcoin/blob/e6e99d4f757f2e5052f0cc68951c75e91e4753e3/src/wallet/wallet.cpp#L4511-L4513

https://github.com/bitcoin/bitcoin/blob/e6e99d4f757f2e5052f0cc68951c75e91e4753e3/src/interfaces/chain.cpp#L96

https://github.com/bitcoin/bitcoin/blob/e6e99d4f757f2e5052f0cc68951c75e91e4753e3/src/chain.h#L468-L469

jnewbery closed this on Jun 20, 2022

DrahtBot locked this on Jun 20, 2023

Remove wallet global nTimeFirstKey #15306