Outbounds assigned to retiring vaults during churn are dropped if the vault retires first

When rescheduled, we drop outbounds from retired vaults: https://gitlab.com/thorchain/thornode/-/blob/v1.127.1/x/thorchain/manager_slasher_current.go?ref_type=tags#L352-357

Example from most recent churn:

2024-02-05T21:30:28-05:00 INF gitlab.com/thorchain/thornode/x/thorchain/manager_slasher_current.go:355 > cannot reassign tx from inactive vault hash=33D96F335F4C0F433AB5AEA9DA5C9BB8AFCE7512949ACAA56BC92CAB777CA4B4
2024-02-04T06:05:23-05:00 INF gitlab.com/thorchain/thornode/x/thorchain/manager_slasher_current.go:355 > cannot reassign tx from inactive vault hash=678665570C21B35536C413B71083B3638A23D91CBC458FB93B852AAFEA006D04

This results in dropping valid outbounds on the floor. We should determine why this was necessary and if it can be removed or worked around.

added priority0 label

I'm putting my initial thoughts (including concurrency implications) in a collapsible session at the bottom
as I now think this Issue's premise has a misstep:
RetiringVaults can't become InactiveVaults while they have an outbound,
because they can only become InactiveVaults after they've sent out all their funds.

---

[Second edit: Looking closer, these two examples aren't even the below--they appear to be true InactiveVault inbounds that started confirmation counting more than two days after the vaults became InactiveVaults,
not during-churn inbounds.]
|
This instead sounds like the existing #1820 (closed)
'Inbounds prematurely increase THORNode vault balances before confirmation counting'
(with an example of swallowed LTC worth ~150,000 RUNE)
|
in which the pre-confirmation inbound increases the vault balance,
THORChain tries (and succeeds) to migrate the inbound it isn't sure has happened yet,
the no-funds vault becomes an InactiveVault,
the confirmation count finishes and THORChain is now sure the inbound has happened,
since it's an InactiveVault it tries to refund it
(rather than attempting a gas-asset-only gas-asset-costing migrate to an ActiveVault and then acting on the memo with reduced Coins, logistically painful)
and the refund fails because the vault doesn't have the funds any more.
|
(I note 2024-01-07's unmerged !3351 (merged) 'Increase THORNode vault balance only after inbound confirmation counting'.)

---

Checking the examples above:
|
https://thornode-v1.ninerealms.com/thorchain/tx/details/33D96F335F4C0F433AB5AEA9DA5C9BB8AFCE7512949ACAA56BC92CAB777CA4B4?height=14584727
No supermajority consensus, finalised or unfinalised. (One txs, 32 signers.)
https://thornode-v1.ninerealms.com/thorchain/tx/details/33D96F335F4C0F433AB5AEA9DA5C9BB8AFCE7512949ACAA56BC92CAB777CA4B4?height=14584728

    "observed_pub_key": "thorpub1addwnpepqgkqux9wqyadksxquuh83tkjp5jyjen673yyu8aud00cv2vhy8kacv8p9yw",
    "external_observed_height": 19166081,
    "external_confirmation_delay_height": 19166083,

"consensus_height": 14584728,
(unfinalised consensus)
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepqgkqux9wqyadksxquuh83tkjp5jyjen673yyu8aud00cv2vhy8kacv8p9yw?height=14584728

  "status": "InactiveVault",
  "status_since": 14479777,

This was a true after-churn InactiveVault inbound and refund attempt,
after having already been an InactiveVault for 104,951 blocks (about 7.29 days).
|
https://thornode-v1.ninerealms.com/thorchain/tx/details/678665570C21B35536C413B71083B3638A23D91CBC458FB93B852AAFEA006D04?height=14561634
No consensus; one txs, 69 signers. (69/104 = 66.3%)
https://thornode-v1.ninerealms.com/thorchain/tx/details/678665570C21B35536C413B71083B3638A23D91CBC458FB93B852AAFEA006D04?height=14561635

    "observed_pub_key": "thorpub1addwnpepqfuz5vr7g9c9nezaceqqkjzel9ls4jyzxgqpzgfyw5qm5yjsr8faz5y04pm",
    "external_observed_height": 828875,
    "external_confirmation_delay_height": 828875,

"consensus_height": 14561635, (finalised consensus)
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepqfuz5vr7g9c9nezaceqqkjzel9ls4jyzxgqpzgfyw5qm5yjsr8faz5y04pm?height=14561635

  "status": "InactiveVault",
  "status_since": 14521844,

This was a true after-churn InactiveVault inbound and refund attempt,
after having already been an InactiveVault for 39,791 blocks (about 2.76 days).

---

Collapsible section initial thoughts, including pipeline thoughts on TSS burden without concurrency or unsignable-item-blocking-all-other-InactiveVault-outbounds if with concurrency

(Particularly unfortunate if the Migrates which finished the vault retirement had also contributed to TSS burden which made rescheduling necessary, though depending on timing this might never be the case.)

I hypothesise that the intent was to avoid nodes being overwhelmed by TSS for rescheduled outbounds from many different InactiveVaults
(for instance if someone were to send just-over-dust to every InactiveVault of every chain for the past month,
maybe every node trying to do TSS signing of 80 outbounds at once);

v1.127.0's !3372 (merged) 'Pipeline Signing Routines' should hopefully be preventing that situation,
but I'd like to ask whether you think a single unsignable InactiveVault outbound
(for instance a refund from the oldest InactiveVault and not having 2/3rds members online)
might block all other InactiveVault outbounds until it were dropped from MaxOutboundAttempts.

SpawnSignings I think(/hope?) would always go through the InactiveVault items in the same order;
that way all available nodes for the 'earliest ranked' item would all try to sign it,
instead of (for instance) half trying to sign that and half trying to sign another that they were a part of (but the first group weren't).
|
There might be a third group which tried and failed to sign something that they and the second group were members of and the first group weren't,
but that signing couldn't take place until signing attempts for the earliest-ranked item in the list ended.
|
(Which returns to the matter of when the earliest-ranked item is an old unsignable one,
and none of the RetiringVault->InactiveVault outbounds can be signed by nodes which were members for it until it's dropped.)

---

This is plausibly a bad(/worse-than-ideal) suggestion, but LackSigning could choose to only drop-without-rescheduling the TxOutItem if it were a Refund? \

Refunds from RetiringVaults would have been inbounds to those same vaults,
but arguably by the point that something needing a refund has been sent to a RetiringVault rather than an ActiveVault something maybe have gone wrong on the user's side anyway. Hmm.

Checking those two example hashes:
https://thornode-v1.ninerealms.com/thorchain/tx/details/33D96F335F4C0F433AB5AEA9DA5C9BB8AFCE7512949ACAA56BC92CAB777CA4B4
"REFUND:33D96F335F4C0F433AB5AEA9DA5C9BB8AFCE7512949ACAA56BC92CAB777CA4B4"
https://thornode-v1.ninerealms.com/thorchain/tx/details/678665570C21B35536C413B71083B3638A23D91CBC458FB93B852AAFEA006D04
"REFUND:678665570C21B35536C413B71083B3638A23D91CBC458FB93B852AAFEA006D04"
Refund-filtering wouldn't have helped them, then (but might still be worth considering?);
|
that is to say, they weren't cases of THORChain assigning a successful outbound to a RetiringVault (because no ActiveVaults had enough funds) and then the outbound remaining assigned to that vault without success or ActiveVault reschedule until the vault became an InactiveVault.

---

[Edit: Collapsible section now made; a GetVault would be meaningless,
as if it were a RetiringVault (I think) it would stay a RetiringVault until the rescheduled outbound succeeded or were dropped.]
Based on the above, another(/a complementary) plausibly-bad suggestion:
In refundTx, do a GetVault and don't do the
https://gitlab.com/thorchain/thornode/-/blob/v1.127.0/x/thorchain/helpers.go#L71
VaultPubKey: tx.ObservedPubKey,
if the vault isn't an InactiveVault.
(The ideal being that an about-to-retire RetiringVault inbound is--wait.
[Edit: Collapsible section now made.]

mentioned in issue #1876 (closed)

added to epic &4

marked this issue as related to #1826 (closed)

added milestone-planning label

removed from epic &4

My bad on the false positive, nothing to do here.

closed

Reopening as I believe we now have a true positive example:

https://thornode-v1.ninerealms.com/thorchain/queue/outbound?height=14521269
OUT:CA18 (and thus locked into it by REFUND:CA18, since before v1.128.0's !3420 (merged)).
23366592519 ETH.SNX outbound amount.

https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepqtxx2msyh358gg4f8smcftqwn78jm4wncd49y295dnfvk4vxx49w2n55vz5?height=14521269
543417042332 ETH.SNX available (plenty).

https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepqtxx2msyh358gg4f8smcftqwn78jm4wncd49y295dnfvk4vxx49w2n55vz5?height=14627719

      "asset": "ETH.SNX-0XC011A73EE8576FB46F5E1C5751CA3B9FE0AF2A6F",
      "amount": "0"

      "asset": "DOGE.DOGE",
      "amount": "713520899074713"

"status": "RetiringVault",
https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepqtxx2msyh358gg4f8smcftqwn78jm4wncd49y295dnfvk4vxx49w2n55vz5?height=14627720

      "asset": "ETH.SNX-0XC011A73EE8576FB46F5E1C5751CA3B9FE0AF2A6F",
      "amount": "0"

      "asset": "DOGE.DOGE",
      "amount": "0"

"status": "InactiveVault",

It looks like all the ETH.SNX was migrated, ignoring that some of its funds were currently spoken for.

What should have happened (I believe) is that the migrate try to migrate only the unspoken-for ETH.SNX until the outbound was either fulfilled or reassigned to an ActiveVault (not possible in this case),
but it seems the migrate process ignored pending outbounds.

Notably, here for LenPendingTxBlockHeights only refers to pending migrates, not other pending outbounds.

The !vault.HasFunds() check should be done before any outbound deduction.

Unlike the txout manager (which only checks the TxOutItem asset), preferable to do all the deductions from the retiring vaults once,
for every asset. perhaps using a map of vault PubKey to RetiringVault

Perhaps at
https://gitlab.com/thorchain/thornode/-/blob/v1.127.0/x/thorchain/manager_network_current.go#L443 for _, coin := range vault.Coins {
used for
https://gitlab.com/thorchain/thornode/-/blob/v1.127.0/x/thorchain/manager_network_current.go#L527
amt := coin.Amount
, using a availableCoins slice instead, made with common.NewCoins(vault.Coins...)
and then deducted with a PubKey -> Coins map (with continue if !ok) to SafeSub TxOutItem amounts one by one.

Care is needed regarding the burn-dust step:
https://gitlab.com/thorchain/thornode/-/blob/v1.127.0/x/thorchain/manager_network_current.go#L558-579
Perhaps there an explicit confirmation of that coin.Amount and the same-Asset Coin in the vault are the same amount?

Though tempting to postpone all migrations until there are no pending outbounds (simpler),
the best case scenario for that is that migrations don't start until outbounds have all been reassigned to ActiveVaults that have had no migrations yet
(behaviour I'd fairly-strongly prefer to avoid).

Incidentally, a different (more succinct?) summary for the outbound itself:

https://thornode-v1.ninerealms.com/thorchain/queue/outbound?height=14627719

    "vault_pub_key": "thorpub1addwnpepqtxx2msyh358gg4f8smcftqwn78jm4wncd49y295dnfvk4vxx49w2n55vz5",
    "coin": {
      "asset": "ETH.SNX-0XC011A73EE8576FB46F5E1C5751CA3B9FE0AF2A6F",
      "amount": "23366592519"
    },
    "memo": "OUT:CA184C5590749607482F7D253D513E97513D7030E14BFC4951C6939AC527687C",

https://thornode-v1.ninerealms.com/thorchain/vault/thorpub1addwnpepqtxx2msyh358gg4f8smcftqwn78jm4wncd49y295dnfvk4vxx49w2n55vz5?height=14627719
"status": "RetiringVault",
201 blocks later:

3:28AM INF gitlab.com/thorchain/thornode/x/thorchain/manager_slasher_current.go:355 > cannot reassign tx from inactive vault hash=CA184C5590749607482F7D253D513E97513D7030E14BFC4951C6939AC527687C
[...]
3:28AM INF github.com/tendermint/tendermint@v0.34.14/state/execution.go:235 > committed state app_hash=0E22AA1EF88E55CAB2E1A7CE275AF3F021EA64132A1093BF8B6D866F91323761 height=14627920 module=state num_txs=16

Now submitted:
!3435 (merged)
'Migrate only funds not spoken for by pending outbounds'.

Ahhh yes this is what I saw originally, I had pulled bad example transactions assuming they were the same cause as earlier cases.

reopened

mentioned in merge request !3435 (merged)

added to epic &4

closed with merge request !3435 (merged)

changed milestone to %Release-1.129.0

removed milestone-planning label