Remove `isNew` condition in FinalizedInbound check

[lumtis]

In this line:

node/x/crosschain/keeper/msg_server_vote_inbound_tx.go

Line 84 in 6e8dfee

if isNew {

This condition removes the check as a condition for double spend if similar inbounds producing difference digests are observed in a short timeframe. The first inbound will not be finalized before the second one get observed.

All votes for new inbounds should be blocked as soon as a inbound with the same data is finalized.

The condition has originally been left to ensure all observers can cast their vote for an inbounds, even if they do it after the inbound get the 2/3 votes need to finalize.
We should investigate an alternative to make the check stricter:
Example: still allowing vote if the associated ballot was already finalized before vote, this means the inbound has been observed, casting more votes has no implication.

VoteOnInboundBallot currently return if the vote is finalized at all. This doesn't tell if it just got finalized in the current vote.

finalized, isNew, err := k.zetaObserverKeeper.VoteOnInboundBallot

The function could return the information if the inbound just got finalized or was already finalized:

justFinalized, wasFinalized, isNew, err := k.zetaObserverKeeper.VoteOnInboundBallot

If the inbound was already finalized, we can skip the check to ensure last vote can be cast

if !wasFinalized {
		if k.IsFinalizedInbound(tmpCtx, msg.InboundHash, msg.SenderChainId, msg.EventIndex) {
			return nil, sdkerrors.Wrapf(
				types.ErrObservedTxAlreadyFinalized,
				"%s, InboundHash:%s, SenderChainID:%d, EventIndex:%d",
				voteInboundID,
				msg.InboundHash,
				msg.SenderChainId,
				msg.EventIndex,
			)
		}
	}

Alternative

Since VoteOnInboundBallot start returning more and more value, we could also return a vote state in the returned values instead

VOTE_STATE = NEW | PENDING | FINALIZING | FINALIZED

[kingpinXD]

We could also refactor to check IsFinalizedInbound to be checked when finalizing the cctx
Even if two inbounds are created in the same block they would still be finalized one after the other. We can just remove isNew in that case

	// If the ballot is not finalized return nil here to add vote to commit state
	if !finalized {
		return &types.MsgVoteInboundResponse{}, nil
	}

	if k.IsFinalizedInbound(tmpCtx, msg.InboundHash, msg.SenderChainId, msg.EventIndex) {
		return nil, sdkerrors.Wrapf(
			types.ErrObservedTxAlreadyFinalized,
			"%s, InboundHash:%s, SenderChainID:%d, EventIndex:%d",
			voteInboundID,
			msg.InboundHash,
			msg.SenderChainId,
			msg.EventIndex,
		)
	}

[lumtis]

Wouldn't this prevent observer to give remaining votes after a inbound is finalized?

[kingpinXD]

The vote gets added here

	// If the ballot is not finalized return nil here to add vote to commit state
	if !finalized {
		return &types.MsgVoteInboundResponse{}, nil
	}

isFinalized actually means , isFinalizedBythisVote

[kingpinXD]

Actually, we should do this check twice
Keep the current logic as it is, and add a check again when finalizing

• The current check is needed so that the observer cannot create unnecessary ballots.
• The new check added when finalizing would be the actual replay protection