Complete withdrawal from a gas asset pool results in pool BalanceAsset insolvency

(A full withdraw of a gas asset pool is unlikely for the deeper pools, but is perhaps worth testing in case it some day takes place for a shallower pool.)

Noticed in the context of #1506:

In this smoke test, the querier displays (and so the smoke test checks) pools with non-zero balances, warning that \

I[2023-04-11 05:46:14,553] 99 PROVIDER-1 => VAULT      [WITHDRAW:ETH.ETH] 0.00000001 THOR.RUNE
E[2023-04-11 05:46:28,511] Bad Pool-ETH.ETH balance: ASSET Sim 119544 != Real 240000

. The discrepancy between Python and Go behaviuor is a matter for #1506, but the ETH.ETH 240000 balance_asset is currently relevant.

Logging the address ETH.ETH balances in a smoke test without querier alteration:
https://gitlab.com/thorchain/thornode/-/jobs/4097967919

I[2023-04-12 06:04:50,449] 98 PROVIDER-1 => VAULT      [WITHDRAW:ETH.TKN-0X40BCD4DB8889A8BF0B1391D0C819DCD9627F9D0A] 0.00000001 THOR.RUNE
I[2023-04-12 06:05:09,256] USER-1 ETH.ETH balance:
I[2023-04-12 06:05:09,256] Mocknet   coin: 399,468,004,793.95166016 ETH.ETH
I[2023-04-12 06:05:09,256] Simulated coin: 399,468,004,793.95166016 ETH.ETH
I[2023-04-12 06:05:09,259] PROVIDER-1 ETH.ETH balance:
I[2023-04-12 06:05:09,259] Mocknet   coin: 396,418,416,781.24237061 ETH.ETH
I[2023-04-12 06:05:09,259] Simulated coin: 396,418,416,781.24237061 ETH.ETH
I[2023-04-12 06:05:09,261] VAULT ETH.ETH balance:
I[2023-04-12 06:05:09,261] Mocknet   coin: 4,044,295,900.00000000 ETH.ETH
I[2023-04-12 06:05:09,261] Simulated coin: 4,044,295,900.00000000 ETH.ETH
[...]
I[2023-04-12 06:05:09,319] 99 PROVIDER-1 => VAULT      [WITHDRAW:ETH.ETH] 0.00000001 THOR.RUNE
I[2023-04-12 06:05:24,185] USER-1 ETH.ETH balance:
I[2023-04-12 06:05:24,185] Mocknet   coin: 399,468,004,793.95166016 ETH.ETH
I[2023-04-12 06:05:24,185] Simulated coin: 399,468,004,793.95166016 ETH.ETH
I[2023-04-12 06:05:24,187] PROVIDER-1 ETH.ETH balance:
I[2023-04-12 06:05:24,188] Mocknet   coin: 400,438,712,681.24237061 ETH.ETH
I[2023-04-12 06:05:24,188] Simulated coin: 400,438,712,681.24237061 ETH.ETH
I[2023-04-12 06:05:24,190] VAULT ETH.ETH balance:
I[2023-04-12 06:05:24,190] Mocknet   coin: 12,045,600.00000000 ETH.ETH
I[2023-04-12 06:05:24,190] Simulated coin: 12,045,600.00000000 ETH.ETH
I[2023-04-12 06:05:24,246] [+] 0.40202959 ETH.ETH | Fee 0.00000000 ETH.ETH | Gas 0.00119544 ETH.ETH
I[2023-04-12 06:05:24,246] [+] 524.88643020 THOR.RUNE | Fee 0.02000000 THOR.RUNE | Gas 0.02000000 THOR.RUNE

In summary,
PROVIDER-1 goes up by 0.40202959 ETH.ETH (same as the outbound amount) from 39.64184167 to 40.04387126
and VAULT goes down by 40202959 + 119544 (outbound amount and gas) from 0.40442959 to 0.00120456 .
Because the VAULT balance has decreased by the outbound amount, the remaining (much smaller) amount is not a pending outbound amount,
and so should still be reflected in the ETH.ETH pool's balance_asset (even if it were small enough to be burnt in the next churn).

