fix(utxo): avoid index out of bounds panics in tx_details_by_hash

[shamardy]

Partially fixes #1906

This PR addresses index out of bounds errors in the tx_details_by_hash functions. The changes replace direct array access with safer methods, avoiding potential panics due to out-of-range indices. In addition, error handling around the function was improved to log and skip transactions in case of errors.
Please note that while this PR doesn't fully resolve the issue outlined in https://github.com/KomodoPlatform/komodo-wallet-desktop/issues/2317, as the transactions causing panics during history fetching will be skipped instead. The PR will facilitate the pinpointing of the actual problem by logging the transactions that cause these issues.

[rozhkovdmitrii]

There are few places where transaction outputs are got: 1, 2, 3 etc...

I'm not sure, but it presumably can have a sense to make all these places safe if we're getting this error to prevent such class of errors in the future.

[caglaryucekaya]

LGTM, but I couldn't find test coverage for transaction_details_with_token_transfers for BCH. Is the test_cashaddresses_in_tx_details_by_hash test intended for it? It calls the tx_details_by_hash in utxo_common. Otherwise, I don't see any problem that could cause the panic, we should wait for it to happen again and use the tx hash that you logged now as you wrote.

[shamardy]

There are few places where transaction outputs are got: 1, 2, 3 etc...

I am aware of these places but thanks @rozhkovdmitrii for pointing it out :)
I wanted to do this fix quickly to include it in the release then work on those later since I also check where each function containing those are used and try to fix any other problems I find.

[shamardy]

but I couldn't find test coverage for transaction_details_with_token_transfers for BCH. Is the test_cashaddresses_in_tx_details_by_hash test intended for it? It calls the tx_details_by_hash in utxo_common.

it also calls UtxoTxHistoryOps::tx_details_by_hash

komodo-defi-framework/mm2src/coins/utxo/utxo_tests.rs

Line 1389 in 730aad8

let tx_details = get_tx_details_eq_for_both_versions(&coin, TX_HASH);

komodo-defi-framework/mm2src/coins/utxo/utxo_tests.rs

Line 139 in 730aad8

let tx_details_v2 = get_tx_details_by_hash_v2(coin, tx_hash, tx_details_v1.block_height, tx_details_v1.timestamp);

komodo-defi-framework/mm2src/coins/utxo/utxo_tests.rs

Line 128 in 730aad8

block_on(UtxoTxHistoryOps::tx_details_by_hash(coin, params)).unwrap()

Otherwise, I don't see any problem that could cause the panic, we should wait for it to happen again and use the tx hash that you logged now as you wrote.

Yeah, that is the intention of this PR, to prevent panic and to see what transactions were causing that in the future so that we can fix their deserializations if it were the problem.

[caglaryucekaya]

it also calls UtxoTxHistoryOps::tx_details_by_hash

Yes, but the coin created in that test is UtxoStandardCoin, not BchCoin, so it does not call the transaction_details_with_token_transfers. It calls the tx_details_by_hash in utxo_common.

[shamardy]

Yes, but the coin created in that test is UtxoStandardCoin, not BchCoin, so it does not call the transaction_details_with_token_transfers. It calls the tx_details_by_hash in utxo_common.

Yeah, I missed that. Thanks for pointing it out. The changes here don't change anything of how the logic works though, it only returns errors where it's necessary, so no need to cover that in this PR.

[caglaryucekaya]

Yeah, I missed that. Thanks for pointing it out. The changes here don't change anything of how the logic works though, it only returns errors where it's necessary, so no need to cover that in this PR.

Sure, I approved the PR and opened an issue for this instead

[Alrighttt]

My only feedback is that we might want to consider logging the full tx hex instead of the txid. I didn't trace where this is actually used, but if it's possible we are dealing with unconfirmed transactions, we may end up logging a txid that we can't retrieve the full hex for at a later time.

[shamardy]

My only feedback is that we might want to consider logging the full tx hex instead of the txid.

I will add hex logging in addition to txid. Thanks for the feedback.

[smk762]

Another user reported same issue, details below

Reported by user jommy99 via discord on behalf of a Brazilian member of the PND community.

User experiences loss of connection to mm2 when activating PND. Issue persists in all current releases for all GUIs using 1.0.5 1d8bebd15 SDK.

mm2 log file ends with

· 2023-08-15 07:54:39 -0700 [tx_history coin=PND] utxo_common:2913] Error "utxo_common:3112] utxo_common:3241] UnexpectedEnd, tx: 0f64cb2d216fb1be996a83283a2ae07ca236578fe45f7e61be0dce390f192211" on getting the details of 904b5d62dad92563b7ecd7c8fdf97c0e0d622e5cf2afbe5a658bad580ae4dff8, skipping the tx
15 14:54:39, common:500] panicked at 'index out of bounds: the len is 1 but the index is 1', mm2src\coins\utxo\utxo_common.rs:3119:29
15 14:54:39, common:501] backtrace
  ??:0] BaseThreadInitThunk 0x7fff0a4784d3
  ??:0] RtlUserThreadStart 0x7fff0cdc1790

Transaction mentioned in logs do not appear to be "exotic"
https://chainz.cryptoid.info/pnd/api.dws?q=txinfo&t=904b5d62dad92563b7ecd7c8fdf97c0e0d622e5cf2afbe5a658bad580ae4dff8
https://chainz.cryptoid.info/pnd/api.dws?q=txinfo&t=0f64cb2d216fb1be996a83283a2ae07ca236578fe45f7e61be0dce390f192211

** Full Logs:**
2023-08-15-14-53-48.log
2023-08-15-14-54-34.mm2.log

A test branch in legacy desktop is building at https://github.com/KomodoPlatform/komodo-wallet-desktop/actions/runs/5876114420 to confirm the fix

[shamardy]

Transaction mentioned in logs do not appear to be "exotic"
https://chainz.cryptoid.info/pnd/api.dws?q=txinfo&t=904b5d62dad92563b7ecd7c8fdf97c0e0d622e5cf2afbe5a658bad580ae4dff8
https://chainz.cryptoid.info/pnd/api.dws?q=txinfo&t=0f64cb2d216fb1be996a83283a2ae07ca236578fe45f7e61be0dce390f192211

These transactions are not the one that caused index out of bounds but the one after them, 1.0.6 will log the one that caused the panic though without causing panics, it will also not be included in history same as the above transactions. But this is still good info since now I know one of the coins that have problems with transaction deserialization and some examples of transactions that couldn't be deserialized properly.
P.S. the one that caused the panic will not be exotic too, due to deserialization problems the output index is getting messed up.