Use lazy initialization for scripts in HdPubKey

[turbolay]

Review with this view: https://github.com/zkSNACKs/WalletWasabi/pull/11848/files/f523cb26d875c6666754674c318c0cc6feacbc82
Rest are tests or debug logs

This PR is great for 2 reasons:

1. GetScriptPubKey is a time-consuming operation (25s for all my keys). But we only really need all of them to start coinjoin. So with the Lazy, the place where it's computed is much better, as it's when the client is "idle", starting coinjoining. not during a blocking operation (startup) as it's the case on master. We also access to the value during the Send Workflow: for keys involved in the payment while building and for all keys when broadcasting.

2. If you have 2 wallets: One really big (let's say the one of your business) and one relatively small (let's say your personal one), you will not need to compute these values for the big wallet at all.

On my laptop with my big wallet, it makes Wasabi GUI to show up in 10s rather than 30s
Note that what takes time is NBitcoin.TaprootFullPubKey.Create(TaprootInternalPubKey, uint256), so the more taproot keys you have the longer it takes. If you prefer, I can change this PR so only taproot is lazy, but I see no harm in having all lazy.

This PR might have consequences I didn't account about

[kiminuo]

@lontivero We talked about this change with @turbolay and I proposed to test this Lazy<T> approach to see what would happen (and it seems to have a big impact).

Given our talk about a previous PR of mine, I believe you are not a fan of Lazy<T> stuff. Here, I would say it might make good sense to just compute. Example for P2pkhScript when P2pkhScript property is accessed, i.e.

-public Script P2pkhScript { get; }
+public Script P2pkhScript => PubKey.GetScriptPubKey(ScriptPubKeyType.Legacy);

instead of

-public Script P2pkhScript { get; }
+public Lazy<Script> P2pkhScript { get; }

This is just an example, Clement says that NBitcoin.TaprootFullPubKey.Create(TaprootInternalPubKey, uint256) is the slow one here.

That would be less invasive change, that would still speed up startup but we would avoid Lazy<T> stuff. Or do you have a better idea?

[lontivero]

I'm happy we came back to this old code that I always wanted to change but was too lazy hahaha. Before starting my rant I want to say that I am okay with the concept behind this PR even when it is not as I think it should be solved.

However, the PR is incredibly intrusive, a local change like this one cannot, and should not, require to change 12 files so, please make locals changes local. @kiminuo's proposal goes in the right direction but you can have the best of this PR and Kimi's idea by simply having lazy fields. For example:

// a field for backing the property getter
private Lazy<Script> _p2pkScript ;  

// here in the constructor we can initialize it
_p2pkScript = new Lazy<Script>(() => PubKey.ScriptPubKey);

// and then the property doesn't need to change and no changes in other files are needed.
public Script P2pkScript => _P2pkScript.Value;

[turbolay]

it is not as I think it should be solved.

I'm open to suggestions. This PR was a base to highlight the problem, solution doesn't have to be implemented like this.

the PR is incredibly intrusive

To be honest, I thought I only had to change 3 fields then I remembered.... The tests 😆
PR is updated and ready for review.

[lontivero]

Yes, this is much better and I think the diff in performance is significant.

[kiminuo]

LGTM

I think "Affiliate" label might be needed here. Even though the PR does not change API for 3rd parties, it still changes behavior if, eg, this line

https://github.com/zkSNACKs/WalletWasabi/pull/11848/files#diff-ea808fd44c4592e44076e9318d2ce25c1cb8b49ddfb6f47ddcdb746263c64261R37

throws an exception.

[kiminuo]

One additional improvement would be to specify explicitly isThreadSafe when creating Lazy (new Lazy<>..) (see).

Personally, I would go with isThreadSafe=true.

[lontivero]

it is not as I think it should be solved.

I'm open to suggestions. This PR was a base to highlight the problem, solution doesn't have to be implemented like this.

I think this PR is okay because it gives us an important improvement at a very low cost.

About the solution, I think this a hack because we create properties in advanced which are initialized when necessary instead of creating the thing when necessary. In this case all these properties are used by the ConstainsScript method only (except p2wpkh that is used in tests and payjoin) what makes them clearly unnecessary.

But as I said, I would go with this PR because it brings something good at low cost.

[kiminuo]

About the solution, I think this a hack because we create properties in advanced which are initialized when necessary instead of creating the thing when necessary.

I agree. Refactorings are pretty slow but ongoing. So hopefully, one day in medium near future.

I think this PR is okay because it gives us an important improvement at a very low cost.

I agree.