[Wallet synchronization] Introduce `BlockDownloadingService` #12184

kiminuo · 2024-01-04T12:25:51Z

Depends on #12236
Part of #10014

The PR adds BlockDownloadService (BDS) that

can download blocks that queues blocks and downloads them in parallel,
BDS is aware of block repository file-system cache so it fetches requested blocks from there if possible.

note: BDS is intended to power #12148 (or any other approach).

WalletWasabi.Tests/UnitTests/Transactions/TransactionProcessorTests.cs

WalletWasabi/Wallets/FilterProcessor/ParallelBlockDownloadService.cs

molnard

Abstraction LGTM - before this gets to a real PR I would like to see how this will fit into the final goal. Maybe merging back into turbolay's PR if that makes sense.

…2024-01-04-ParallelBlockDownloadingService

turbolay

Huge cACK, really good code.
I didn't find problem with the logic, I can see that usage will be great.
Sorry for taking long time to review!

WalletWasabi/Wallets/FilterProcessor/ParallelBlockDownloadService.cs

turbolay · 2024-01-08T18:20:35Z

WalletWasabi/Wallets/FilterProcessor/ParallelBlockDownloadService.cs

+	/// Guarded by <see cref="Lock"/>.
+	/// <para>Internal for testing purposes.</para>
+	/// </remarks>
+	internal PriorityQueue<Request, uint> BlocksToDownload { get; } = new(Comparer<uint>.Default); // TODO: Turbo requests should have precedence over non-turbo.


TODO: Turbo requests should have precedence over non-turbo.

Given how your queue works, this is required, otherwise you might push Turbo all the time, because when we are in this case it means that the wallet is opened and we are doing the second synchronization in the background. You can use the priority used in WalletFilterProcessor, which should be almost the same

WalletWasabi/Wallets/FilterProcessor/ParallelBlockDownloadService.cs

turbolay · 2024-01-09T04:25:03Z

WalletWasabi/Wallets/FilterProcessor/ParallelBlockDownloadService.cs

