<p><b>@sqrrm</b> commented on this pull request.</p>
<p>I think this is a better way to handle it than through the parser. I tried using the parser and that's just too risky and hard to control in the end, this way is much more controlled and only very specifically designated change outputs can be spent without confirmation and parsing which is safer.</p>
<p>The problem would be that this is more fragile if we start changing anything in the BSQ parsing as this is now a separate kind of parsing that needs to be handled with great care.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2482#discussion_r261544268">core/src/main/java/bisq/core/btc/wallet/BsqCoinSelector.java</a>:</p>
<pre style='color:#555'>> + Transaction parentTransaction = output.getParentTransaction();
+ if (parentTransaction == null)
+ return false;
+
+ // If it is a normal confirmed BSQ output we use the default lookup at the daoState
+ if (daoStateService.isTxOutputSpendable(new TxOutputKey(parentTransaction.getHashAsString(), output.getIndex())))
+ return true;
+
+ // It might be that it is an unconfirmed change output which we allow to be used for spending without requiring a confirmation.
+ // We check if we have the output in the dao state, if so we have a confirmed but unspendable output (e.g. confiscated).
+ if (daoStateService.getTxOutput(new TxOutputKey(parentTransaction.getHashAsString(), output.getIndex())).isPresent())
+ return false;
+
+ // Only if its not existing yet in the dao state (unconfirmed) we use our unconfirmedBsqChangeOutputList to
+ // check if it is an own change output.
+ return unconfirmedBsqChangeOutputListService.hasTransactionOutput(output);
</pre>
<p>Don't we need to check that it's also unspent or is the unconfirmed list only for unspent txs?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2482#discussion_r261547206">core/src/main/java/bisq/core/dao/state/unconfirmed/UnconfirmedBsqChangeOutputListService.java</a>:</p>
<pre style='color:#555'>> + }
+ }
+ }
+
+
+ ///////////////////////////////////////////////////////////////////////////////////////////
+ // API
+ ///////////////////////////////////////////////////////////////////////////////////////////
+
+ /**
+ * Once a tx gets committed to out BSQ wallet we store the change output for allowing it to be spent in follow-up
+ * transactions.
+ */
+ public void onCommitTx(Transaction tx, TxType txType, Wallet wallet) {
+ // We remove all potential connected outputs from our inputs as they would have been spent.
+ removeConnectedOutputsOfInputsOfTx(tx);
</pre>
<p>Ok, I see the outputs are removed when spent, previous comment is then void since all transactions in the unconfirmed list are unspent.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2482#discussion_r261548631">core/src/main/java/bisq/core/dao/state/unconfirmed/UnconfirmedBsqChangeOutputListService.java</a>:</p>
<pre style='color:#555'>> + // If no BTC output (unlikely but
+ // possible) the index points to the BTC output and then we detect that it is not part of our wallet.
+ //
+ List<TransactionOutput> outputs = tx.getOutputs();
+ if (changeOutputIndex > outputs.size() - 1)
+ return;
+
+ TransactionOutput change = outputs.get(changeOutputIndex);
+ if (!change.isMine(wallet))
+ return;
+
+ UnconfirmedTxOutput txOutput = UnconfirmedTxOutput.fromTransactionOutput(change);
+ if (unconfirmedBsqChangeOutputList.containsTxOutput(txOutput))
+ return;
+
+ log.error("We add txOutput {}", txOutput);
</pre>
<p>Why log an error?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2482#discussion_r261548821">core/src/main/java/bisq/core/dao/state/unconfirmed/UnconfirmedBsqChangeOutputListService.java</a>:</p>
<pre style='color:#555'>> + return;
+
+ TransactionOutput change = outputs.get(changeOutputIndex);
+ if (!change.isMine(wallet))
+ return;
+
+ UnconfirmedTxOutput txOutput = UnconfirmedTxOutput.fromTransactionOutput(change);
+ if (unconfirmedBsqChangeOutputList.containsTxOutput(txOutput))
+ return;
+
+ log.error("We add txOutput {}", txOutput);
+ unconfirmedBsqChangeOutputList.add(txOutput);
+ persist();
+ }
+
+ //TODO not sure how we should handle it but seems safest to just clear all
</pre>
<p>Agree, rather be safe and delay trading for one new confirmed block.</p>
<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/2482#pullrequestreview-209507460">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZtmD1FJ_GnTRRxMTGQhxRwjXCPmRiks5vSQGdgaJpZM4bYQEC">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtgVMkQ2ibEwDpEwRE5U5usPiY3kGks5vSQGdgaJpZM4bYQEC.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 commented on #2482"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq/pull/2482#pullrequestreview-209507460"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/2482#pullrequestreview-209507460",
"url": "https://github.com/bisq-network/bisq/pull/2482#pullrequestreview-209507460",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>