wallet: Remove redundant wpkh script import

[qubicorn]

• CWallet::ImportPrivKeys() in previous code block already imports the
  wpkh script. The block being removed effectively just printed a false
  positive warning. Fixes importprivkey always gives warning about already having script  #16711

[qubicorn]

feature_fee_estimation.py passes locally, and doesn't seem related.

[laanwj]

Concept ACK

CWallet::ImportPrivKeys() in previous code block already imports the
wpkh script

Do you know where this happens? That function doesn't seem to do it directly, at least.

[qubicorn]

It's done by FillableSigningProvider::ImplicitlyLearnRelatedKeyScripts():

bitcoin/src/script/signingprovider.cpp

Lines 87 to 92 in adff8fe

	if (pubkey.IsCompressed()) {
	CScript script = GetScriptForDestination(WitnessV0KeyHash(key_id));
	// This does not use AddCScript, as it may be overridden.
	CScriptID id(script);
	mapScripts[id] = std::move(script);
	}

Here's a stack trace showing how it's called by CWallet::ImportPrivKeys():

(gdb) bt
#0  FillableSigningProvider::ImplicitlyLearnRelatedKeyScripts (this=0x555556634590, pubkey=...) at script/signingprovider.cpp:73
#1  0x0000555555c4319f in FillableSigningProvider::AddKeyPubKey (this=0x555556634590, key=..., pubkey=...) at script/signingprovider.cpp:109
#2  0x0000555555ae8454 in CWallet::AddKeyPubKeyInner (this=0x555556634590, key=..., pubkey=...) at wallet/wallet.cpp:4913
#3  0x0000555555ac5ba4 in CWallet::AddKeyPubKeyWithDB (this=0x555556634590, batch=..., secret=..., pubkey=...) at wallet/wallet.cpp:370
#4  0x0000555555acfb9c in CWallet::ImportPrivKeys (this=0x555556634590, privkey_map=std::map with 1 element = {...}, timestamp=1)
    at wallet/wallet.cpp:1820
#5  0x0000555555b80e99 in importprivkey (request=...) at wallet/rpcdump.cpp:189

[laanwj]

Thanks! That's pretty sneaky, I was indeed searching for AddCScript or something similar, not something that bypasses everything and adds directly to the map.

Looks good to me then. Code review ACK b6e7aa8

[qubicorn]

It is sneaky. The author of these lines obviously didn't see it either. Thanks for the review!

[meshcollider]

Concept ACK, the fix makes sense, haven't checked that the behaviour of ImplicitlyLearnRelatedKeyScripts behaves in the same way as ImportScripts though (with respect to writing to the wallet).

I think this is fine?

[achow101]