+				{
+					if (result.Attempts >= MaxFailedAttempts)
+					{
+						Logger.LogInfo($"Attempt to download block {result.BlockHash} (height: {result.BlockHeight}) failed {MaxFailedAttempts} times. Dropping the request.");


Even if it makes sense to have this retry mechanism, which as stated in my previous comment, I'm not sure. Is failure to get a block really a LogInfo? In the usage of this code, what would really happen if we fail to get a block here? Wallet state broken? Or do you defer the responsibility of throwing/logging error to the consumer component?

LogInfo is probably a bit too much I just find it useful for debugging (no need to change log level) but yes, you are right.

So currently the idea is that PBDS is like a prefetching service and does that on best effort basis. Meaning, it would not be an issue not to download a block. But ideas here are welcome - nothing is set to stone at this point.

Wallet state broken?

No. I don't think that this should happen.

Or do you defer the responsibility of throwing/logging error to the consumer component?

Yes. As of now. That's the idea. But it's a subject to change based on new ideas. So yes but can change if there is a better way.

…gService

turbolay · 2024-01-10T01:26:34Z

I made turbolay@3a60c17

It's a merge with my PoC PR with PreProcessing support, removing the attempts, using the correct Priority in the PriorityQueue and general improvements. I commented very well, maybe too much but I wanted to be sure the logic will be understood

The main changes are in files BlockDownloadService.cs, WalletFilterProcessor.cs. Rest is mainly integration, and in the BlockDownloadServiceTests.cs I removed the test related to the attempts.

It's untested and in general has to be improved (it doesn't handle reorgs well, misses cancellation tokens, some locks, P2P nodes management etc...) but the logic should be correct. I really like the code btw I think it's clear and quite good. I think it could be a good base to finish this PR and continue working on the feature.

…gService

…gService # Conflicts: # WalletWasabi/Wallets/FilterProcessor/Priority.cs

…2024-01-04-ParallelBlockDownloadingService

…ory-improvements' into feature/2024-01-04-ParallelBlockDownloadingService

kiminuo · 2024-01-23T20:55:54Z

@turbolay I removed MaxAttempts.

During this work I discovered quite an ugly case when Task<RequestResponse> task = await Task.WhenAny(activeTasks).ConfigureAwait(false); in BDS.ExecuteAsync does not take into account new requests (so if it is waiting for 1 task to finish, it does not take into account that there might new jobs to do that can be started). That's an imperfection that is probably still OK in the real world, but it's really a nuisance for testing. I have been trying to fix it for quite some time but I'm not happy with any solution I found so far... so hopefully I'll get an idea later on.

kiminuo · 2024-01-24T10:11:21Z

@turbolay Added NoSuchProviderResult so that https://github.com/kiminuo/WalletWasabi/pull/1/files#r1463832936 if there is no P2P provider, then GetBlockNoCacheAsync can escape from while (true).

kiminuo · 2024-01-24T10:27:00Z

@turbolay Would it make sense to merge this PR even without your integration because then:

Do not disconnect nodes after a false-positive filter in Download blocks faster and more private #10010 (comment) can be implemented and as such we disconnect less...?
SmartBlockProvider could be removed and in the process we would make sure that the BDS's API is good enough.

WDYT?

turbolay · 2024-01-24T13:44:15Z

@kiminuo My bad, I forgot you were not in the meetings and I had to provide you more context. I was asked to do this:

Maybe we could make a full PoC integration prior to merging?

So problems with the API could be caught beforehand. This is why I created kiminuo#1, it was not meant to be merged at the same time.

Can you fix the test?

kiminuo · 2024-01-24T14:42:02Z

Can you fix the test?

Fixed

kiminuo · 2024-01-25T07:50:19Z

45be3e3 fixes #12184 (comment). At least I hope so. I modified the test to the state that previously was causing issues so it looks OK now.

…2024-01-04-ParallelBlockDownloadingService

turbolay

tACK, code LGTM
Good job with the PR, but especially with 45be3e3

kiminuo · 2024-01-26T14:08:05Z

@turbolay

btw: I tried that integration test here and it finished in

(something like) 1.5 second with BDS [maxParallelTasks=5]
(something like) 6.5 seconds with BDS [maxParallelTasks=1]

It was downloaded from a single peer but that in my mind make it even more impressive because my expectation was that the improvement would come from downloading in parallel from multiple P2P nodes.

(Obviously, not statistically correct measurement... but anyway)

molnard

cACK. The diff is huge and introduces many changes. I am not able to review it thoroughly. I am open to suggestions, on what next.

molnard · 2024-01-29T15:53:16Z

WalletWasabi/Wallets/FileSystemBlockRepository.cs

-	/// <param name="cancellationToken">The cancellation token.</param>
-	public async Task RemoveAsync(uint256 hash, CancellationToken cancellationToken)
+	/// <inheritdoc/>
+	public async Task RemoveAsync(uint256 blockHash, CancellationToken cancellationToken)


Please try to avoid increasing the diff with renamings in the future. If you want to do it, create a separate PR just for this change. 🙏

molnard · 2024-01-29T16:11:33Z

WalletWasabi/Wallets/P2PNodesManager.cs

Why was any of the changes in this file was necessary?

Why did you change the order of the usings?

We want more control over the nodes, to move the logic of disconnection out of this class etc. In general, I believe that this file might drastically change or disappear. Making ConnectedNodes.Count accessible outside of the class is a first step to decide which nodes to disconnect

The order of the using was incorrect, CF requests usings to be alphabetical order.

Just one line: public uint ConnectedNodesCount => (uint)Nodes.ConnectedNodes.Count;

I see no CF issue generated by the order of the using. #12327

I suggested to do it: #12184 (comment)
If you are not happy with this change can you please revert df095cc

https://documentation.help/StyleCop/SA1210.html
It's off for us, that's why CF didn't raise issue with it. His IDE might be configure to change that automatically (mine is).
Please revert

molnard · 2024-01-29T16:15:18Z

WalletWasabi/Wallets/FilterProcessor/BlockDownloadService.cs

+
+		if (response.Result is SuccessResult)
+		{
+			_ = request.Tcs.TrySetResult(response.Result);


Suggested change

_ = request.Tcs.TrySetResult(response.Result);

request.Tcs.TrySetResult(response.Result);

Why? _ = is good to emphasize that we are not using the result. It's a global standard, and we are using this all throughout codebase, even in recent PRs.

No this is something new.
NACK. It looks annoying. Do this only to suppress when you have warnings to use the return value. Please revert.

Please revert yourself, I don't have the rights and Kimi is not here.

BTW this form is a standard in C# and is used more or less 30 times in the codebase, excluding tests, I don't really understand why the fight for it now.

You even used it yourself recently:

https://github.com/zkSNACKs/WalletWasabi/blob/6a2de0aca907d54a64bef6a8d202e4ccc1f78b97/WalletWasabi/Wallets/WalletFilterProcessor.cs#L195

molnard · 2024-01-29T16:15:35Z

WalletWasabi/Wallets/FilterProcessor/BlockDownloadService.cs

+			{
+				while (BlocksToDownload.TryDequeue(out Request? request, out _))
+				{
+					_ = request.Tcs.TrySetResult(CanceledResult.Instance);


Suggested change

_ = request.Tcs.TrySetResult(CanceledResult.Instance);

request.Tcs.TrySetResult(CanceledResult.Instance);

molnard · 2024-01-29T16:33:57Z

WalletWasabi/Wallets/FilterProcessor/BlockDownloadService.cs

+
+					for (int i = 0; i < toStart; i++)
+					{
+						// Dequeue does not provide priority value.


Nit: I don't understand this comment. It provides, it is there.

I think it's a leftover, forgot to be removed from #12184 (comment).

molnard · 2024-01-29T16:34:39Z

WalletWasabi/Wallets/FilterProcessor/BlockDownloadService.cs

+					for (int i = 0; i < toStart; i++)
+					{
+						// Dequeue does not provide priority value.
+						if (!BlocksToDownload.TryDequeue(out Request? queuedRequest, out Priority? _))


Suggested change

if (!BlocksToDownload.TryDequeue(out Request? queuedRequest, out Priority? _))

if (!BlocksToDownload.TryDequeue(out Request? queuedRequest, out _))

molnard · 2024-01-29T16:56:32Z

WalletWasabi/Wallets/FilterProcessor/BlockDownloadService.cs

+		catch (Exception ex)
+		{
+			Logger.LogError(ex);
+			throw;


This means, game over. What happens with the wallet after this? How does this scenario look like?

When loading?

When opened?

This cannot happen. Only UnreachableException can be thrown from inside this trycatch block. Probably Exception should be changed to UnreachableException, or the clause removed altogether

I don't know, is it OK to stop in this case?

There is no case. UnreachableException means that it's... not reachable. It is made to emphasize that the case can't happen, or that a modification was wrong if it triggers the exception.
You can catch UnreachableException if you want, or remove the clause.

molnard · 2024-01-29T17:10:12Z

WalletWasabi/Wallets/FilterProcessor/BlockDownloadService.cs

+			{
+				if (TrustedFullNodeBlockProviders.Length == 0)
+				{
+					return new RequestResponse(request, NoSuchProviderResult.Instance);


Interesting. The plan is to introduce some kind of new behavior? What happens if I set my local node as source and it cannot provides? We are not falling back to random nodes anymore?

kiminuo#1 (comment)

And the implementation as integration made here:
https://github.com/zkSNACKs/WalletWasabi/pull/12148/files#diff-a3e12639ae8c2e59db996ca151cbe714473c840c9bfa4f6da1d8f30508ab1d6eR334

Basically, we want for that the caller to provide its own method to choose the source from.

Quickly checking this code: https://github.com/zkSNACKs/WalletWasabi/pull/12148/files#diff-a3e12639ae8c2e59db996ca151cbe714473c840c9bfa4f6da1d8f30508ab1d6eR338

If the full node fails then we go to P2P?

Yes, same as what SmartBlockProvider is currently doing

WalletWasabi/Wallets/BlockProvider/IP2PBlockProvider.cs

molnard

@turbolay can you create another PR that you can touch and finish this? Kimi won't be here for a week or more AFAIK

…2024-01-04-ParallelBlockDownloadingService

This reverts commit df095cc

kiminuo · 2024-02-10T15:43:44Z

I added changes from #12328 here except the following commits:

because I don't think those changes are good. Adding static property public static TerminateService? Instance { get; private set; }:

adds a circular dependency in the sense that BDS depends on TerminateService but TS is a higher level component.
static just adds tech debt¹
writing a unit test for the change would show that it's bad idea but none was added.

We should just let the UnreachableException just bubble up and trigger launching of the crash reporter.

Or alternatively we can just call [Environment.Exit(Int32)](https://learn.microsoft.com/en-us/dotnet/api/system.environment.exit?view=net-8.0) and end without crash reporter to work around the issue at the moment.

Continuing after UnreachableException can be dangerous, we should really crash as fast as possible because UnreachableException means "catastrophic error" and even things like saving settings can break the app in unexpected ways.

tl;dr: UnreachableException means reaching state that should never occur and if it does then we have a serious bug in code. Testing before release should uncover these bugs.

We have some statics in code already and it's very costly to fix them (speed of refactoring efforts to remove them is ~0, so ...). ↩

molnard

ACK. I agree to merge with the promise of adding some effective meaning of that throw there - whatever it will be.

Fix some warnings

4a198c3

pull-request-size bot added the size/L label Jan 4, 2024

kiminuo commented Jan 4, 2024

View reviewed changes

WalletWasabi.Tests/UnitTests/Transactions/TransactionProcessorTests.cs Outdated Show resolved Hide resolved

kiminuo commented Jan 4, 2024

View reviewed changes

WalletWasabi/Wallets/FilterProcessor/ParallelBlockDownloadService.cs Outdated Show resolved Hide resolved

kiminuo changed the title ~~Introduce ParallelBlockDownloadingService~~ [Wallet synchronization] Introduce ParallelBlockDownloadingService Jan 4, 2024

kiminuo requested a review from turbolay January 4, 2024 12:44

Implement ParallelBlockDownloadService

455e3e4

kiminuo force-pushed the feature/2024-01-04-ParallelBlockDownloadingService branch from 70743c5 to 455e3e4 Compare January 4, 2024 12:47

molnard suggested changes Jan 6, 2024

View reviewed changes

kiminuo added 2 commits January 6, 2024 21:50

Merge commit '0b588849964ee1358aeec88070231ccba3f34e09' into feature/…

4ef8974

…2024-01-04-ParallelBlockDownloadingService

More tests

876c26d

turbolay requested changes Jan 9, 2024

View reviewed changes

Merge branch 'master' into feature/2024-01-04-ParallelBlockDownloadin…

65c9332

…gService

kiminuo added 3 commits January 11, 2024 09:34

Merge branch 'master' into feature/2024-01-04-ParallelBlockDownloadin…

4b51771

…gService

Rename class

c9dcfe7

Re-design to work with priorities and refactor code

1f22226

kiminuo changed the title ~~[Wallet synchronization] Introduce ParallelBlockDownloadingService~~ [Wallet synchronization] Introduce BlockDownloadingService Jan 11, 2024

kiminuo added 5 commits January 11, 2024 15:03

Merge branch 'master' into feature/2024-01-04-ParallelBlockDownloadin…

721cec5

…gService # Conflicts: # WalletWasabi/Wallets/FilterProcessor/Priority.cs

Merge commit 'd724622604d14bc00e8db4e5849f99f7d04c61b2' into feature/…

5c042fc

…2024-01-04-ParallelBlockDownloadingService

Add results of operations

5a50ac1

Improve

37a7648

Remove IRepository and replace it with IFileSystemBlockRepository

7d81d7b

kiminuo mentioned this pull request Jan 12, 2024

[trivial] Remove IRepository and replace it with IFileSystemBlockRepository #12236

Merged

kiminuo added 2 commits January 12, 2024 10:11

Merge remote-tracking branch 'kiminuo/feature/2024-01-12-BlockReposit…

25e0f7f

…ory-improvements' into feature/2024-01-04-ParallelBlockDownloadingService

Support for FileSystemBlockRepository

373b516

pull-request-size bot added size/XL and removed size/L labels Jan 12, 2024

CF?

96c9553

kiminuo force-pushed the feature/2024-01-04-ParallelBlockDownloadingService branch from 168b94f to 96c9553 Compare January 12, 2024 09:40

Remove max-attempts behavior, modify tests

897b8d8

Add NoSuchProviderResult

dca0ad7

bigger timeout

e44d784

Improve parallel behavior

45be3e3

Merge commit '7137843fd202dc7ea95d6172a93b9184a22cf156' into feature/…

9088c42

…2024-01-04-ParallelBlockDownloadingService

kiminuo mentioned this pull request Jan 25, 2024

Integrate BlockDownloadService #12314

Closed

turbolay approved these changes Jan 26, 2024

View reviewed changes

turbolay requested a review from molnard January 26, 2024 06:03

molnard suggested changes Jan 29, 2024

View reviewed changes

molnard suggested changes Jan 30, 2024

View reviewed changes

turbolay mentioned this pull request Jan 30, 2024

[Wallet synchronization] Introduce BlockDownloadingService - Continuation #12328

Closed

kiminuo and others added 7 commits February 10, 2024 16:27

Merge commit '7207224510a653789ef9cba62c470235793386f8' into feature/…

9fc90f4

…2024-01-04-ParallelBlockDownloadingService

Revert "P2PNodesManager: Use ConnectedNodesCount everywhere"

e835870

This reverts commit df095cc

Code review suggestions.

3ec7c62

fix cf

26785d0

Style suggestions

f8a264a

Clarify P2P and Trusted

bc57424

Add remarks for P2pSourceRequest.Automatic

5d190d4

molnard approved these changes Feb 16, 2024

View reviewed changes

kiminuo merged commit 7452bad into WalletWasabi:master Feb 17, 2024
6 of 7 checks passed

molnard mentioned this pull request Mar 19, 2024

[BLOCKS DL] Save block in parallel to download #9130

Closed

turbolay mentioned this pull request Mar 24, 2024

Download blocks faster and more private #10010

Open

10 tasks

	_ = request.Tcs.TrySetResult(response.Result);
	request.Tcs.TrySetResult(response.Result);

	_ = request.Tcs.TrySetResult(CanceledResult.Instance);
	request.Tcs.TrySetResult(CanceledResult.Instance);

	if (!BlocksToDownload.TryDequeue(out Request? queuedRequest, out Priority? _))
	if (!BlocksToDownload.TryDequeue(out Request? queuedRequest, out _))

[Wallet synchronization] Introduce BlockDownloadingService #12184

[Wallet synchronization] Introduce BlockDownloadingService #12184

Conversation

kiminuo commented Jan 4, 2024 • edited Loading

molnard left a comment

Choose a reason for hiding this comment

turbolay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbolay commented Jan 10, 2024 • edited Loading

kiminuo commented Jan 23, 2024

kiminuo commented Jan 24, 2024

kiminuo commented Jan 24, 2024

turbolay commented Jan 24, 2024

kiminuo commented Jan 24, 2024

kiminuo commented Jan 25, 2024

turbolay left a comment

Choose a reason for hiding this comment

kiminuo commented Jan 26, 2024 • edited Loading

molnard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molnard Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbolay Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbolay Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molnard left a comment

Choose a reason for hiding this comment

kiminuo commented Feb 10, 2024 • edited Loading

Footnotes

molnard left a comment

Choose a reason for hiding this comment

[Wallet synchronization] Introduce `BlockDownloadingService` #12184

[Wallet synchronization] Introduce `BlockDownloadingService` #12184

kiminuo commented Jan 4, 2024 •

edited

Loading

turbolay commented Jan 10, 2024 •

edited

Loading

kiminuo commented Jan 26, 2024 •

edited

Loading

molnard left a comment •

edited

Loading

molnard Jan 30, 2024 •

edited

Loading

turbolay Jan 30, 2024 •

edited

Loading

turbolay Jan 29, 2024 •

edited

Loading

kiminuo commented Feb 10, 2024 •

edited

Loading