pendulum-chain / spacewalk

Apache License 2.0
34 stars 7 forks source link

501 fix occassional horizonresponse decodeerror #502

Closed b-yap closed 5 months ago

b-yap commented 5 months ago

closes #501

The error found was 403, and as explained by @ebma, it should be recoverable. Not only that, I've updated the HorizonResponseError structure, flexible enough to accept either a ReqwestError or an error in string format.

The string format is important to find the correct error.

b-yap commented 5 months ago

@ebma by flagging this error as "recoverable", it will resend the same transaction again. see https://github.com/pendulum-chain/spacewalk/blob/501-fix-occassional-horizonresponse-decodeerror/clients/wallet/src/horizon/horizon.rs#L167.

ebma commented 5 months ago

Yes, the logic makes sense to me. I was asking if you tested it because it would actually be interesting to see how this behaves in practice. Because if this error is indeed somehow related to rate limiting, our logic here is not enough to make the transaction eventually go through. Since even if we retry with the same transaction over and over again, the rate-limiting would continue to block it. If this is the case, which I don't know, then we should add some kind of delay here and wait for the rate-limiting window to expire.

b-yap commented 5 months ago

Yesterday it was difficult with the accounts not existing.

There is already a delay added on the loop, the backoff_delay_counter.

https://github.com/pendulum-chain/spacewalk/blob/a2624f0f5921126f1d5007a66797a078aac7c6c8/clients/wallet/src/horizon/horizon.rs#L177-L183

Will this slowdown the CI? Possible.

ebma commented 5 months ago

Ahhh true, we already have that delay in place 👍 That's great.

I found out that the Horizon's apparently have IP-based rate limiting of 3600 requests per hour ie. one request per second on average.

ebma commented 5 months ago

Only one integration test failed so we can consider it a shaky test. I checked cargo clippy and rustfmt locally and there are no complaints. Merging.