This will prevent the segwit output script from being written to the wallet file. While ImplicitlyLoadRelatedScripts allowed for those same scripts to be loaded, if the wallet were to be loaded into an older version of Bitcoin Core (without segwit I think, don't remember when this was added, but probably with segwit), outputs involving such scripts will not be recognized as they won't be loaded into the wallet in any way.

[qubicorn]

@achow101 That's a good point, I think you're right.

Here's another idea. The original code, for reference:

bitcoin/src/wallet/rpcdump.cpp

Lines 188 to 196 in f5db3f2

	// Use timestamp of 1 to scan the whole chain
	if (!pwallet->ImportPrivKeys({{vchAddress, key}}, 1)) {
	throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
	}
	// Add the wpkh script for this key if possible
	if (pubkey.IsCompressed()) {
	pwallet->ImportScripts({GetScriptForDestination(WitnessV0KeyHash(vchAddress))}, 0 /* timestamp */);
	}

If we switch the order of these two blocks, like so:

// Add the wpkh script for this key if possible 
if (pubkey.IsCompressed()) { 
    pwallet->ImportScripts({GetScriptForDestination(WitnessV0KeyHash(vchAddress))}, 0 /* timestamp */); 
} 

// Use timestamp of 1 to scan the whole chain 
if (!pwallet->ImportPrivKeys({{vchAddress, key}}, 1)) {
    throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet"); 
} 

the warning referenced in #16711 goes away, because ImplicitlyLoadRelatedScripts doesn't show a warning when the key already exists. Yes it's being written to the map twice in quick succession, but that shouldn't be an issue.

[qubicorn]

Actually, the above would work, but this highlights a bigger problem. ImportScripts() relies on FillableSigningProvider::HaveKey() to determine whether to write the script to the wallet or not. However, as @achow101 just noted, a script appearing in the map doesn't mean it's written to the wallet. Thus, ImplicitlyLoadRelatedScripts can prevent scripts from being written.

bitcoin/src/wallet/wallet.cpp

Lines 1778 to 1781 in e9ef1b2

	if (HaveCScript(id)) {
	WalletLogPrintf("Already have script %s, skipping\n", HexStr(entry));
	continue;
	}

So perhaps instead, the conditional statement above could be changed to read from the actual wallet to determine if a script exists.

[fanquake]

@achow101 @meshcollider did you want to follow up with approach thoughts here?

[meshcollider]

IMO places where the map is edited directly without writing (other than the initial read of the wallet) are going to be dangerous, that behavior should be changed instead of adding more checks that the script is in the wallet. I think we should make it not possible to add to the map without adding to the wallet file.

EDIT: Thinking about this more, I think it is okay to just make ImportScripts check the database rather than just the map. Just make sure FillableSigningProvider doesn't have to worry about the database. Approach ACK 👍

This is actually a bugfix

[instagibbs]

Are there any legit reasons to not even try to write to DB?

[qubicorn]

I'm now leaning towards a write to the wallet after ImplicitlyLoadRelatedScripts(). It's not safe to have addresses in the map that are never written to the wallet. Changing HaveKeys() to check from the wallet directly instead of the map defeats the purpose of the map and ImplicitlyLoadRelatedScripts. After all, what's the point of implicitly loading anything if it's going to be ignored?

My initial understanding of the map is that it's just a cache to avoid repeated disk reads. Is that accurate? If that's true, 1) it should always write through immediately and 2) reads should almost always be done from the map (other than initially populating the map).

Thanks for your patience, everyone! I'm still wrapping my head around the codebase.

[qubicorn]

To reiterate, there is a more significant bug here than the superfluous warning this was initially submitted for:

ImplicitlyLoadRelatedScripts() is adding scripts to the map but not the wallet. HaveKeys() (which works by checking only the map) is used to determine whether a key should be added to the wallet. So scripts that have been implicitly loaded can never be written to the wallet, even if done later via AddCScript().

[achow101]

I think it is okay to just make ImportScripts check the database rather than just the map.

I'm not really a fan of that. It's going to be a ton of disk reads and slow down large imports even further (we recently improved it with batched writes).

---

To reiterate, there is a more significant bug here than the superfluous warning this was initially submitted for:

ImplicitlyLoadRelatedScripts() is adding scripts to the map but not the wallet. HaveKeys() (which works by checking only the map) is used to determine whether a key should be added to the wallet. So scripts that have been implicitly loaded can never be written to the wallet, even if done later via AddCScript().

I'm not sure that that's necessarily a bug. IIRC, the point of ImplicitlyLoadRelatedScripts() is to let keys in the keypool be detected as used when their p2sh-p2wpkh were used. This would really only effect backups. Those scripts are later written to the wallet to allow for downgrading to non-segwit versions without losing the ability to spend used coins.

Perhaps @sipa can provide some more insights about that as he wrote the code.

---

If the intention of writing the scripts to the wallet was just to allow downgrading, we can actually just check the wallet version number to decide whether to write or not as recent wallet versions are too new to be downgraded anyways.

[qubicorn]

Those scripts are later written to the wallet to allow for downgrading to non-segwit versions without losing the ability to spend used coins.

Scripts in the map are written to the wallet somewhere?

If the intention of writing the scripts to the wallet was just to allow downgrading, we can actually just check the wallet version number to decide whether to write or not as recent wallet versions are too new to be downgraded anyways.

Yes, I was trying to account for your earlier comment that loading the wallet into older versions of bitcoin core would not carry the segwit scripts loaded implicitly with it.

[achow101]

Scripts in the map are written to the wallet somewhere?

Keys in the keypool don't have their scripts written to the wallet. Instead those scripts are loaded into memory via ImplicitlyLearnRelatedScripts. However once a key leaves the keypool, it's scripts are written to the wallet.