Improved transactions getting logic.

[sergeylappo]

Removed unneeded hashset creation where possible, optimised method logic.
Tests passed as multiple-hd-accounts

[Giszmo]

This is safe because we use the same lock as in getTransactions() and we don't change the transactions.

This is performant, as we don't need the Set property. Only iteration.

[Giszmo]

Please, explain it to me.

if (!isActuallySpent) {
    // return isSpent == isActuallySpent;
    // return isSpent == false;
    return !isSpent;
}

ok, so far so good. I would prefer if (!isActuallySpent) {break;} though, for simplicity. The inconsistency is an exceptional case, so optimizing boolean expressions at the expense of readability is probably not worth it. Also isActuallySpent variable becomes obsolete if you write it like this:

    boolean isTxConsistent(final Transaction tx, final boolean isSpent) {
        for (TransactionOutput o : tx.getOutputs()) {
            if (o.isAvailableForSpending()) {
                if (o.isMineOrWatched(this)) {
                    return !isSpent;
                }
                if (o.getSpentBy() != null) {
                    log.error("isAvailableForSpending != spentBy");
                    return false;
                }
            } else {
                if (o.getSpentBy() == null) {
                    log.error("isAvailableForSpending != spentBy");
                    return false;
                }
            }
        }
        return isSpent;
    }

which makes me even more confused about what is going on here.

Something like

    boolean isTxConsistent(final Transaction tx, final boolean isSpent) {
        Predicate<? super TransactionOutput> isUnspentPredicate = new Predicate<TransactionOutput>() {
            @Override
            public boolean apply(TransactionOutput transactionOutput) {
                return transactionOutput.isAvailableForSpending() && transactionOutput.isMineOrWatched(Wallet.this);
            }
        };
        return isSpent == !Iterators.any(tx.getOutputs().iterator(), isUnspentPredicate);
    }

would be readable but what about the getSpentBy() check? Is it necessary?

[Giszmo]

Good find but unfortunately one call to getTransactions was changed to inlcude and return also the deads.

[Giszmo]

includeDead is not always true.