-
Notifications
You must be signed in to change notification settings - Fork 92
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
client: Set conf deadline per asset. #1552
Conversation
If pending you can see here where the error from So, not doing pending gives us a better error. I think we are getting this five second timeout for the contract call https://github.com/ethereum/go-ethereum/blob/127dc5982e3484406eae0631326bbc356f914749/rpc/client.go#L44 I don't guess we don't need to worry about pending state... For one it doesn't seem to work right, and for the other block times are fast enough I think we can wait for our txn to be mined. |
aeacb90
to
8f9fe9d
Compare
Looks to me like it's being passed from us to the contract via Line 29 in c77da58
Should we make that longer?
Also, pending with a LES (light) client is kinda flakey and unpredictable all around. Under what circumstances would pending contract state even be possible to determine? If we do not do pending, as in this PR, why does a deadline exceeded "always happen once for new swaps"? I would think the
Does that seem like a go-ethereum bug? |
oh! How'd I miss that... Will try increasing. Declared in trade.go and used in wallet.go, ok. |
Setting
I don't know without digging through the code... but it's probably safe to assume the light client is not capable of calculating the evm state by itself.
Again not sure, I guess it just takes a while to find them at first as I am seeing no errors after increasing the timeout x5. Will change the comment. I can set up some timers to see how long it usually takes on testnet.
Bad practice leading to possible bug? It would be better if they returned the context deadline error there. It would have helped with our debugging. Guess I could make an issue... |
Throwing in some logs:
And the highest spike I've seen is:
But it doesn't look like it has to do anything with it being the first fetch. Just seems to go up and down randomly right now, due to network conditions I guess. I have not seen 3+ though, so maybe setting to three seconds for eth is safe. Not sure if mainnet would be different. The client logs, eth and dcr are mixed... but the longer ones are eth |
I think it's probably better to error now that we have more information. If it errors too much for mainnet, we would need to up the deadline more I guess. |
8f9fe9d
to
2ca36ba
Compare
Ethereum appears to need a little more time to find swap confirmations sometimes on testnet. Make the confirmation deadline a per-asset constant and double it for eth.
2ca36ba
to
8cb2bf6
Compare
Apologies, the pr changed a lot. |
No problem. Thanks for characterizing execution time. 4 sec does seem good for ETH given what you've shown. |
ctx, cancel := context.WithTimeout(ctx, confCheckTimeout) | ||
defer cancel() |
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.
Now I'm reconsidering timeouts for the dcr and spv wallets if they need to do a cfilters scan because those can be more time consuming.
Actually, did you mean to skip BTC's timeout in this PR?
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.
It looked like btc didn't use the context. Does 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.
dcrdex/client/asset/btc/btc.go
Lines 2766 to 2769 in 11fe415
// SwapConfirmations gets the number of confirmations for the specified swap | |
// by first checking for a unspent output, and if not found, searching indexed | |
// wallet transactions. | |
func (btc *baseWallet) SwapConfirmations(_ context.Context, id dex.Bytes, contract dex.Bytes, startTime time.Time) (uint32, bool, error) { |
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.
Ah, good point. We have a TODO to update call
so a context gets used, but you're right it's not used now.
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.
Seems good. Let's see how this goes.
BTW, nice work with ethereum/go-ethereum#24645. I'll update #1542 to consume that commit at least |
closes #1537