An invalid txout item cycles eternally in the outbound queue

This Issue is a work in progress.

Since Block 6320000 (2022-07-07), invalid txout item with in_hash 87035FCAA7B0A0E0425D8CDC9F925C1A65A8865E91D69ED59AB98D04E4FD9435 has remained in the outbound queue, with its vault_pub_key reassigned (and a new "scheduled_outbound" event emitted) every 300 blocks.

"scheduled_outbound" event examples:
https://midgard.thorchain.info/v2/debug/block/6320000
https://midgard.thorchain.info/v2/debug/block/6320300
https://midgard.thorchain.info/v2/debug/block/6357500
https://ops.ninerealms.com/monitor?tab=outbounds&search=1012878

Specifically, its aggregator_target_asset is "sushi".
(Acknowledgement for context to @oxyc#0505 .)

LackSigning affects txout items which have exceeded the transaction signing period and does not slash Asgard vaults, so this happily appears to not be causing slashes.

The txout item could be removed from the queue by changing MaxOutboundAttempts to a non-zero value, but this would be addressing a symptom rather than the root. As for the root, rather than only the question of how an invalid-at-the-time txout entered the queue originally, the question remains of how an originally-valid txout item should be handled if it later becomes invalid (such as by the removal of an aggregator address or a target asset becoming unavailable--should for instance in then be refunded?).

LackSigning calls UnSafeAddTxOutItem.
Notably, TryAddTxOutItem would call prepareTxOutItem which deducts an outbound fee, and so is not appropriate for vault-reassignment after SigningTransactionPeriod blocks without being signed.
However, UnSafeAddTxOutItem calls addToBlockOut which calls AppendTxOut which calls SetTxOut without checking whether the txout item is still (or was ever) valid.

I currently propose that in some form a vault-reassigning include a check for txout item validity.
|
Edit: Specifically, either in the shared common downstream functions addToBlockOut (before telemetry and the "scheduled_outbound" event is emitted)
or in a separate function (something like 'ValidateTxOut') which could be called early by parallel functions and similarly avoid code duplication/inconsistency.
|
Arguably, even if in a separate function, being called from a necessarily common function would avoid the possibility of it being accidentally left out of a new parallel function in future.
|
At this time, my preference is for an addition to the start of addToBlockOut.

Second edit: Updating this issue with an associated logged error, originating from this line of code in LackSigning and this line of code in updateTxOutGasV88.

[LOG] error
{
  "error": "fail to find tx out in ObservedTxVoter 87035FCAA7B0A0E0425D8CDC9F925C1A65A8865E91D69ED59AB98D04E4FD9435",
  "hash": "87035FCAA7B0A0E0425D8CDC9F925C1A65A8865E91D69ED59AB98D04E4FD9435",
  "caller": "/app/x/thorchain/manager_slasher_current.go:356",
  "message": "Failed to update MaxGas of action in ObservedTxVoter"
}

Third edit:
More specifically, this code is relevant:
https://gitlab.com/thorchain/thornode/-/blob/v1.93.0/bifrost/pkg/chainclients/ethereum/ethereum.go#L442-445

if !strings.EqualFold(targetAddr.String(), tx.AggregatorTargetAsset) {
	c.logger.Error().Msgf("aggregator target asset address can't roundtrip , ignore tx (%s != %s)", tx.AggregatorTargetAsset, targetAddr.String())
	return nil, nil
}

Note this log:
"aggregator target asset address can't roundtrip , ignore tx (sushi != 0x0000000000000000000000000000000000000000)"