Skip to content

Commit

Permalink
Fix other warnings from Wallet.cs
Browse files Browse the repository at this point in the history
  • Loading branch information
Turbolay committed Jan 9, 2024
1 parent 9fe6ab4 commit 885374f
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 15 deletions.
2 changes: 1 addition & 1 deletion WalletWasabi.Fluent/Models/Wallets/WalletTransactionsModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public async Task SendAsync(BuildTransactionResult transaction)

private IEnumerable<TransactionModel> BuildSummary()
{
var orderedRawHistoryList = _wallet.BuildHistorySummary(sortForUI: true);
var orderedRawHistoryList = _wallet.BuildHistorySummary(sortForUi: true);
var transactionModels = _treeBuilder.Build(orderedRawHistoryList);
return transactionModels;
}
Expand Down
21 changes: 7 additions & 14 deletions WalletWasabi/Wallets/Wallet.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private set
public PaymentBatch BatchedPayments { get; }

public int AnonScoreTarget => KeyManager.AnonScoreTarget;
public bool ConsolidationMode { get; set; } = false;
public bool ConsolidationMode { get; set; }

public bool IsMixable =>
State == WalletState.Started // Only running wallets
Expand Down Expand Up @@ -164,9 +164,9 @@ public IEnumerable<SmartTransaction> GetTransactions()
/// <summary>
/// Get all wallet transactions along with corresponding amounts ordered by blockchain.
/// </summary>
/// <param name="sortForUI"><c>true</c> to sort by "first seen", "height", and "block index", <c>false</c> to sort by "height", "block index", and "first seen".</param>
/// <param name="sortForUi"><c>true</c> to sort by "first seen", "height", and "block index", <c>false</c> to sort by "height", "block index", and "first seen".</param>
/// <remarks>Transaction amount specifies how it affected your final wallet balance (spend some bitcoin, received some bitcoin, or no change).</remarks>
public List<TransactionSummary> BuildHistorySummary(bool sortForUI = false)
public List<TransactionSummary> BuildHistorySummary(bool sortForUi = false)
{
Dictionary<uint256, TransactionSummary> mapByTxid = new();

Expand Down Expand Up @@ -196,7 +196,7 @@ public List<TransactionSummary> BuildHistorySummary(bool sortForUI = false)
}
}

return sortForUI
return sortForUi
? mapByTxid.Values.OrderBy(x => x.FirstSeen).ThenBy(x => x.Height).ThenBy(x => x.BlockIndex).ToList()
: mapByTxid.Values.OrderByBlockchain().ToList();
}
Expand Down Expand Up @@ -433,11 +433,6 @@ private async void IndexDownloader_NewFiltersAsync(object? sender, IEnumerable<F
NewFiltersProcessed?.Invoke(this, filterModels);
await Task.Delay(100).ConfigureAwait(false);

if (Synchronizer is null || BitcoinStore?.SmartHeaderChain is null)
{
return;
}

// Make sure fully synced and this filter is the latest filter.
if (BitcoinStore.SmartHeaderChain.HashesLeft != 0 || BitcoinStore.SmartHeaderChain.TipHash != filterModels.Last().Header.BlockHash)
{
Expand All @@ -446,10 +441,7 @@ private async void IndexDownloader_NewFiltersAsync(object? sender, IEnumerable<F

var task = BitcoinStore.MempoolService.TryPerformMempoolCleanupAsync(Synchronizer.HttpClientFactory);

if (task is { })
{
await task.ConfigureAwait(false);
}
await task.ConfigureAwait(false);
}
catch (OperationCanceledException)
{
Expand Down Expand Up @@ -486,6 +478,7 @@ private async Task LoadWalletStateAsync(CancellationToken cancel)
KeyManager.SetBestTurboSyncHeight(startingSegwitHeight);
}

// ReSharper disable once ExplicitCallerInfoArgument
using (BenchmarkLogger.Measure(LogLevel.Info, "Initial Transaction Processing"))
{
TransactionProcessor.Process(BitcoinStore.TransactionStore.ConfirmedStore.GetTransactions().TakeWhile(x => x.Height <= bestTurboSyncHeight));
Expand Down Expand Up @@ -535,7 +528,7 @@ private async Task LoadDummyMempoolAsync()
foreach (var tx in BitcoinStore.TransactionStore.MempoolStore.GetTransactions())
{
uint256 hash = tx.GetHash();
if (mempoolHashes.Contains(hash.ToString()[..compactness]))
if (mempoolHashes.Contains(hash.ToString()?[..compactness] ?? string.Empty))

This comment has been minimized.

Sign in to view

Copy link
@kiminuo

kiminuo Jan 9, 2024

I think that the approach should be slightly different: kiminuo@699d4fc and even that hash.ToString() is totally inefficient as it allocates as crazy:

-> I think that there should be a method in NBitcoin for converting uint256 to ReadOnlyMemory<char> which would prevent many and many allocations.

But then I understand that the algo here is not entirely great so maybe it's not worth doing it. But I think it would be a great improvement.

This comment has been minimized.

Sign in to view

Copy link
@kiminuo

kiminuo Jan 9, 2024

This is what I meant MetacoSA/NBitcoin#1198.

This comment has been minimized.

Sign in to view

Copy link
@turbolay

turbolay Jan 9, 2024

Owner

Why not, IDK about merging these kind of changes in NBitcoin, but the suggesed change for Wallet.cs is good I believe, it's elegant and suppresses the warning. Even if if I understand correctly, the behavior is extremely similar to my code.

This comment has been minimized.

Sign in to view

Copy link
@kiminuo

kiminuo Jan 9, 2024

Yes, it's very similar but I attempt to avoid allocations hence the span use. To put it blantly:

  • This x.ToString()[..compactness] allocates during ToString() AND it creates a substring when [..compactness] is evaluated
  • This x.ToString().AsMemory()[..compactness] allocates during ToString() but not when [..compactness] is called
{
txsToProcess.Add(tx);
Logger.LogInfo($"'{WalletName}': Transaction was successfully tested against the backend's mempool hashes: {hash}.");
Expand Down

0 comments on commit 885374f

Please sign in to comment.