<p><b>@sqrrm</b> approved this pull request.</p>

<p>utACK</p>
<p>I have some suggested changes that you can use if you feel like.</p>
<p>There is also some questions on how lenient we should be with irregular txs for failed issuance, just added my thoughts.</p><hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2587#discussion_r269326337">core/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java</a>:</p>
<pre style='color:#555'>> @@ -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);
</pre>
<p>Would be easier to understand the code if the name was isLastBlockInPhase and the check later was for !isLastBlockInPhase.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2587#discussion_r269326483">core/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java</a>:</p>
<pre style='color:#555'>> @@ -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);
</pre>
⬇️ Suggested change
<pre style="color: #555">-                    boolean isNotLastBlockInPhase = chainHeight != periodService.getLastBlockOfPhase(chainHeight, DaoPhase.Phase.VOTE_REVEAL);
+                    boolean isLastBlockInPhase = chainHeight == periodService.getLastBlockOfPhase(chainHeight, DaoPhase.Phase.VOTE_REVEAL);
</pre>


<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2587#discussion_r269326594">core/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java</a>:</p>
<pre style='color:#555'>>                      boolean isBlindVoteTxInCorrectPhaseAndCycle = periodService.isTxInPhaseAndCycle(myVote.getTxId(), DaoPhase.Phase.BLIND_VOTE, chainHeight);
-                    if (isInVoteRevealPhase && isBlindVoteTxInCorrectPhaseAndCycle) {
+                    if (isInVoteRevealPhase && isNotLastBlockInPhase && isBlindVoteTxInCorrectPhaseAndCycle) {
</pre>
⬇️ Suggested change
<pre style="color: #555">-                    if (isInVoteRevealPhase && isNotLastBlockInPhase && isBlindVoteTxInCorrectPhaseAndCycle) {
+                    if (isInVoteRevealPhase && !isLastBlockInPhase && isBlindVoteTxInCorrectPhaseAndCycle) {
</pre>


<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2587#discussion_r269331989">core/src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>>  
-            // TODO don't burn in such cases
+            // Make sure the optionalIssuanceCandidate is set the BTC
</pre>
⬇️ Suggested change
<pre style="color: #555">-            // Make sure the optionalIssuanceCandidate is set the BTC
+            // Make sure the optionalIssuanceCandidate is set to BTC
</pre>


<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2587#discussion_r269333609">core/src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>> @@ -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;
</pre>
<p>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.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2587#discussion_r269334058">core/src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>>                  }
 
                 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;
</pre>
<p>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.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2587#discussion_r269334750">core/src/main/java/bisq/core/dao/node/parser/TxParser.java</a>:</p>
<pre style='color:#555'>> @@ -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;
</pre>
<p>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.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/2587#discussion_r269336809">desktop/src/main/java/bisq/desktop/main/dao/wallet/dashboard/BsqDashboardView.java</a>:</p>
<pre style='color:#555'>> @@ -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();
</pre>
⬇️ Suggested change
<pre style="color: #555">-        daoFacade.getTotalAmountOfConfiscatedTxOutputs();
</pre>


<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/2587#pullrequestreview-219174060">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZtsOvAu49ZVPlEL1wm1dzGpRgfxzAks5vapvYgaJpZM4cKo_c">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtpkuFm_A07KyXzVUIjYY1cOTt7Azks5vapvYgaJpZM4cKo_c.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/bisq-network/bisq","title":"bisq-network/bisq","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/bisq-network/bisq"}},"updates":{"snippets":[{"icon":"PERSON","message":"@sqrrm approved #2587"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq/pull/2587#pullrequestreview-219174060"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/2587#pullrequestreview-219174060",
"url": "https://github.com/bisq-network/bisq/pull/2587#pullrequestreview-219174060",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>