Skip to content

Inbounds prematurely increase THORNode vault balances before confirmation counting

[2024-01-10 update: Please note near the bottom of the Issue for a specific example of (in one transaction) about 150 thousand RUNE's worth of LTC being swallowed by vault oversolvency without swapping or refunding due to this.]

Erring on the side of caution,
be quick to notice your balance may have decreased
and be slow to believe your balance has truly increased,
in case you treat it as true too early and then a blockchain re-org stops it from actually happening.

However, currently though handler_observed_txin.go waits until confirmation-counted ('finalised') consensus to act on an inbound memo,
it updates the vault balance immediately upon pre-confirmation-counting consensus.
https://gitlab.com/thorchain/thornode/-/blob/v1.125.3/x/thorchain/handler_observed_txin.go#L206-214

		if vault.IsAsgard() {
			if !voter.UpdatedVault {
				vault.AddFunds(tx.Tx.Coins)
				voter.UpdatedVault = true
			}
		}
		if voter.HasFinalised(activeNodeAccounts) {
			vault.InboundTxCount++
		}

This Issue proposes that this be changed to the below.

		if voter.HasFinalised(activeNodeAccounts) {
			if vault.IsAsgard() && !voter.UpdatedVault {
				vault.AddFunds(tx.Tx.Coins)
				voter.UpdatedVault = true
			}
			vault.InboundTxCount++
		}

Relevant:
https://gitlab.com/thorchain/thornode/-/blob/v1.125.3/bifrost/pkg/chainclients/ethereum/ethereum.go#L834-840
"in ETH PoS (post merge) reorgs are harder to do but can occur[...]Thus, the determination by thorsec, 9R and devs were to set the new min conf is 2."

Discord context (copied/paraphrsaed from below)
of the THOR block 14117897 SolvencyHaltETHChain that directly prompted the creation of this Issue:
https://discord.com/channels/838986635756044328/1192153792276344973/1193284462130241617

First, the Ethereum block scanner only allows fetching transactions from blocks, not a mempool.
https://gitlab.com/thorchain/thornode/-/blob/v1.125.3/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L169-172

// FetchMemPool get tx from mempool
func (e *ETHScanner) FetchMemPool(_ int64) (stypes.TxIn, error) {
    return stypes.TxIn{}, nil
}

https://etherscan.io/tx/0x5ce02fd082d17b6440e794eb11a5f8355e1d9a023f754672e3efbe86c401e052
ETH block 18925821, timestamp Jan-03-2024 09:03:59 AM +UTC.

ETH block that MsgSolvencies were reported for, one ETH block (12 seconds) earlier:
https://etherscan.io/block/18925820
Timestamp: Jan-03-2024 09:03:47 AM +UTC

THORChain consensus vault-balance-increasing from this transaction (one txs item):
https://thornode-v1.ninerealms.com/thorchain/tx/details/5ce02fd082d17b6440e794eb11a5f8355e1d9a023f754672e3efbe86c401e052?height=14117890

      "external_observed_height": 18925819,
      "external_confirmation_delay_height": 18925852,
  "consensus_height": 14117890,
  "updated_vault": true

That block's timestamp: https://thornode-v1.ninerealms.com/blocks/14117890
"time": "2024-01-03T09:03:33.386036493Z",
Timestamp in Etherscan terms: Jan-03-2024 09:03:33 AM +UTC.

That's 14 seconds earlier than the insolvency-claimed ETH block and 26 seconds earlier than the transaction's ETH block.

This isn't transaction instant-observation from THORChain's broadcasting nodes, it's supermajority observation by 2/3rds of all Active nodes.

No wonder then that Bifrosts concluded THORChain's ETH-block-18925820 ETH.XDEFI balance was insolvent, if THORNode's claimed the ETH.XDEFI balance had gone up from the transaction in ETH block 18925819, but the tranasction itself hadn't happened before ETH block 18925821.

SolvencyHaltETHChain was not automatically unhalted until ~4 minutes later in block 14117931 (09:08:01) upon reaching ETH block 18925840. \

In that block:
https://thornode-v1.ninerealms.com/thorchain/tx/details/5ce02fd082d17b6440e794eb11a5f8355e1d9a023f754672e3efbe86c401e052?height=14117931
Three txs items.
1st txs (mentioned above, two ETH blocks early):

      "external_observed_height": 18925819,
      "external_confirmation_delay_height": 18925852,

2nd txs (consistent with the chain-halting balance timing):

      "external_observed_height": 18925821,
      "external_confirmation_delay_height": 18925823,

3rd txs (finalised, consistent with the chain-halting balance timing):

      "external_observed_height": 18925823,
      "external_confirmation_delay_height": 18925823,

My impression is that because of the !voter.UpdatedVault conditional the code change would not result in any double-increase,
and because the increase is moved later rather than earlier it would not result in missing any increase
(even if a transaction's pre-confirmation-counting and finalised consensus heights were to be on either side of a network version change).

Even if the reverse situation were to occur
(e.g. higher wallet balance in ETH block 18925820, but pre-confirmation-counting consensus not until after ETH block 18925821),
there would be no solvency halt since the apparent situation would be temporary (unobserved-wallet-balance-funds) oversolvency rather than insolvency.

My specific code proposal: !3351 (merged)
'Increase THORNode vault balance only after inbound confirmation counting'.

2024-01-09 update:

Copying Son of Odin thoughts and my thoughts based on that
(on the effect of an InactiveVault refund attempt on funds already migrated from having already been recorded in a RetiringVault's balance):
https://discord.com/channels/838986635756044328/1193286662118178866/1194004754779619428

Copied text

If i remember correctly, the reason for this is to ensure that we dont set a vault into inactive status while funds are being sent to it. Adding to the vault prior to confs accomplishes this

Thank you for this context!

Was the RetiringVault actually held as a RetiringVault until the conf counting was finished, or were the funds send to an ActiveVault and the memo acted on upon finalisation?

Now that there are InactiveVault automatic (best try) refunds, is the below what happens in that situation?

  1. Funds are sent to RetiringVault; THORNode vault balance increases, Bifrosts start conf counting.

  2. RetiringVault migrates remaining funds to ActiveVault (including conf-counting received funds) and becomes InactiveVault.

  3. Bifrost scanners reach the conf-counting height and broadcast the finalised observations.

  4. Vault members see that the inbound vault was an InactiveVault and try to refund (recently-added code), but cannot since the funds were already sent to an ActiveVault.
    https://gitlab.com/thorchain/thornode/-/blob/v1.125.3/x/thorchain/handler_observed_txin.go#L243-254

        if !voter.HasFinalised(activeNodeAccounts) {
            ctx.Logger().Info("Tx has not been finalised yet , waiting for confirmation counting", "hash", voter.TxID)
            continue
        }

        if vault.Status == InactiveVault {
            ctx.Logger().Error("observed tx on inactive vault", "tx", tx.String())
            if newErr := refundTx(ctx, tx, h.mgr, CodeInvalidVault, "observed inbound tx to an inactive vault", ""); newErr != nil {
                ctx.Logger().Error("fail to refund", "error", newErr)
            }
            continue
        }
  1. Inbound refunds remain floating in ActiveVaults as oversolvency without being deposited or swapped through pools or otherwise acted on.

If so, it appears to me that THORNode vault balance only upon finalisation
could help ensure that migration-done-once-conf-counted inbound funds either get refunded or acted on,
rather than being swallowed without either done.
<curious>

2024-01-10 update:

Now added to the same Discord thread,
(from one transaction)
about 150 thousand RUNE's worth of LTC swallowed into vault oversolvency without swapping or refunding due to this--

A specific example of the above:

https://thornode-v1.ninerealms.com/thorchain/tx/details/8BE5CE25F3693B66CD1B578B25539D0F50B0AFF95024C9123DA56CD882A4CF34?height=13546535

          "asset": "LTC.LTC",
          "amount": "1200000000000"

According to
https://thornode-v1.ninerealms.com/thorchain/pool/LTC.LTC?height=13546535
that was worth
( ( 1200000000000 × 63082118525379 / 5102845409285 ) / 1e8 = )
~148,345 RUNE at the time.

    "observed_pub_key": "thorpub1addwnpepqgq0nwlpganjt57y6s7eksq8z48l33pmgtw8zupx3az65akhp88z2qq9rk7",
    "external_observed_height": 2582438,
    "external_confirmation_delay_height": 2584356,

1,918 LTC confirmation counting blocks.

  "actions": null,
  "out_txs": null,
  "consensus_height": 13500600,
  "updated_vault": true

Comparing the consensus height to the block I started with:
https://thornode-v1.ninerealms.com/blocks/13500600
2023-11-18
https://thornode-v1.ninerealms.com/blocks/13546535
2023-11-22
Four days (more than a churn, but way under the InactiveVault refund span of a month).

The inbound vault at that that consensus height:
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepqgq0nwlpganjt57y6s7eksq8z48l33pmgtw8zupx3az65akhp88z2qq9rk7?height=13500600

      "asset": "LTC.LTC",
      "amount": "2798372751104"

"status": "ActiveVault",

However, the same vault at the block I started with:
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepqgq0nwlpganjt57y6s7eksq8z48l33pmgtw8zupx3az65akhp88z2qq9rk7?height=13546535

      "asset": "LTC.LTC",
      "amount": "0"

One block later, the ObservedTxInVoter:
https://thornode-v1.ninerealms.com/thorchain/tx/details/8BE5CE25F3693B66CD1B578B25539D0F50B0AFF95024C9123DA56CD882A4CF34?height=13546536

    "external_observed_height": 2584356,
    "external_confirmation_delay_height": 2584356,
  "actions": [
    {
      "chain": "LTC",
      "to_address": "ltc1qtka23uma5mh8e4nue89mxl8tvtx9ah5r4ncz4z",
      "vault_pub_key": "thorpub1addwnpepqgq0nwlpganjt57y6s7eksq8z48l33pmgtw8zupx3az65akhp88z2qq9rk7",
      "coin": {
        "asset": "LTC.LTC",
        "amount": "1199998509470"
      },
      "memo": "REFUND:8BE5CE25F3693B66CD1B578B25539D0F50B0AFF95024C9123DA56CD882A4CF34",
      "max_gas": [
        {
          "asset": "LTC.LTC",
          "amount": "10875",
          "decimals": 8
        }
      ],
      "gas_rate": 43,
      "in_hash": "8BE5CE25F3693B66CD1B578B25539D0F50B0AFF95024C9123DA56CD882A4CF34"
    }
  ],
  "out_txs": null,
  "consensus_height": 13546536,
  "finalised_height": 13546536,
  "updated_vault": true,
  "outbound_height": 13547256

Same block (noting the 'reason):
https://thornode-v1.ninerealms.com/thorchain/block?height=13546536

          {
            "chain": "LTC",
            "code": "104",
            "coin": "1200000000000 LTC.LTC",
            "from": "ltc1qtka23uma5mh8e4nue89mxl8tvtx9ah5r4ncz4z",
            "id": "8BE5CE25F3693B66CD1B578B25539D0F50B0AFF95024C9123DA56CD882A4CF34",
            "memo": "=:BTC.BTC:bc1q4kq0r2ygpx8q57s90jzjuwxfnyu8xz2vtqzhzc:0/1/0:tr:0",
            "reason": "observed inbound tx to an inactive vault",
            "to": "ltc1qk8zc6950u59gw96upxr5na94pkmu4c8ykt896x",
            "type": "refund"
          }

Since the vault had 0 LTC, refunding wasn't possible.

A recent height (2024-01-09):
https://thornode-v1.ninerealms.com/thorchain/tx/details/8BE5CE25F3693B66CD1B578B25539D0F50B0AFF95024C9123DA56CD882A4CF34?height=14200000
"out_txs": null,

The inbound was indeed swallowed by the vaults
(migrated and not swapped)
without refunding because the vault balance was updated prior to confirmation counting
(by which time it had become an InactiveVault).

2024-02-04 update:

Regarding #1869 (closed)
'Solvency reports which do not reflect future-height supermajority-observed inbounds can trigger solvency halts',
SetLastChainHeight should be simultaneously moved to within a HasFinalised conditional to only update the height once THORChain is confident an inbound has in fact taken place for that height.
|
Edit: After looking closer, I mostly retract the above. SetLastChainHeight would then ideally reflect the block for which inbounds were confirmed,
but in
if err := h.mgr.Keeper().SetLastChainHeight(ctx, tx.Tx.Chain, tx.BlockHeight); err != nil {
the finalised-transaction tx.BlockHeight is the block in which it was finalised, not the block of which the inbounds had been finalised;
further, some observations may need confirmation counting and yet happen to only be broadcast once their confirmation counting is already complete,
leaving no record of their included block in the ObservedTxVoter.
|
SetLastChainHeight can then stay outside the HasFinalised conditional,
updating to what height node supermajority observations have reached whether confirmed or not,
but I do support moving it to before the IsAsgard check and its continue
so that InactiveVault inbounds also update the record of where node supermajority observations have reached.

2024-02-18:

As a minor note, this would also address the unintuitive behaviour in which a noop:novault inbound
increases the vault balance on pre-conf-count consensus and then decreases it again on post-conf-count consensus.

Edited by Multipartite