`SpecificNodeBlockProvider`: Modify handshake warning logging #12949

kiminuo · 2024-04-29T12:29:28Z

This PR is basically just what @turbolay suggested here.

Testing

Run WW with your bitcoin node already started. No warning should be reported.
Run WW with your bitcoin node stopped and then start your bitcoin node. A single warning should be produced.
The same as 2 but start and stop your bitcoin node while WW is running.

kiminuo · 2024-04-29T12:30:55Z

WalletWasabi/Wallets/SpecificNodeBlockProvider.cs

@@ -110,7 +115,8 @@ private async Task ReconnectingLoopAsync()
 			}
 			catch (Exception ex)
 			{
-				if (ex is OperationCanceledException && connectCts.Token.IsCancellationRequested)


I believe that connectCts.Token.IsCancellationRequested should not be here as that looks like a bug to me. I replaced it with !shutdownToken.IsCancellationRequested as that makes more sense to me.

You are right it looks like it made no sense

…nection

kiminuo · 2024-04-29T20:11:20Z

WalletWasabi/Wallets/SpecificNodeBlockProvider.cs

@@ -110,14 +116,17 @@ private async Task ReconnectingLoopAsync()
 			}
 			catch (Exception ex)
 			{
-				if (ex is OperationCanceledException && connectCts.Token.IsCancellationRequested)
+				// In general, we do do not log a message if we fail to connect to the bitcoin core endpoint. The only exception is when a handshake error occurs.


This is current behavior. I find it it weird though. I would expect that we log once that we cannot log to the specified bitcoin node (i.e. to log when SocketException is thrown`).

We are logging, simply in Trace. We could move to Warning and extend logWarningOnHandshakeError to any type of exception.

We are logging, simply in Trace.

Ah, I see now.

We are logging, simply in Trace. We could move to Warning and extend logWarningOnHandshakeError to any type of exception.

Do you want me to change it in this PR or is like an idea?

I can do it in a subsequent PR

kiminuo · 2024-04-29T20:12:25Z

WalletWasabi/Wallets/SpecificNodeBlockProvider.cs

-				if (ex is OperationCanceledException && connectCts.Token.IsCancellationRequested)
+				// In general, we do do not log a message if we fail to connect to the bitcoin core endpoint. The only exception is when a handshake error occurs.
+				// If we are not shutting down the application and if this is not a repeated-handshake failure, then log a warning.
+				if (ex is OperationCanceledException && !shutdownToken.IsCancellationRequested && logWarningOnHandshakeError)


Catching OperationCanceledException seems like it's some sort of workaround, I tried to modify requirements of the local node (I required a nonsense witness version) and this warning was not shown.

So I'm not too sure how we ended up with this condition.

adamPetho

I don't get a full node, so I can't test it, but code changes LGTM

WalletWasabi/Wallets/SpecificNodeBlockProvider.cs

kiminuo · 2024-05-09T10:39:43Z

I don't get a full node, so I can't test it, but code changes LGTM

You can test it with a RegTest node as well. That's what I did.

Co-authored-by: adamPetho <45069029+adamPetho@users.noreply.github.com>

adamPetho · 2024-05-09T11:38:10Z

Wasabi + backend + Knots are running, everything is fine.
Then I kill Knots, but the exception is SocketException all the time.

I couldn't manage to get this warning log Wasabi could not complete the handshake with the node....
Anything specific I should do while testing this? I tried to follow your description.

Works fine
& 3. SocketExceptions

kiminuo · 2024-05-09T11:41:25Z

Wasabi + backend + Knots are running, everything is fine. Then I kill Knots, but the exception is SocketException all the time.

I couldn't manage to get this warning log Wasabi could not complete the handshake with the node.... Anything specific I should do while testing this? I tried to follow your description.
1. Works fine

2. & 3. SocketExceptions

It sounds like this: https://github.com/zkSNACKs/WalletWasabi/pull/12949/files#r1583684668

adamPetho

t(rust)ACK, I wasn't able to repro the original issue. Code still LGTM

turbolay · 2024-05-09T12:47:25Z

WalletWasabi/Wallets/SpecificNodeBlockProvider.cs

@@ -156,6 +165,9 @@ private async Task ReconnectingLoopAsync()
 		}
 	}

+	/// <exception cref="SocketException">When connecting to the node fails.</exception>
+	/// <exception cref="OperationCanceledException">When handshake protocol with the node fails.</exception>


According to your previous comment, isn't this statement incorrect?

I have removed that line. I've been looking into NBitcoin and its Node.VersionHandshake and it throws InvalidOperationException in multiple scenarios. But it's a mess because NBitcoin is such a sh* library (Try* methods throw, convert one exception to another one, etc.).

How exactly it throws OperationCanceledException, I don't know.

My best guess is that this:

https://github.com/MetacoSA/NBitcoin/blob/583e33b038e6e04c9c87d9aee0b553b00d74d7a0/NBitcoin/Protocol/NodeListener.cs#L51

can throw an OCE and then the scenario is: You send a handshake and you don't get a response (which can presumably happen if you are starting Bitcoin Core and it is not fully initialized).

But like who knows. NBitcoin has been written in an adhoc way so it's hard to say what's intentional and what is a bug.

Yeah I see the problem... I will make a new PR to log in all cases, just waiting for CI

pull-request-size bot added the size/XS label Apr 29, 2024

kiminuo commented Apr 29, 2024

View reviewed changes

kiminuo force-pushed the feature/2024-04-29-SpecificNodeBlockProvider-log-better branch from bf7ded4 to 42e9db2 Compare April 29, 2024 12:41

Log a handshake warning only the first time or after a successful con…

f7aee44

…nection

kiminuo force-pushed the feature/2024-04-29-SpecificNodeBlockProvider-log-better branch from 42e9db2 to f7aee44 Compare April 29, 2024 19:56

pull-request-size bot added size/S and removed size/XS labels Apr 29, 2024

kiminuo changed the title ~~SpecificNodeBlockProvider: Log warning only the first time or after a successful connection~~ SpecificNodeBlockProvider: Modify handshake warning logging Apr 29, 2024

kiminuo commented Apr 29, 2024

View reviewed changes

kiminuo marked this pull request as ready for review April 29, 2024 20:12

kiminuo requested a review from turbolay April 29, 2024 20:12

kiminuo requested a review from adamPetho May 9, 2024 06:17

kiminuo mentioned this pull request May 9, 2024

[Tor] Towards Whonix & Tails (take 2) #12991

Merged

adamPetho previously approved these changes May 9, 2024

View reviewed changes

WalletWasabi/Wallets/SpecificNodeBlockProvider.cs Outdated Show resolved Hide resolved

Update WalletWasabi/Wallets/SpecificNodeBlockProvider.cs

626e922

Co-authored-by: adamPetho <45069029+adamPetho@users.noreply.github.com>

kiminuo dismissed adamPetho’s stale review via 626e922 May 9, 2024 10:39

adamPetho previously approved these changes May 9, 2024

View reviewed changes

turbolay reviewed May 9, 2024

View reviewed changes

remove

2324ec0

kiminuo dismissed adamPetho’s stale review via 2324ec0 May 9, 2024 13:12

turbolay approved these changes May 9, 2024

View reviewed changes

kiminuo merged commit 925ef8e into WalletWasabi:master May 9, 2024
5 of 8 checks passed

kiminuo deleted the feature/2024-04-29-SpecificNodeBlockProvider-log-better branch May 9, 2024 13:25

turbolay mentioned this pull request May 9, 2024

SpecificNodeProvider: Log Warning on every connection ex #13010

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`SpecificNodeBlockProvider`: Modify handshake warning logging #12949

`SpecificNodeBlockProvider`: Modify handshake warning logging #12949

kiminuo commented Apr 29, 2024 •

edited

kiminuo Apr 29, 2024 •

edited

turbolay May 9, 2024

kiminuo Apr 29, 2024

turbolay May 9, 2024

kiminuo May 9, 2024

turbolay May 9, 2024

kiminuo Apr 29, 2024

adamPetho left a comment

kiminuo commented May 9, 2024

adamPetho commented May 9, 2024

kiminuo commented May 9, 2024

adamPetho left a comment

turbolay May 9, 2024

kiminuo May 9, 2024 •

edited

kiminuo May 9, 2024 •

edited

turbolay May 9, 2024

SpecificNodeBlockProvider: Modify handshake warning logging #12949

SpecificNodeBlockProvider: Modify handshake warning logging #12949

Conversation

kiminuo commented Apr 29, 2024 • edited

Testing

kiminuo Apr 29, 2024 • edited

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

adamPetho left a comment

Choose a reason for hiding this comment

kiminuo commented May 9, 2024

adamPetho commented May 9, 2024

kiminuo commented May 9, 2024

adamPetho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiminuo May 9, 2024 • edited

Choose a reason for hiding this comment

kiminuo May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`SpecificNodeBlockProvider`: Modify handshake warning logging #12949

`SpecificNodeBlockProvider`: Modify handshake warning logging #12949

kiminuo commented Apr 29, 2024 •

edited

kiminuo Apr 29, 2024 •

edited

kiminuo May 9, 2024 •

edited

kiminuo May 9, 2024 •

edited