Fix issue of Trade Step 1 validation done too soon

[ghost]

This PR fixes an issue where some trade data validation was performed too soon, before the transaction has been applied to the trade, resulting in an erroneous popup informing the user "There is an issue with a missing or invalid transaction".

Resolves: #5494.

---

The trade data validation is done at transition to Trade Step 1, it does some basic sanity checking of the deposit transaction: that it is present, that it has 2 inputs, etc. Unfortunately there is a timing issue whereby Trade Step 1 is sometimes reached before the transaction has been applied to the trade.

The trade currently transitions to Trade Step 1 when the deposit transaction is exchanged P2P
in task SellerSendsDepositTxAndDelayedPayoutTxMessage.

After broadcasting, the deposit transaction is stored on the Trade via applyDepositTx(). This can be up to 5 seconds later.

Within that window of time, the trade shows with red "failed" iconography and the validation done at Trade Step 1 fails because of missing information in the Trade.

The goal of this PR is to make Trade Step 1 compatible with the assumption that the trade is initialized enough to check. It makes a simple change moving the Trade Step 1 milestone further along the trade init path, after the broadcast, when the deposit tx has been applied to the trade. This way we can be safe in the assumption that the trade is initialized enough to perform our validation.

---

Other considerations:

There are a number of other ways this could be addressed, such that the deposit tx is applied to the trade early enough in initialization.

1. Task SellerPublishesDepositTx could be invoked before SellerSendsDepositTxAndDelayedPayoutTxMessage.
2. applyDepositTx() to the trade in SellerAsMakerFinalizesDepositTx.

Currently, I have picked the least intrusive change to fix this particular bug, although testing showed that these other two options also would work. Upon reflection. I notice the chosen solution is slightly incongruent with the the ordering of Trade.State and the concept of trade Phase. Perhaps it would be better to consider the options above, of applying deposit Tx to Trade earlier.

An added benefit of applying the deposit transaction earlier would be that the trade would no longer erroneously flash the failed icons prior to broadcast completing (which currently has the effect of causing undue alarm).

So in fact I'd prefer to have solution (2) above; it is cleaner although it might carry more change risk.

[ripcurlx]

I experienced this exact same issue for the first time this weekend (being on the seller side) as well. As I'm not so fluent with the trade protocol I'll wait for ACKs by @sqrrm or @chimp1984.

[chimp1984]

Thanks @jmacxx for looking into that.
I think the proposed fix is a bit dangerous as we use the ordinal of the trade states in various areas, so changing the order of states can has unintended side effects.

I think better is to change the handling in PendingTradesViewModel.onTradeStateChanged to something like that:

   // #################### Phase DEPOSIT_PAID
            case SELLER_PUBLISHED_DEPOSIT_TX:
                buyerState.set(BuyerState.STEP1);
                sellerState.set(SellerState.STEP1);
                break;

                // DEPOSIT_TX_PUBLISHED_MSG
                // seller perspective
            case SELLER_SENT_DEPOSIT_TX_PUBLISHED_MSG:
            case SELLER_SAW_ARRIVED_DEPOSIT_TX_PUBLISHED_MSG:
            case SELLER_STORED_IN_MAILBOX_DEPOSIT_TX_PUBLISHED_MSG:
            case SELLER_SEND_FAILED_DEPOSIT_TX_PUBLISHED_MSG:
                break;

                // buyer perspective
            case BUYER_RECEIVED_DEPOSIT_TX_PUBLISHED_MSG:

                // Alternatively the maker could have seen the deposit tx earlier before he received the DEPOSIT_TX_PUBLISHED_MSG
            case BUYER_SAW_DEPOSIT_TX_IN_NETWORK:
                buyerState.set(BuyerState.STEP1);
                sellerState.set(SellerState.STEP1);
                break;

I have not tested it, but I think it should lead to the expected behaviour to move to step 1 only after the deposit tx is published.

I guess there might be an issue though with false positive - failed broadcastTx attempts. Sometimes the broadcast gets a timeout in 5 sec. even the broadcast works but due some bitcoinj issue it returns a failure. In such cases the user would never move to step 1. I think the SellerPublishesDepositTx need to handle that to in the onFailure case (apply same code as in onSuccess). That might be a bit risky as if it really fails we still would apply it. Maybe the tx from the wallet gives enough information to decide if the broadcast worked or not. I guess the tx gets committed to the wallet also in case of a failure, so to just look up the tx in the wallet will likely not work. But maybe the confidence or some other tx details reveal more....

[chimp1984]

Ah the above would not work as the ordinal of SELLER_PUBLISHED_DEPOSIT_TX is < SELLER_SAW_ARRIVED_DEPOSIT_TX_PUBLISHED_MSG and we do not allow setting states to lower states.

All those states:

SELLER_SENT_DEPOSIT_TX_PUBLISHED_MSG(Phase.DEPOSIT_PUBLISHED),
SELLER_SAW_ARRIVED_DEPOSIT_TX_PUBLISHED_MSG(Phase.DEPOSIT_PUBLISHED),
SELLER_STORED_IN_MAILBOX_DEPOSIT_TX_PUBLISHED_MSG(Phase.DEPOSIT_PUBLISHED),
SELLER_SEND_FAILED_DEPOSIT_TX_PUBLISHED_MSG(Phase.DEPOSIT_PUBLISHED),

are only used in the PendingTradesViewModel and as we should ignore those anyway we could decide to NOT set them, so the setting of the SELLER_PUBLISHED_DEPOSIT_TX would not get blocked (a comment why we dont set them and marking those states deprecated would be good as well).

The order of the states is a relict from the old trade protocol and also sometimes there is no deterministic order of events.
The state handling got too fine grained for the level where we use it.

Regarding the failed broadcast:
We use the default TxBroadcaster.Callback.onTimeout method which calls onSuccess at those expected false positive failures (see comment there). So the code in SellerPublishesDepositTx should work for those cases as it is. Would have been an issue anyway otherwise as the code at onSuccess is important to be executed and it would have led to bugs if that would not get called.

[chimp1984]

utACK

[ghost]

To also prevent the initial red failed icons in Open Trades list, something like this would do it, what do you think?

PendingTradesView.java L370:

private boolean isMaybeInvalidTrade(Trade trade) {
    return trade.hasErrorMessage() ||
            (Trade.Phase.DEPOSIT_PUBLISHED.ordinal() <= trade.getPhase().ordinal() && trade.isTxChainInvalid());
}

[chimp1984]

To also prevent the initial red failed icons in Open Trades list, something like this would do it, what do you think?

PendingTradesView.java L370:

private boolean isMaybeInvalidTrade(Trade trade) {
    return trade.hasErrorMessage() ||
            (Trade.Phase.DEPOSIT_PUBLISHED.ordinal() <= trade.getPhase().ordinal() && trade.isTxChainInvalid());
}

Looks ok to me.

[chimp1984]

utACK

[ripcurlx]

utACK based on #5746 (review)