[Bug] !3148 (EVM insta observe at tx broadcast) ObservedTx ToAddress is Router rather than Destination (thus Stagenet bond-slashes for full txout amount)
Stagenet example:
https://stagenet-thornode.ninerealms.com/thorchain/vaults/asgard?height=9697716
"chain": "ETH",
"router": "0xB11a1735C2e3BCC5FC8c1d147fb64629d3d0caC5"
https://stagenet-thornode.ninerealms.com/thorchain/queue/outbound?height=9697716
"to_address": "0x3021c479f7f8c9f1d5c7d8523ba5e22c0bcb5430",
https://stagenet-thornode.ninerealms.com/thorchain/tx/details/491f5dd3dea9cb20baf02e3ec9fa1209d31595235af75d1d1ccf08b897918d53
"to_address": "0xB11a1735C2e3BCC5FC8c1d147fb64629d3d0caC5",
"coins": [
{
"asset": "ETH.ETH",
"amount": "315810"
}
],
"gas": [
{
"asset": "ETH.ETH",
"amount": "240000"
}
],
https://stagenet-midgard.ninerealms.com/v2/debug/block/9697716
{
"attributes": {
"ETH.ETH": "-555810",
"THOR.RUNE": "558012669",
"pool": "ETH.ETH"
},
"type": "slash"
}
The nodes were bond-slashed (five "bond" BondCost events) for the full observed amount because the observation didn't match a queue TxOutItem.
Many blocks later:
https://stagenet-thornode.ninerealms.com/thorchain/queue/outbound?height=9777800
The to_address
0x..5430 outbound is still in the queue.
https://stagenet-thornode.ninerealms.com/thorchain/tx/details/359A6E48D4A1D58DF96B4CB5052F9FD0FCDC9C157AB4A63D2ECA4AF5D3C6B407?height=9777800
The inbound still has a dangling actions item for to_address
0x..5430
.
For a Mainnet instance:
https://thornode-v1.ninerealms.com/thorchain/tx/details/31f20e07e0d8cb2823ecd8d191b1ccf964747200591db5ee3431a95410c966b3
Of 93 Active nodes, 13 (2/3rds of a vault's membership) observed for to_address
0x..7146 (the router) and 79 (all other Active nodes except for .lwlp) observed for to_address
0x..B605 .
Relevant code:
https://gitlab.com/thorchain/thornode/-/blob/v1.121.1/bifrost/pkg/chainclients/ethereum/ethereum.go#L613-627
txIn = stypes.NewTxInItem(
chainHeight+1,
signedTx.Hash().Hex()[2:],
tx.Memo,
fromAddr.String(),
createdTx.To().String(),
common.NewCoins(
coin,
),
gas,
tx.VaultPubKey,
"",
"",
nil,
)
Specifically:
createdTx.To().String(),
.
Though the createdTx creation takes the form of
https://gitlab.com/thorchain/thornode/-/blob/v1.121.1/bifrost/pkg/chainclients/ethereum/ethereum.go#L529
createdTx := etypes.NewTransaction(nonce, ecommon.HexToAddress(contractAddr.String()), estimatedETHValue, MaxContractGas, gasRate, data)
, the TxOutItem-matching destination address relates to the 'data' part rather than the 'contractAddr' part.
Specifically ('dest' used later in making of 'data'):
https://gitlab.com/thorchain/thornode/-/blob/v1.121.1/bifrost/pkg/chainclients/ethereum/ethereum.go#L401
dest := ecommon.HexToAddress(tx.ToAddress.String())
I thus propose that the createdTx.To().String()
be replaced by tx.ToAddress.String()
.
(If I have misunderstood what would this would do, correction is welcome.)
( @the_eridanus )
More specifically: !3174 (merged)
'[Version-unspecific; bugfix] Correct EVM-insta-observe ToAddress'.
As a further comment on the Overview of !3148 (merged): \
the nodes that observe this will be slashed once the real outbound is observed
My interpretation of this was that the signing nodes would keep observation slash points
(one block's lost rewards for each EVM txout signed),
something to perhaps seek a resolution for in future (for instance after !2722 / !3165 / !3166 (closed) processed).
if txOutItem.InHash.Equals(inTxID) &&
txOutItem.OutHash.IsEmpty() &&
tx.Tx.Chain.Equals(txOutItem.Chain) &&
tx.Tx.ToAddress.Equals(txOutItem.ToAddress) &&
strings.EqualFold(tx.Aggregator, txOutItem.Aggregator) &&
strings.EqualFold(tx.AggregatorTarget, txOutItem.AggregatorTargetAsset) &&
tx.ObservedPubKey.Equals(txOutItem.VaultPubKey) {
matchCoin := tx.Tx.Coins.EqualsEx(common.Coins{txOutItem.Coin})
if txOutItem.Chain.Equals(common.ETHChain) {
maxGasAmount := txOutItem.MaxGas.ToCoins().GetCoin(common.ETHAsset).Amount
gasAmount := tx.Tx.Gas.ToCoins().GetCoin(common.ETHAsset).Amount
// If thornode instruct bifrost to spend more than MaxETHGas , then it should not be slashed.
if gasAmount.GTE(cosmos.NewUint(constants.MaxETHGas)) && maxGasAmount.LT(cosmos.NewUint(constants.MaxETHGas)) {
ctx.Logger().Info("ETH chain transaction spend more than MaxETHGas should be slashed", "gas", gasAmount.String(), "max gas", constants.MaxETHGas)
matchCoin = false
}
}
As long as the exact Coins Amount and fields like the ToAddress match, then even for single-vault Stagenet the result should be vault oversolvency
(THORChain thinking it's spent the MaxGas rather than less--ideally never insolvency, though I should check later?)
rather than bond slashes for the main TxOutItem Amount.
(Other chains, as long as they have the right Coins Amount, can have gas arbitrarily large relative to the MaxGas and still count as legitimate--perhaps something to address in a separate Issue?)
[Edit: #1711/!3175 now submitted for that, not to be confused with this Issue's !3174 (merged).]