340 add handling for non recoverable stellar submission errors

[adelarja]

The aim of this PR is to add error handling logic for failed submissions.

Closes #340

General overview of the changes:

1. A few method renames;
2. Introduce new file: resubmissions.rs, dedicated for resubmission and handling of errors. Bulk of the code is here
3. Introduced new file: mock.rs, contains the function helpers for the tests cases
4. Also updated the resubmit_transactions_from_cache(), with a parameter that helps handle failed tasks

How to begin the review:

• clients/wallet/src/resubmissions.rs
  • type FailedTasks = essential for removing transaction envelopes in cache for failed resubmissions
    • ⭐ instead of complicating things with channels, I just immediately call the remove_tx_envelope_from_cache() if the error is not viable for resubmission
  • fn resubmit_transactions_from_cache() = the only public method. Every 30 minutes it will read from cache and resubmit existing ones
  • fn _resubmit_transactions_from_cache()
    1. Collect all envelopes from cache.
    2. Loops through all envelopes and resubmit them
    3. ^ collects errors from the loop and tries to handle them
    4. if auto check is set to true, check fn auto_check_running_tasks()
  • fn handle_errors()
    • loops through all the errors and handle them one by one
    • ⭐ for errors that cannot be resubmitted again, its corresponding envelope is removed from cache
  • fn handle_error() = resubmits transactions with the ff errors:
    • tx_bad_seq
    • tx_internal_error
    • CacheErrorKind::SequenceNumberAlreadyUsed
• fn handle_tx_internal_error() = resubmits a transaction with the error tx_internal_error
• fn handle_tx_bad_seq_error_with_xdr() = resubmits a transaction with the error tx_bad_seq
• fn bump_sequence_number_and_submit() = bump the sequence number of a transaction and resubmits it
• fn is_transaction_already_submitted() = a costly checking if a transaction already exists. This is currently limited to just 10 pages
• fn auto_check_running_tasks() = auto checks for failed resubmissions and forcefully remove transactions from the cache

[ebma]

Looks good so far. We'd still need to add some very descriptive logging of course but the core logic seems on track

[ebma]

Added some minor comments but I really like the new structure overall and I think it should do the trick. Good job @b-yap 👍

[ebma]

Why do we need to pause it? We are already pausing in the loop of the outer function, no?

[b-yap]

removed

[ebma]

I think it's better to keep remove this here and keep it in line 270. In the (very rare/unlikely) case that our vault client shuts down when executing the logic between line 260 and 264, the actual resubmission might not have been happened (because the client dies) but the sequence number and envelope got removed from cache, so upon restart the vault client would not try that one again, which is bad.

[ebma]

Oh actually, I'm wrong. I missed that we might return from this function in line 264 already. Let's move this line into the if statement of lines 263 to 265.

[b-yap]

moved

[ebma]

Suggested change

	// Check if we already submitted this transaction
	if !self.is_transaction_already_submitted(&tx).await {
	return self.bump_sequence_number_and_submit(tx).await
	}
	// Check if we already submitted this transaction
	if !self.is_transaction_already_submitted(&tx).await {
	self.bump_sequence_number_and_submit(tx).await
	self.remove_tx_envelope_from_cache(tx.seq_num);
	return;
	}

[b-yap]

i called remove_tx_envelope_from_cache() first, before the bump_sequence_number_and_submit()

[ebma]

It looks good overall, I just added some minor suggestions

[ebma]

Should we rename this function to indicate that this will actually start scheduling a periodic resubmission? Maybe we should also pass in the resubmission interval into this function call instead. Something like: start_periodic_resubmission_of_transactions_from_cache(&self, interval_in_seconds: u64)?

[b-yap]

renamed, and added the interval param

[ebma]

I think this is good to go now. At least I don't see any other immediate problems with this implementation and the error handling seems to be the way we need it. Good job @b-yap and ready for merge (once the CI errors are gone).