External chain withdraw-initiator transactions' coins are adding to vault solvency without being added to their pool
This was something noted while doing an overhaul of the SQL query 'Coinsource ver12' to make the more accurate 'Coinsource ver13'.
Example:
https://midgard.thorchain.info/v2/debug/block/7374663
(2022-09-17, network version 1.96.1)
|
For BTC.BTC:
"rewards":
RUNE to the pool (no Asset change)
"withdraw":
"coin": "10001 BTC.BTC"
"emit_asset": "933296"
"fee":
"57000 BTC.BTC"
"scheduled_outbound":
"coin_amount": "876296"
933296 to be emitted from the position, 57000 withheld as the outbound fee, giving exactly the 876296 scheduled_outbound amount.
https://thornode.ninerealms.com/thorchain/pool/BTC.BTC?height=7374662
balance_asset "75522646076"
https://thornode.ninerealms.com/thorchain/pool/BTC.BTC?height=7374663
balance_asset "75521769780"
|
A pool balance change of -876296, exactly matching the scheduled_outbound amount.
https://thornode.ninerealms.com/thorchain/tx/8D6405BF080587BC79B53096AF9AB4D446164AF4342E40FABED31B78C3FC2783/signers
to_address "bc1q8nmnawzzngd4uxk7vde5v6ewgut2ffpfa5cd2c"
https://thornode.ninerealms.com/thorchain/vaults/asgard?height=7374662
asset "BTC.BTC"
amount "15309656165"
https://thornode.ninerealms.com/thorchain/vaults/asgard?height=7374663
asset "BTC.BTC"
amount "15309666166"
|
An increase of exactly 10001.
In summary:
A vault receives 10001 Asset,
schedules an outbound of 876296 Asset,
and deducts 876296 from the pool's Asset depth.
The vault holdings go up by 10001 which is unreflected by the pool.
(A minor thing, but with room for improvement.)
((Also, my apologies if I have unwittingly duplicated another's Issue.))
Considerations:
For context, the 10001 in this example (only needing to be 10000 as of !2444 (merged) in network version 1.95.0, from block 6967341 (2022-08-20) onwards)
is to reach the minimum observable amount for the BTC chain.
While it is tempting to add the withdraw-initiator amount directly to the scheduled_ _outbound amount,
as asymmetric withdrawal can be specified for the opposite side of a pool from the initiator transaction,
checking for this would be extra complexity and returning only when not the case would be slightly unfair.
|
Similarly, extra RUNE received in this manner (as part of a withdraw-initiator transaction) is currently sent to the Reserve (similar reasoning applicable),
so keeping the small amount of observation-requirement asset is consistent for the network.
Something to double-check is the Summer 2021 occurrence where coins were treated as being received without being actually received.
While my impression is that this has been corrected, any code change should be done with full understanding of how that correction was done,
in order to be confident that the correction is not being unknowingly reversed.
|
(Note balance/solvency checks carried out for vault addresses.)
Where something would be added (taking care to add the received asset to the pool which matches it):
(Or assets, going through a range with the common.RuneAsset() checked before a GetPool and BalanceAsset incrementation and SetPool?)
https://gitlab.com/thorchain/thornode/-/blob/v1.96.2/x/thorchain/handler_withdraw.go#L247-254
// any extra rune in the transaction will be donated to reserve
reserveCoin := msg.Tx.Coins.GetCoin(common.RuneAsset())
if !reserveCoin.IsEmpty() {
if err := h.mgr.Keeper().AddPoolFeeToReserve(ctx, reserveCoin.Amount); err != nil {
ctx.Logger().Error("fail to add fee to reserve", "error", err)
return nil, err
}
}Updating with newer thoughts:
As a reason for not changing the RUNE approach to add to the pool rather than the Reserve,
I note that for a gas asset withdraw transaction for non-gas-asset, the gas asset wouldn't go into the withdrawn pool
(thus asymmetrical if withdraw Asset goes into its own pool and withdraw RUNE goes into the withdrawn pool,
compared to Asset into its pool and RUNE into the Reserve).
|
Since typically expected to be dust, trying to swap gas asset to RUNE for the withdraw-pool seems wastefully inefficient (processing time and event emission) compared to just updating that gas asset pool's BalanceAsset as though a donate.(No new events needed; plausibly Midgard might need an update to record BalanceAsset changes from withdraw events after the change?)