Are some nodes wrongly keeping slash points for transactions they have correctly observed?

Example:
https://thornode-v1.ninerealms.com/thorchain/tx/details/197b4a48f0a69973f7c5f504548872b946483c1b431c8a7981b789443b115c7f?height=11268854
The finalised_height is block 11268754, 100 blocks earlier.
In this block there were 87 Active nodes, so the consensus threshold was 58.
It can be seen that the consensus Tx lists 42 signers (below the consensus threshold) and has external_confirmation_delay_height 17470985, that the first Txs has 43 signers and the same external_confirmation_delay_height,
and the second Txs has 73 signers and external_confirmation_delay_height 17470987 (two blocks later).
Node .9426 as an example is listed only in the second Txs.

One block before the finalised_height: https://thornode-v1.ninerealms.com/thorchain/tx/details/197b4a48f0a69973f7c5f504548872b946483c1b431c8a7981b789443b115c7f?height=11268753
.9426 is already listed in the second Txs.

Slash points are decremented once finalisation has taken place--here, in or after block 11268754.
https://gitlab.com/thorchain/thornode/-/blob/v1.111.1/x/thorchain/handler_observed_txout.go#L117-129

	if voter.HasFinalised(nas) {
		if voter.FinalisedHeight == 0 {
			ok = true
			voter.FinalisedHeight = ctx.BlockHeight()
			voter.Tx = voter.GetTx(nas)
			// tx has consensus now, so decrease the slashing point for all the signers whom voted for it
			h.mgr.Slasher().DecSlashPoints(slashCtx, observeSlashPoints, voter.Tx.GetSigners()...)

		} else if ctx.BlockHeight() <= (voter.FinalisedHeight+observeFlex) && voter.Tx.Equals(tx) {
			// event the tx had been processed , given the signer just a bit late , so we still take away their slash points
			h.mgr.Slasher().DecSlashPoints(slashCtx, observeSlashPoints, signer)
		}
	}

This means that when finalisation occurs, all the signers of voter.Tx have their slash points decremented,
and then any signers later than that within constants.ObservationDelayFlexibility also have their slash points decremented.

However, what of .9426?

HasFinalised as well as GetTx (which sets voter.Tx) refer to getConsensusTx.
https://gitlab.com/thorchain/thornode/-/blob/v1.111.1/x/thorchain/types/type_observed_tx.go#L343-363

	for _, txFinal := range m.Txs {
		voters := make(map[string]bool)
		if txFinal.IsFinal() != final {
			continue
		}
		for _, txIn := range m.Txs {
			if txIn.IsFinal() != final {
				continue
			}
			if !txFinal.Tx.EqualsEx(txIn.Tx) {
				continue
			}
			for _, signer := range txIn.GetSigners() {
				_, exist := voters[signer.String()]
				if !exist && accounts.IsNodeKeys(signer) {
					voters[signer.String()] = true
				}
			}
		}
		if HasSuperMajority(len(voters), len(accounts)) {
			return txFinal

This means that the signers in the second Txs (satisfying EqualsEx) count towards the signers that qualify the first Txs to be counted as reaching consensus,
but only the signers of the first Txs are included in the consensus Tx to have their slash points decremented.

This at present to me appears unintentional/undesirable.

To resolve this, noting that SetDone and AddOutTx possess versioning, I would like to suggest either that getConsensusTx be versioned so as to include all signers that have contributed to consensus (which should be all signers of all matching Txs when the consensus message is being processed),
perhaps by clearing the txFinal's signers and adding to them at the same time as the voters[signer.String()] = true,
or that a new getConsensusSigners function be made which uses similar logic to get a list of signers,
to be used in place of voter.Tx.GetSigners() when initially decrementing slash points.

Further notes:
I believe this applies equally to handler_observed_txin.go .
|
This Issue is not resolved by the unrelated !2722 (merged) 'Slash non-signers upon consensus',
and I currently suggest addressing it with a separate MR rather than adding to !2722 (merged).