Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Wallet synchronization] Add BlockFilterIterator #12185

Merged

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Jan 4, 2024

This PR attempts to do the following things:

  • Simplify WalletFilterProcessor implementation as it relies on BlockFilterIterator which has the complexity of iterating blocks one by one.
  • On master, there is single FiltersCache but the issue with this cache is that it is shared by all SyncTypes and as such if turbo sync works with height X and non-turbo with height Y, it might require unnecessary database lookups (cache misses).
  • BlockFilterIterator also removes block filters from memory once they are not needed, this should decrease memory use a bit (not peak memory but average memory use).

@kiminuo kiminuo force-pushed the feature/2024-01-04-BlockFilterIterator branch from d7c7703 to 8f6fda3 Compare January 4, 2024 12:42
Comment on lines -149 to -161
if (!FiltersCache.TryGetValue(currentHeight, out var filterToProcess))
{
// We don't have the next filter to process, so fetch another batch of filters from the database.
FiltersCache.Clear();

var filtersBatch = await BitcoinStore.IndexStore.FetchBatchAsync(new Height(currentHeight), MaxNumberFiltersInMemory, cancellationToken).ConfigureAwait(false);
foreach (var filter in filtersBatch)
{
FiltersCache[filter.Header.Height] = filter;
}

filterToProcess = filtersBatch.First();
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got simplified.

@kiminuo kiminuo requested a review from turbolay January 4, 2024 12:44
Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK, I've thought quite deeply about the concept and it's quite wise, many different cases are handled the right way with this.

Code also LGTM, few nits, nothing important

@kiminuo kiminuo marked this pull request as ready for review January 5, 2024 07:47

public interface IIndexStore
{
Task<FilterModel[]> FetchBatchAsync(uint fromHeight, int batchSize, CancellationToken cancellationToken);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw: This should help with moving SQLite dependency to Daemon (#10650)

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a bug while testing, if you change your Height in the wallet file to something lower than the first filter, it will crash. This is not expected because it's easier when wanting a full resync to set the height to 0, and let the software ignores it until the first filter.

I understand that it's not really the responsibility of this PR; but the behavior change reveals this problem, so I think it should be fixed here

@turbolay
Copy link
Collaborator

turbolay commented Jan 5, 2024

diff --git a/WalletWasabi/Wallets/Wallet.cs b/WalletWasabi/Wallets/Wallet.cs
index 6a06b3511..ffd3d49e1 100644
--- a/WalletWasabi/Wallets/Wallet.cs
+++ b/WalletWasabi/Wallets/Wallet.cs
@@ -9,6 +9,8 @@ using System.Threading.Tasks;
 using WalletWasabi.Backend.Models;
 using WalletWasabi.Blockchain.Analysis.Clustering;
 using WalletWasabi.Blockchain.Analysis.FeesEstimation;
+using WalletWasabi.Blockchain.BlockFilters;
+using WalletWasabi.Blockchain.Blocks;
 using WalletWasabi.Blockchain.Keys;
 using WalletWasabi.Blockchain.TransactionOutputs;
 using WalletWasabi.Blockchain.TransactionProcessing;
@@ -473,11 +475,20 @@ public class Wallet : BackgroundService, IWallet
 
                Height bestKeyManagerHeight = KeyManager.GetBestTurboSyncHeight();
 
+               // Make sure best height is at least height of segwit activation.
+               var startingSegwitHeight = new Height(SmartHeader.GetStartingHeader(Network, IndexType.SegwitTaproot).Height);
+               if (startingSegwitHeight > bestKeyManagerHeight)
+               {
+                       KeyManager.SetBestHeights(startingSegwitHeight, startingSegwitHeight);
+                       bestKeyManagerHeight = startingSegwitHeight;
+               }
+
                using (BenchmarkLogger.Measure(LogLevel.Info, "Initial Transaction Processing"))
                {
                        TransactionProcessor.Process(BitcoinStore.TransactionStore.ConfirmedStore.GetTransactions().TakeWhile(x => x.Height <= bestKeyManagerHeight));
                }
 
+
                // Each time a new batch of filters is downloaded, request a synchronization.
                var lastHashesLeft = BitcoinStore.SmartHeaderChain.HashesLeft;
                while (BitcoinStore.SmartHeaderChain.HashesLeft > 0)

@kiminuo
Copy link
Collaborator Author

kiminuo commented Jan 6, 2024

I found a bug while testing, if you change your Height in the wallet file to something lower than the first filter, it will crash. This is not expected because it's easier when wanting a full resync to set the height to 0, and let the software ignores it until the first filter.

I understand that it's not really the responsibility of this PR; but the behavior change reveals this problem, so I think it should be fixed here

Merged master with your PR fixing this

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@lontivero
Copy link
Collaborator

cACK

Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@kiminuo kiminuo merged commit 8f6d485 into WalletWasabi:master Jan 9, 2024
7 checks passed
@kiminuo kiminuo deleted the feature/2024-01-04-BlockFilterIterator branch January 9, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants