polygon: fix wallet connected/disconnected notifications show up constantly #3163
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,7 @@ const ( | |
| // confCheckTimeout is the amount of time allowed to check for | ||
| // confirmations. Testing on testnet has shown spikes up to 2.5 | ||
| // seconds. This value may need to be adjusted in the future. | ||
| confCheckTimeout = 30 * time.Second | ||
|
|
||
| // coinIDTakerFoundMakerRedemption is a prefix to identify one of CoinID formats, | ||
| // see DecodeCoinID func for details. | ||
|
|
@@ -3700,7 +3700,7 @@ func (eth *ETHWallet) monitorBlocks(ctx context.Context) { | |
| // tipChange callback function is invoked and a goroutine is started to check | ||
| // if any contracts in the findRedemptionQueue are redeemed in the new blocks. | ||
| func (eth *ETHWallet) checkForNewBlocks(ctx context.Context) { | ||
|
There was a problem hiding this comment. Thirty seconds also seems excessive here... does 8 work? It's only asking for a header. There was a problem hiding this comment. Answered here - #3163 (comment) but the general idea is size isn't the only thing that matters when it comes to network-communications, there can be random delays we can't predict (like router buffers overflows) so I guess excessive/non-excessive should be defined as "do we see the issue appear on daily/weekly/monthly basis" (30s seems to resolve it at least on a daily/weekly basis) |
||
| ctx, cancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer cancel() | ||
| bestHdr, err := eth.node.bestHeader(ctx) | ||
| if err != nil { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are a little worried about trade ticks taking a long time, and this may increase those. Do the errors end in failed trades or they are just loud?
Actually the comment is probably from back when we thought we could use light nodes, so providers would be longer. 30 seconds feels excessive though. Does just a little more work? like... 6 or 8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polygon connect/disconnect notifications are just noisy (I haven't seen any negative side effects with either master branch or this PR)
I have tried 10 seconds initially, but that wasn't quite enough to resolve the issue completely (those notifications would still show up couple times a day)
could maybe try 15 or 20 though (testing, will report back in several days or 1 week)
besides, if that's indeed a problem ^ perhaps separating fetching blockchain data from trade ticks (so ticks just read whatever the latest blockchain state is rather than go ahead and fetch the latest data) is how we should address it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been running this change with 15s (instead of 30s) for about 5 days now, it seems to work just as flawlessly for me so far - but maybe that's just me (my network, and location)
15s might not be enough for Tor-based connections though for example, so I personally think 30s is more reasonable timeout value for www-request
as I mentioned above, trying to tweak this timeout lower for the sake of trade ticks seems like a wrong way to go about it (the interaction with trade ticks needs to be handled differently, not the timeout)
cc @buck54321 @martonp if you have any opinion on this ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've long considered some kind of system where the default is something long like 1 - 5 minutes, but if there are pending transactions, or if the user takes some action that indicates that they may be waiting for a transaction, such as calling
FundOrderor opening the "Receive" dialog in the UI, we signal the wallet to check more frequently, like every 10 or 20 seconds, for a while.There are also probably some improvements to be made around the handling of "ticks" in core. A good audit of the synchronicity patterns in those routines would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 30 seconds is fine for now. I have seen this notification as well and it's a bit annoying.
Agree with this.