As seen above for the querier modification, the ETH.ETH pool balance_asset is indeed non-zero; however it is 240000, reflecting
0.00240000 ETH.ETH, not 0.00120456 ETH.ETH .

I do not yet know how this happens, but it currently appears to me to be the breaking of an invariant (something which should never happen).

Especially, this is a case where the pool balance_asset claims a larger balance than the vault balance backing it, namely pool insolvency rather than oversolvency.

In the near future I plan to check this behaviour in more detail with a regression test, rather than with smoke test logging.

Update:
A regression test is now here, job here. (All operations (checks) succeeded, but pools were insolvent at the end.)

Testing this also alerted me to my oversight here (the SafeSub arguments in the wrong order), for which this correction commit was appropriate. My apologies for not noticing this until now.

Odd aspects noted at this time (predicted worthy of fixing) about a full (symmetrical) withdraw from a gas asset pool:

The asset outbound indeed has max_gas of 1.5x the estimated gas, but the asset withheld from the outbound is also exactly 1.5x the estimated gas, not 3x. Relatedly(?), though asset "scheduled_outbound" events are emitted, no asset "fee" events are are emitted for them.
Gas asset pools start empty and Available and should always be Available even if they return to being empty, yet in the test the gas asset pools are observed to become Staged.
Pool balance_asset and pending outbound coin amount and vault balance are all consistent until the outbound is observed, at which point the gas is deducted from the vault balance and not from the pool balance_asset, breaking the invariant (and leaving the pool balance_asset insolvent).

Leaving notes for later:

This (1) looks to be the code that takes no asset fee if a pool has no units, even if it is a gas asset. Since a gas asset outbound should always withhold the outbound fee for vaults to pay the gas cost with, even the pool BalanceAsset is somehow zero, I currently propose the conditional instead be if pool.Asset.IsGasAsset() || (!pool.BalanceAsset.IsZero() && !pool.BalanceRune.IsZero()) { , treating the balances as the deciding factor rather than the units.

This (2) is the withdraw.go code that can set even a pool.Asset.IsGasAsset() pool to Staged.

This (3) looks to be the code that skips gas (SafeSub) subtraction from the pool when the gas pool's balance_rune (together with LP_units) is empty.
|
I currently propose that the runeGas.IsZero() check be included (with a !) in the conditional for the transfer from the Reserve,
and also (unrelated) that there be an if gas.Amount.GT(pool.BalanceAsset) { gas.Amount = pool.BalanceAsset } equivalent beforehand (both for runeGas calculation and for an accurate gasPool event later).
|
Regarding accuracy of the gasPool event, note that if an outbound transaction goes through when a pool doesn't have enough BalanceAsset yet is oversolvent (there being enough in the vaults), there's a choice to be made about whether to have a gasPool event that reports uncapped gas.Amount (while optionally a capped gasAsset is used for the runeGas calculation) or capped gas.Amount.
|
Rather than report more than was subtracted from the pool, my current inclination is to reimburse the pool for only the gas taken from it (rather than any higher amount taken from an unpooled vault amount), and use the gas event for only reporting the pool interactions.

Update:
This code in withdraw.go appears relevant for withholding 1.5x estimated gas when a gas pool is unitless.
Particularly given that a pool could still have depths (synth-backing) allowing outbound fee collection, as well as that external chain network fees could vary and require higher MaxGas,
my current inclination is to leave the gas amount deduction entirely to manager_txout_current (modifying (1)'s code) and taking out that section from withdraw.go .
|
[Edit: Related to this, the MinimumL1OutboundFeeUSD Mimir key's usage makes the BNB-specific fee code (handler_withdraw.go, withdraw.go?) effectively moot, which combines well with this simplification.] | [Further edit: RagnarokTx modifications to have zero Amount, to not increment pool Asset depths (since !2777 (merged)), also became necessary.]

Proposed commits:
https://gitlab.com/thorchain/thornode/-/commits/0f46c6230de5477b8762ec47e913365a931d404b

Passed pipelines for the intermediate commits:
(3) Regression test update (manager_gas_current commit)
(2) Regression test update (withdraw.go commit)
(1) Regression test update (manager_txout_current commit)

Edited Apr 15, 2023 by Multipartite