[Version-unspecific] Use consistent no-0x-prefix hash format for EVM/ETH signerCacheManager

[Draft: See the discussion thread.]

[Version-unspecific]

This is prompted by #1837 (closed)
'Failed outbounds not properly removed from ethereum (and evm?) signer cache'
, but does not close it.

Observations (instant observations of self-signed transactions or observations of transactions signed by nodes in other vaults)
call SetSigned in OnObservedTxIn for the Tx field of a TxInItem of a type from
stypes "gitlab.com/thorchain/thornode/bifrost/thorclient/types"
. https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/pkg/chainclients/evm/client.go#L754-767

It currently appears to me that this is intended to have the no-0x-prefix format of THORChain TxIDs,
and as such for consistent SetSigned/RemoveSigned interaction all EVM/ETH SetSigned/RemoveSigned should act on hashes with no 0x prefix.

My full reasoning(/comparison) is here:
#1837 (comment 1728126863)

As always, feedback is welcome!

---

Double-checking the impression that the /bifrost/thorclient/types TxInItem Tx field is expected to be of (no-0x-previx) THORChain format:

https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/observer/observe.go#L22
"gitlab.com/thorchain/thornode/bifrost/thorclient/types"

chunkifyAndSendToThorchain:
https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/observer/observe.go#L225-234

	for _, txIn := range o.chunkify(deck) {
		if err := o.signAndSendToThorchain(txIn); err != nil {
			o.logger.Error().Err(err).Msg("fail to send to THORChain")
			// tx failed to be forward to THORChain will be added back to queue , and retry later
			newTxIn.TxArray = append(newTxIn.TxArray, txIn.TxArray...)
			continue
		}

		i, ok := chainClient.(interface {
			OnObservedTxIn(txIn types.TxInItem, blockHeight int64)

signAndSendToThorchain (which recieves the same types.TxInItem as OnObservedTxIn)
chain-unspecifically called getThorchainTxIns:
https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/observer/observe.go#L516

getThorchainTxIns then
https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/observer/observe.go#L606
calls
txID, err := common.NewTxID(item.Tx)
, assuming NewTxID to already be in a no-0x form.
NewTxID is able to accept, but does not itself truncate, a 0x prefix.
https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/common/tx.go#L25-41

As further context for that the TxInItem Tx field is expected to have no 0x prefix:
https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L792-794

func (e *ETHScanner) getTxInFromTransaction(tx *etypes.Transaction) (*stypes.TxInItem, error) {
	txInItem := &stypes.TxInItem{
		Tx: tx.Hash().Hex()[2:],

https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L751-754

func (e *ETHScanner) getTxInFromSmartContract(tx *etypes.Transaction, receipt *etypes.Receipt) (*stypes.TxInItem, error) {
	e.logger.Debug().Msg("parse tx from smart contract")
	txInItem := &stypes.TxInItem{
		Tx: tx.Hash().Hex()[2:],

getTxInFromFailedTransaction: https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L888-891

	txHash := tx.Hash().Hex()[2:]

	return &stypes.TxInItem{
		Tx:     txHash,

For consistency, the choice then appears to be to remove the 0x from other instances (this MR's commit at time of writing),
or in the chain-specific OnObservedTxIn
https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/pkg/chainclients/ethereum/ethereum.go#L895
affix an "0x" prefix to the TxID used for the SetSigned call.

Though affixing a prefix would be a smaller change, my default inclination is in the direction that would allow eventual consolidation of OnObservedTxIn code
(being called from chunkifyAndSendToThorchain once observations have already been added to a common channel),
itself using only THORChain formats and chain-unspecifically calling chain-specific scanners.

(A further thought: rather than only check the length > 2 before truncating the first two characters,
also do a && thorTxID[:2] == "0x" check or not?)

---

2014-01-16 minor thought: My current impression is that EVM/ETH are the only chains that this is relevant for, because all others already use unprefixed hashes (consistent with THORChain's TxID format).

(As noted in the discussion thread below, it may be neater to put an 0x prefix check in SetSigned and/or RemoveSigned rather than alter (and need to remember to alter) every call.)

---

2024-01-17 update:

Approach now changed to the above, namely a check for and removal of "0x" in the CacheManager's SetSigned and RemoveSigned
(together with an explanatory comment),
including a temporary call of prefix-retained CacheStore RemoveSigned for while residual 0x-prefix-SetSigned hashes are still of concern.

My current impression is that the temporary CacheStore RemoveSigned call show generate no persistent log spam (from either it or the main call following),
thanks to the existing
https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/pkg/chainclients/shared/signercache/signer_cache_store.go#L63-68

	mapKey := s.getMapKey(transactionHash)
	value, err := s.db.Get([]byte(mapKey), nil)
	if err != nil {
		// bifrost didn't sign this tx , so it is fine
		if errors.Is(err, leveldb.ErrNotFound) {
			return nil

; perhaps if pre-modification SetSigned has been called for a hash both with and without the 0x prefix
(namely the same CacheHash and different transaction hashes from BroadcastTx and from OnObservedTxIn)
there might be confusion upon attempting to delete an unset CacheHash for the second set transaction hash,
but this should occur only once per residual set transaction (and only for failed/unstuck transaction special cases),
thus even if extra logs are generated not predicting meaningful log spam.

Incidental double-check:
https://gitlab.com/thorchain/thornode/-/blob/v1.126.0/bifrost/thorclient/types/tx_in.go#L84-88 \

// CacheHash calculate the has used for signer cache
func (t TxInItem) CacheHash(chain common.Chain, inboundHash string) string {
	str := fmt.Sprintf("%s|%s|%s|%s|%s", chain, t.To, t.Coins, t.Memo, inboundHash)
	return fmt.Sprintf("%X", sha256.Sum256([]byte(str)))
}

I am reassured that EVM/ETH instant observations and actual observations with the same Coins and different Gas would still have the same CacheHash,
thus my impression is that this does not risk any changed behaviour in the form of a transaction hash being pointed at a different CacheHash and leaving the earlier CacheHash unremovable.