[bisq-network/bisq-core] Use BisqRiskAnalysis (#124)

chirhonul notifications at github.com
Tue Jun 26 10:27:42 UTC 2018


chirhonul approved this pull request.

utACK b1bffdc.

See a few suggestions inline, but nothing I would want to block the review on.

I compared the `BisqRiskaAnalysis` class with the `org.bitcoinj.wallet.DefaultRiskAnalysis` that it was forked from. The only changes, as described in the comments, were the removal of the RBF-becomes-`NON_FINAL` check.

The only other changes in this PR is to set the risk analyzer to the `BisqRiskAnalysis.Analyzer()`, which seems straightforward.

Ideally a change like this would come with tests that would fail before the code change, but I understand if this is difficult given the current stage of the project.

> +import org.bitcoinj.crypto.TransactionSignature;
+import org.bitcoinj.script.ScriptChunk;
+import org.bitcoinj.wallet.DefaultRiskAnalysis;
+import org.bitcoinj.wallet.RiskAnalysis;
+import org.bitcoinj.wallet.Wallet;
+
+import java.util.List;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+
+import static com.google.common.base.Preconditions.checkState;
+
+// Copied from DefaultRiskAnalysis as DefaultRiskAnalysis has mostly private methods and constructor so we cannot

(Optional) I will not hold up the review for this, and the suggestion might have wider consequences than this PR, but another option instead of copying the code would be to ship a patch / set of patches on top of upstream bitcoinJ. That way, we could stay up to date with work going on upstream, and only tweak the specific functionality we need that the library won't let us override.

> +
+// Copied from DefaultRiskAnalysis as DefaultRiskAnalysis has mostly private methods and constructor so we cannot
+// override it.
+// Only change to DefaultRiskAnalysis is removal of the RBF check.
+// For Bisq's use cases RBF is not considered risky. Requiring a confirmation for RBF payments from a users
+// external wallet to Bisq would hurt usability. The trade transaction requires anyway a confirmation and we don't see
+// a use case where a Bisq user accepts unconfirmed payment from untrusted peers and would not wait anyway for at least
+// one confirmation.
+
+/**
+ * <p>The default risk analysis. Currently, it only is concerned with whether a tx/dependency is non-final or not, and
+ * whether a tx/dependency violates the dust rules. Outside of specialised protocols you should not encounter non-final
+ * transactions.</p>
+ */
+public class BisqRiskAnalysis implements RiskAnalysis {
+    private static final Logger log = LoggerFactory.getLogger(DefaultRiskAnalysis.class);

Shouldn't the class passed to `getLogger()` be the current class, i.e. `BisqRiskAnalysis.class`?

> @@ -0,0 +1,273 @@
+/*
+ * Copyright 2013 Google Inc.
+ * Copyright 2014 Andreas Schildbach

Just curious, should we have our own copyright as well? I.e. `Copyright 2018 Bisq developers` or similar?

> +        analyzed = true;
+
+        Result result = analyzeIsFinal();
+        if (result != null && result != Result.OK)
+            return result;
+
+        return analyzeIsStandard();
+    }
+
+    @Nullable
+    private Result analyzeIsFinal() {
+        // Transactions we create ourselves are, by definition, not at risk of double spending against us.
+        if (tx.getConfidence().getSource() == TransactionConfidence.Source.SELF)
+            return Result.OK;
+
+        // For Bisq's use cases RBF is not considered risky

I would suggest just removing the commented-out code, since the class-level comment makes it clear what the changes we made were (and it becomes ambiguous to understand the intent when the comment on L99 clashes with the comment on L101..).

-- 
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-core/pull/124#pullrequestreview-131953947
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20180626/e94c6291/attachment.html>


More information about the bisq-github mailing list