ETH observed txouts are generating undecremented slash points
Not to be confused with #1558 (closed)
(which referred to voter.Tx signers being used to decrement consensus signers, but not including all consensus signers),
this is based on this Discord discussion.
TLDR SUMMARY OF THE BELOW:
Outbounds can have non-zero ConfirmationRequired and be observed twice, yet
GetObservationsStdTx makes the first unfinalised observation final
and handler_observed_txout.go (unlike handler_observed_txin.go) cannot unslash for both pre- and post-confirmation-count observations,
so when this happens nodes are slashed and never unslashed for the second observation.
|
This is perhaps a fundamental problem, now brought to attention by
the minimum 2 blocks Ethereum confirmation count that incurs the problem for every Ethereum outbound.
[quote]
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/x/thorchain/handler_observed_txout.go#L127
} else if ctx.BlockHeight() <= (voter.FinalisedHeight+observeFlex) && voter.Tx.Equals(tx) {
That Equals, unlike getConsensusTx's transaction-only (height-ignoring) comparison, compares the heights too.
I imagine that if there were confirmation counting, but the post-count Txs reached consensus before pre-count Txs was fully signed,
then the last few pre-count signings wouldn't get their slash points decremented even if they were within the ObservationDelayFlexibility period.
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/x/thorchain/handler_observed_txin.go#L103-130
handler_observed_txin has logic for setting the voter.Tx both after and before finalisation (decrementing for each case),
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/x/thorchain/handler_observed_txout.go#L119-131
but handler_observed_txout only sets it once (after finalisation), decrementing slash points only for the ObservedTx matching that voter.Tx,
but as we've seen there are two distinct Txs (with the same signers) for the ETH outbound
D395EADC347DBDD6F0E3CF7067C2F610ADFD67EADF42D84A035350FC1910583D
.
[/quote]
I would like to ask for the preferred resolution, particularly from @heimdallthor,
whether for instance to have handler_observed_txin-like slash point decrementing both pre- and post-confirmation-counting,
and/or whether to alter something in the Bifrost code which currently seems to broadcast two IsFinal MsgObservedTxOuts.
(Please contrast with the ETH inbound D8EA8B40694A9659BF1164F8434750BE7D973735DB7839A943D9FE925848DA92, where one !IsFinal and one IsFinal MsgObservedTxIns appear to be broadcast.)
The above being an illustration of what happens,
the below is the logic flow of why it happens:
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/bifrost/pkg/chainclients/ethereum/ethereum_block_scanner.go#L169-172
I note there's no Mempool for Ethereum;
FetchMemPool returns an empty struct;
also, there are two instances in ethereum_blocK-scanner.go1 of Mempool:withfalse, and none with true`.
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/bifrost/observer/observe.go#L171
In sendDeck, deck.ConfirmationRequired is set to 2,
by calling GetConfirmationCount which calls getBlockRequiredConfirmation,
which returns a minimum of 2 here.
Next in sendDeck (after making newTxin which at this point will have SentUnFinalised false),
there's a ConfirmationCountReady check here.
ConfirmationCountReady returns false because the current block height isn't 2 blocks past the observation height. \
sendDeck then appends deck.TxArray... to newTxIn.TxArray,
and here calls chunkifyAndSendToThorchain before setting the SentUnFinalised field to true.
Here newTxIn is appended to newDeck (for setting here).
The transaction is then re-added to the deck without further action being taken (chainClient.ConfirmationCountReady false and then SentUnFinalised field true) until the block height is sufficient (that is, 2 blocks later) at which point chunkifyAndSendToThorchain is called again, this time with the SentUnFinalised field already being true.
To reiterate, for both chunkifyAndSendToThorchain calls the Mempool field is false.
(As a note, for chains with mempools like Bitcoin, observation from the mempool results in ConfirmationCountReady returning true, so it's removed from the deck with only a single chunkifyAndSendToThorchain call.)
chunkifyAndSendToThorchain calls signAndSendToThorchain.
signAndSendToThorchain gets ObservedTxs from the TxIn by calling getThorchainTxIns.
In getThorchainTxIns:
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/bifrost/observer/observe.go#L617-623
height := item.BlockHeight
if txIn.Finalised {
height += txIn.ConfirmationRequired
}
tx := stypes.NewObservedTx(
common.NewTx(txID, sender, to, item.Coins.NoneEmpty(), item.Gas, item.Memo),
height,That txIn.Finalised was set in chunkifyAndSendToThorchain by its last argument (a boolean).
Revisiting, the Finalised (ConfirmationRequired added) is true when and only when chainClient.ConfirmationCountReady was.
In NewObservedTx, the 'height' argument becomes the BlockHeight, namely adding the ConfirmationRequired to the BlockHeight to make it equal to the FinaliseHeight.
Now working with type ObservedTxs, signAndSendToThorchain calls GetObservationsStdTx (to get messages to broadcast),
and GetObservationsStdTx calls NewMsgObservedTxOut for the 'outbound' ObservedTxs.
Now we get to the point:
In GetObservationsStdTx, before the NewMsgObservedTxOut call:
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/bifrost/thorclient/thorchain.go#L313-316
} else if tx.Tx.FromAddress.Equals(obAddr) && !outbound.Contains(tx) {
// for outbound transaction , there is no need to do confirmation counting
tx.FinaliseHeight = tx.BlockHeight
outbound = append(outbound, tx)Thus, both observed Ethereum outbounds and inbounds are observed twice by nodes, and only with outbounds the first observation's FinaliseHeight is set to the BlockHeight,
and the second observation's BlockHeight is set to the FinaliseHeight.
|
BlockHeight and FinaliseHeight being equal count as finality, so upon consensus for the first observation THORChain takes action, and upon signing for the second observation observing nodes receive slash points which are never unslashed.
Notably, getBlockRequiredConfirmation called from GetConfirmationCount does distinguish between inbound and outbound transactions!
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/bifrost/pkg/chainclients/ethereum/ethereum.go#L808-814
asgards, err := c.getAsgardAddress()
if err != nil {
c.logger.Err(err).Msg("fail to get asgard addresses")
asgards = c.asgardAddresses
}
c.logger.Debug().Msgf("asgards: %+v", asgards)
totalTxValue := c.getTotalTransactionValue(txIn, asgards)In Client-received getTotalTransactionValue:
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/bifrost/pkg/chainclients/ethereum/ethereum.go#L771-779
fromAsgard := false
for _, fromAddress := range excludeFrom {
if strings.EqualFold(fromAddress.String(), item.Sender) {
fromAsgard = true
break
}
}
if fromAsgard {
continueThat said, since this is in a TxArray range, it is plausible that there could be inbound observations from the same block, in which case a non-zero ConfirmationRequired could be appropriate even for some outbounds.
(In other words, even without the minimum 2 confirmation count the same phenomenon could still occur.)
Summarising, a group of inbounds and outbounds from the same block might be sent (with chunkifyAndSendToThorchain) for observation twice,
but only the outbounds don't have to wait for confirmation counting,
and so nodes observing both times get extra slash points which are never unslashed.
|
This is perhaps a fundamental problem, but brought to attention by the Ethereum minimum confirmation count which ensures that even outbounds not in the same block as inbounds display this phenomenon
(instead of only being observed once due to ConfirmationRequired being 0).
A simple resolution could be to remove the
tx.FinaliseHeight = tx.BlockHeight
line for outbounds in GetObservationsStdTx;
[Edit: This perhaps wouldn't result in solvency issues as long as the vault was updated on the pre-confirmation-count consensus like handler_observed_txin.go,
but unlike handler_observed_txin.go,
including the HasFinalised later check of handler_observed_txin.go,
handler_observed_txout.go appears to have no code for unslashing for both pre-confirmation-count and after-confirmation-count, so such code would need to be added (to make it more like handler_observed_txin.go].
Another approach could be to put an earlier outbound check as part of
https://gitlab.com/thorchain/thornode/-/blob/v1.113.1/bifrost/observer/observe.go#L183
newTxIn.TxArray = append(newTxIn.TxArray, deck.TxArray...)
(within the if !chainClient.ConfirmationCountReady(deck) {),
so that outbounds in the TxArray weren't re-added for a second observation.
(This relies on that GetObservationsStdTx will later do the
tx.FinaliseHeight = tx.BlockHeight
line for outbounds.)
HOWEVER, it should be kept in mind that for internal transactions GetObservationsStdTx adds one ObservedTx to the inbounds and one (made final) to the outbounds;
it would be undesirable if an internal transaction's inbound side remained unfinalised because its second observation never happened.
(I will continue to think on this.)
Update:
Other suggestions remain welcome; at present I lean towards carrying out consensus actions immediately upon first consensus (the same as current code behaviour),
but unslashing a second time if there is a separate FinalisedHeight.
Specifically: !3165 (merged)
'Outbound actions still upon consensus height, but second unslash if a separate FinalisedHeight'.
Update:
Also the Bifrost portion of this, to be merged only after !3165 (merged) implemented (perhaps as a .1 patch), !3166 (closed)
'Don't make both pre- and post-confirmation MsgObservedTxOuts Final'.