[bisq-network/bisq] Add irregular txType, add check for total balance (#2587)

[sqrrm]

sqrrm approved this pull request.

utACK

I have some suggested changes that you can use if you feel like.

There is also some questions on how lenient we should be with irregular txs for failed issuance, just added my thoughts.

> @@ -179,8 +179,11 @@ private void maybeRevealVotes(int chainHeight) {
                 .filter(myVote -> myVote.getRevealTxId() == null) // we have not already revealed
                 .forEach(myVote -> {
                     boolean isInVoteRevealPhase = periodService.getPhaseForHeight(chainHeight) == DaoPhase.Phase.VOTE_REVEAL;
+                    // If we would create the tx in the last block it would be confirmed in the best case in th next
+                    // block which would be already the break and would invalidate the vote reveal.
+                    boolean isNotLastBlockInPhase = chainHeight != periodService.getLastBlockOfPhase(chainHeight, DaoPhase.Phase.VOTE_REVEAL);

Would be easier to understand the code if the name was isLastBlockInPhase and the check later was for !isLastBlockInPhase.

> @@ -179,8 +179,11 @@ private void maybeRevealVotes(int chainHeight) {
                 .filter(myVote -> myVote.getRevealTxId() == null) // we have not already revealed
                 .forEach(myVote -> {
                     boolean isInVoteRevealPhase = periodService.getPhaseForHeight(chainHeight) == DaoPhase.Phase.VOTE_REVEAL;
+                    // If we would create the tx in the last block it would be confirmed in the best case in th next
+                    // block which would be already the break and would invalidate the vote reveal.
+                    boolean isNotLastBlockInPhase = chainHeight != periodService.getLastBlockOfPhase(chainHeight, DaoPhase.Phase.VOTE_REVEAL);

```suggestion
                    boolean isLastBlockInPhase = chainHeight == periodService.getLastBlockOfPhase(chainHeight, DaoPhase.Phase.VOTE_REVEAL);
```

>                      boolean isBlindVoteTxInCorrectPhaseAndCycle = periodService.isTxInPhaseAndCycle(myVote.getTxId(), DaoPhase.Phase.BLIND_VOTE, chainHeight);
-                    if (isInVoteRevealPhase && isBlindVoteTxInCorrectPhaseAndCycle) {
+                    if (isInVoteRevealPhase && isNotLastBlockInPhase && isBlindVoteTxInCorrectPhaseAndCycle) {

```suggestion
                    if (isInVoteRevealPhase && !isLastBlockInPhase && isBlindVoteTxInCorrectPhaseAndCycle) {
```

>  
-            // TODO don't burn in such cases
+            // Make sure the optionalIssuanceCandidate is set the BTC

```suggestion
            // Make sure the optionalIssuanceCandidate is set to BTC
```

> @@ -393,19 +399,19 @@ static TxType evaluateTxTypeFromOpReturnType(TempTx tempTx, OpReturnType opRetur
                 boolean hasCorrectNumOutputs = tempTx.getTempTxOutputs().size() >= 3;
                 if (!hasCorrectNumOutputs) {
                     log.warn("Compensation/reimbursement request tx need to have at least 3 outputs");
-                    // This is not an issuance request but it should still not burn the BSQ change
-                    // TODO do we want to burn in such a case?
-                    return TxType.INVALID;
+                    // This is not a valid issuance tx
+                    // We tolerate such an incorrect tx and do not burn the BSQ
+                    return TxType.IRREGULAR;

In this case I would also be ok to burn the BSQ. This is a malformed tx which should not be possible to create with an unmodified client. This is one of the cases where being strict might be worth it as this is about issuance.

>                  }
 
                 TempTxOutput issuanceTxOutput = tempTx.getTempTxOutputs().get(1);
                 boolean hasIssuanceOutput = issuanceTxOutput.getTxOutputType() == TxOutputType.ISSUANCE_CANDIDATE_OUTPUT;
                 if (!hasIssuanceOutput) {
                     log.warn("Compensation/reimbursement request txOutput type of output at index 1 need to be ISSUANCE_CANDIDATE_OUTPUT. " +
                             "TxOutputType={}", issuanceTxOutput.getTxOutputType());
-                    // This is not an issuance request but it should still not burn the BSQ change
-                    // TODO do we want to burn in such a case?
-                    return TxType.INVALID;
+                    // This is not a valid issuance tx
+                    // We tolerate such an incorrect tx and do not burn the BSQ
+                    return TxType.IRREGULAR;

Same here about being strict. Possibly worth being stricter unless you can see a way that normal usage of the client would result in this kind of malformed issuance tx.

> @@ -423,8 +429,8 @@ static TxType evaluateTxTypeFromOpReturnType(TempTx tempTx, OpReturnType opRetur
                 return TxType.PROOF_OF_BURN;
             default:
                 log.warn("We got a BSQ tx with an unknown OP_RETURN. tx={}, opReturnType={}", tempTx, opReturnType);
-                // TODO do we want to burn in such a case?
-                return TxType.INVALID;
+                // We tolerate such an incorrect tx and do not burn the BSQ
+                return TxType.IRREGULAR;

This case I'm not sure about. Perhaps it's good to be this lenient as this is no special kind of tx and would just result in a normal BSQ tx sending BSQ from one output to the next.

> @@ -199,18 +200,25 @@ private void updateWithBsqBlockChainData() {
         Coin totalUnlockingAmount = Coin.valueOf(daoFacade.getTotalAmountOfUnLockingTxOutputs());
         Coin totalUnlockedAmount = Coin.valueOf(daoFacade.getTotalAmountOfUnLockedTxOutputs());
         Coin totalConfiscatedAmount = Coin.valueOf(daoFacade.getTotalAmountOfConfiscatedTxOutputs());
-        availableAmount = issuedAmountFromGenesis
+        Coin totalUtxoAmount = Coin.valueOf(daoFacade.getTotalAmountOfUnspentTxOutputs());
+
+        daoFacade.getTotalAmountOfConfiscatedTxOutputs();

```suggestion
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/2587#pullrequestreview-219174060
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190326/9bed7bf4/attachment-0001.html>