Remove brain farts

[lontivero]

This PR is for simplifying the HdPubKeyCache that contains unnecessary complexities that make it slow (the idea behind whatever is named blahblahCache is to make things faster.

The PR contains many things, changes and improvements all across the key management code because I think those are changes that we need to do anyway but by sure not all of them in the same PR.

One clear improvement in term of performance is in the HdPubKeyCache::GetView (a method that is called millions of times).

master (results in microseconds)

| Method  | WalletSize | Mean         | Error     | StdDev    |
|-------- |----------- |-------------:|----------:|----------:|
| GetView | 100        |     9.994 us | 0.0770 us | 0.0643 us |
| GetView | 1000       |    99.277 us | 1.2050 us | 1.0682 us |
| GetView | 10000      | 1,248.137 us | 5.1878 us | 4.5989 us |

PR (result in nanoseconds 1us = 1000 ns)

| Method  | WalletSize | Mean      | Error    | StdDev   | Median    |
|-------- |----------- |----------:|---------:|---------:|----------:|
| GetView | 100        |  92.55 ns | 1.907 ns | 4.495 ns |  91.07 ns |
| GetView | 1000       |  92.21 ns | 1.218 ns | 1.139 ns |  91.91 ns |
| GetView | 10000      | 108.51 ns | 1.284 ns | 1.201 ns | 107.79 ns |

About the new brain fart involving ArraySegment, that's there to avoid changing behavior. I mean, I think it is absolutely unnecessary but it is to avoid thinking and testing.

[turbolay]

To help me reviewing, can you explain why a version using only a Dictionary in the HdPubKeyCache is not good, or can change behavior? turbolay@b75679d

[lontivero]

Using only a dictionary is possible but the assumption in the code is that the HdPubKeyCache always gives you an IEnumerable over a collection that never changes, in master this is achieved by returning a clone converted to an ImmutableList while the version in this PR achieves the same by using an ArraySegment over an array that only adds up so, the question is: how do you achieve the same using a dictionary?

[turbolay]

Thank you this is exactly the answer I needed. I will review soon

[kiminuo]

It would be great to have at least a few unit tests for this class. There is no HdPubKeyCacheTests file.

There we could verify the test scenario:

1. Add 750 keys
2. Add another 750 keys
3. Verify that HdPubKeyCache.Snapshot for both previous states return the same first 750 keys (HdPubKeyInfo defines the == operator so it should be trivial)

This should work because ArraySegment holds reference to _hdPubKeyInfos.

[lontivero]

Whatever we modify in the code has real consequences observable from the external world, otherwise why would someone modify code? This means that this should be observable through the KeyManager. You should be able to request the keys, generate a new key and verify that the enumeration that you received before adding the key doesn't contain it.

[kiminuo]

Whatever we modify in the code has real consequences observable from the external world, otherwise why would someone modify code?

Lucas, you are good at arguing almost anything but we write tests¹ for good reasons.

So would a test like this one https://github.com/kiminuo/WalletWasabi/tree/simplify-hdpkcache (kiminuo@6271bbf) hurt?

Personally, I would be also interested in

/// <summary>
/// Tests adding the same key twice. 
/// </summary>
[Fact]
public void SameKeyTwiceTest()
{
	KeyManager keyManager = ServiceFactory.CreateKeyManager(password: "password");
	HdPubKey NewKey(string label) => keyManager.GenerateNewKey(label, KeyState.Used, isInternal: true);
	HdPubKeyCache cache = new();

	HdPubKey key = NewKey($"Key#1");
	cache.AddKey(key, ScriptPubKeyType.Segwit);
	cache.AddKey(key, ScriptPubKeyType.Segwit);
}

if this should throw an exception or not. I understand that it should not ever happen but in your implementation there is

_hdPubKeyIndexedByScriptPubKey[info.ScriptPubKey] = info.HdPubKey;

which does not throw.

Footnotes

1. Not as much as I would like lately but anyway. ↩

[lontivero]

Sure. You are testing exactly as I suggested (testing the KeyManager). So, yes, except for the code added for testing only, which is not necessary.

The SameKeyTwiceTest is not good because it assumes the cache must fail in case of duplicated keys but that is not a responsibility of the cache. In case the cache receives a duplicated key that means that we have a bug in the KeyManager and we need to fix it.

[kiminuo]

The SameKeyTwiceTest is not good because it assumes the cache must fail in case of duplicated keys but that is not a responsibility of the cache. In case the cache receives a duplicated key that means that we have a bug in the KeyManager and we need to fix it.

Well, and that's precisely what I'm after. The test would make it clear that we don't expect to get the same key twice. I'm not sure why it's not responsibility of cache to throw an exception in such case because it looks to me that in this scenario the internal state of cache would get broken.

However, I guess you don't watch such test. Anyway, my goal here is basically to document expectations. But we don't document code mostly so the alternative is to write a test but if even test is not possible then ...

[kiminuo]

Looks like a great improvement to me.

[turbolay]

LGTM, and I tested and didn't find problems.
My 2 suggestions are just for cf

[molnard]

Is there a specific reason to break the code convention of "everything should be a property"?

[molnard]

Just extKeys?

[molnard]

Delete the file.

[molnard]

I loaded an old wallet and I got the following exc.

System.NullReferenceException
  HResult=0x80004003
  Message=Object reference not set to an instance of an object.
  Source=WalletWasabi
  StackTrace:
   at WalletWasabi.Blockchain.Keys.HdPubKeyCache.<>c.<get_HdPubKeys>b__6_0(HdPubKeyInfo x) in WalletWasabi\Blockchain\Keys\HdPubKeyCache.cs:line 19