Routinely returns `AllAttemptsErrored([])` on failed requests

[binarybaron]

In these parts of the code we returning before adding the latest error to the errors: Vec<_>. This causes this code to return AllAttemptsErrored([]) which is a bit frustrating.

rust-electrum-client/src/client.rs

Lines 56 to 65 in 761796c

	let failed_attempts = errors.len() + 1;
	if retries_exhausted(failed_attempts, $self.config.retry()) {
	warn!("call '{}' failed after {} attempts", stringify!($name), failed_attempts);
	return Err(Error::AllAttemptsErrored(errors));
	}
	warn!("call '{}' failed with {}, retry: {}/{}", stringify!($name), e, failed_attempts, $self.config.retry());
	errors.push(e);

rust-electrum-client/src/client.rs

Lines 80 to 89 in 761796c

	let failed_attempts = errors.len() + 1;
	if retries_exhausted(failed_attempts, $self.config.retry()) {
	warn!("re-creating client failed after {} attempts", failed_attempts);
	return Err(Error::AllAttemptsErrored(errors));
	}
	warn!("re-creating client failed with {}, retry: {}/{}", e, failed_attempts, $self.config.retry());
	errors.push(e);

[ValuedMammal]

I'm looking for a way to reproduce it. Do you have any log output you can share? Have you tried increasing the Config::retry?

[ValuedMammal]

Yes it would make sense to add the latest error to errors before checking retries_exhausted. Also we're using the same errors vec to track both failed network calls as well as failure to recreate the client, but don't know if that's significant.

[binarybaron]

I don't have a good reproducable code sample unfortunately.

I think part of this may have also been caused by our internal wrapper we wrote around the client (electrum-pool) and the way we propagate errors from that wrapper to the rest of the codebase (where it is logged).

Our electrum-pool which load balances request over multiple servers sets the retries to 0 as we handle retries a layer above (in the load balancer itself). This may result in us never receiving the actual client errors.

[oleonardolima]

Yes it would make sense to add the latest error to errors before checking retries_exhausted. Also we're using the same errors vec to track both failed network calls as well as failure to recreate the client, but don't know if that's significant.

Agreed, maybe it's a good idea to revamp it and use different error vectors for each concern.

[binarybaron]

The eigenwallet project will award a bounty of 0.1 XMR (Monero) (around $39) to whoever fixes this issue. Maybe one of our contributors will pick this up at some point...