TransactionHistoryBuilder: Do not update "FirstSeen" and "Labels"

[kiminuo]

This is another part of #11458¹.

The PR itself is trivial but arguing that it's correct is not trivial. So my argument is:

• TransactionProcessor is the main source of truth for SmartTransactions (references!)² as it keeps CoinsRegistry in an up-to-date state.
  • TransactionProcessor updates SmartTransactions by the code that was removed from TransactionHistoryBuilder (this PR):
    https://github.com/zkSNACKs/WalletWasabi/blob/e7a5e3c4fd8beb19d172d9d0cba3ab11f9c79f3d/WalletWasabi/Blockchain/TransactionProcessing/TransactionProcessor.cs#L106-L120
  • Note that TransactionProcessor is the only class that calls CoinsRegistry.TryAdd(SmartCoin coin).
• TransactionHistoryBuilder calls wallet.GetAllCoins() which operates only on coins from CoinsRegistry.

This should deliver some memory savings for big wallets.

Testing

One way to make sure that references are correct is to do:

foreach (SmartCoin coin in wallet.GetAllCoins())
{
	if (mapByTxid.TryGetValue(coin.TransactionId, out TransactionSummary? found)) // If found then update.
	{
+		#if DEBUG
+		if (!ReferenceEquals(coin.Transaction, found.Transaction))
+		{
+			Logger.LogError($"Transaction reference mismatch for txid '{coin.TransactionId}'!");
+			Environment.Exit(1337);
+		}
+		#endif

		found.Amount += coin.Amount;
	}

Maybe it would be useful to add this piece of code to uncover potential bugs.

Measurements

I simply add the following code:

public static List<TransactionSummary> BuildHistorySummary(Wallet wallet)
{
+    Logger.LogError($"MEMORY_START: {GC.GetTotalMemory(forceFullCollection: true)}");

+    Logger.LogError($"MEMORY_END: {GC.GetTotalMemory(forceFullCollection: true)}");
}

It's not correct way but it gives some estimates at leasts.

master

2023-09-28 15:05:53.592 [7] ERROR       TransactionHistoryBuilder.BuildHistorySummary (16)      MEMORY_START: 271529760
2023-09-28 15:05:53.716 [7] ERROR       TransactionHistoryBuilder.BuildHistorySummary (57)      MEMORY_END: 273296880 (273296880 - 271529760 = 1767120 ~ 1.7 MB)

2023-09-28 15:11:18.561 [9] ERROR       TransactionHistoryBuilder.BuildHistorySummary (16)      MEMORY_START: 272233792
2023-09-28 15:11:18.691 [9] ERROR       TransactionHistoryBuilder.BuildHistorySummary (57)      MEMORY_END: 275360248 (275360248 - 272233792 = 3126456 ~ 3.1 MB)

PR

2023-09-28 14:59:43.587 [17] ERROR      TransactionHistoryBuilder.BuildHistorySummary (15)     MEMORY_START: 317154280
2023-09-28 14:59:43.719 [17] ERROR      TransactionHistoryBuilder.BuildHistorySummary (47)     MEMORY_END: 317292416 (317292416 - 317154280 = 138136 ~ 138 KB)

2023-09-28 15:14:18.600 [33] ERROR      TransactionHistoryBuilder.BuildHistorySummary (15)      MEMORY_START: 272503912
2023-09-28 15:14:18.714 [33] ERROR      TransactionHistoryBuilder.BuildHistorySummary (47)      MEMORY_END: 273323352 (273323352 - 272503912 = 819440 ~ 819 kB)

Footnotes

1. In that PR, TransactionWithAmount was introduced but with this PR, TransactionSummary has basically become TransactionWithAmount. ↩

2. Because we read all transactions when WW starts and we pass them to TransactionProcessor. Similarly for mempool transactions that we receive, etc. ↩

[kiminuo]

@soosr Is there any concern from your side? I'm asking mostly just to double check.

[soosr]

@soosr Is there any concern from your side?

I don't think so

[adamPetho]

LGTM