polygon: fix wallet connected/disconnected notifications show up constantly #3163
Conversation
There was a problem hiding this comment.
Thank you for looking at this norwnd. All of these values may be back from when we thought we could use local light nodes, and now when testing its mostly simnet, so the actual time it takes for providers to respond, normally, is useful info.
Maybe we should update the live tests to output the time it takes for all providers to respond with all methods.
| @@ -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 = 4 * time.Second | |||
There was a problem hiding this comment.
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.
Do the errors end in failed trades or they are just loud?
Polygon connect/disconnect notifications are just noisy (I haven't seen any negative side effects with either master branch or this PR)
30 seconds feels excessive though. Does just a little more work? like... 6 or 8?
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)
We are a little worried about trade ticks taking a long time, and this may increase those.
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
| @@ -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) { | |||
| ctx, cancel := context.WithTimeout(ctx, 2*time.Second) | |||
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)
|
Oh the tests do show time for a set provider, just not for the free ones. Here are some times randomly for me and getting the header: They all seem pretty low, ofc that could change based on how busy they are and distance from them... |
Been running this change for couple of weeks now, pretty sure it's stable and it fixes the issue.
Closes #3143