Don't treat RBF transactions as risky #124
Conversation
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.
There was a problem hiding this comment.
utACK b1bffdc.
See a few suggestions inline, but nothing I would want to block the review on.
Edit: oh and I should call out that I am not very familiar with the code base at all yet. It might be good to wait for more reviews from more experienced reviewers, but it's up to you.
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 static com.google.common.base.Preconditions.checkState; | ||
|
|
||
| // Copied from DefaultRiskAnalysis as DefaultRiskAnalysis has mostly private methods and constructor so we cannot |
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
Yes agree. Though our branch is far out of sync with master as well their release branch from where we forked. So it will take considerable effort to sync up there. It is also unclear when they will have a new release which carries the lots of changes in master. Forking from master might be risky as not deep tested I assume...
There was a problem hiding this comment.
Right, it would be somewhat sensitive regardless.. if it was desireable to pursue this, I would start by trying to switch to latest release of BitcoinJ (looks like v0.14.7 at time of writing, https://github.com/bitcoinj/bitcoinj/tree/v0.14.7), then apply the commits to do any necessary changes for Bisq, then create patches from those commits so only a clean checkout would be necessary, and apply the .patch files during some build step.
| * transactions.</p> | ||
| */ | ||
| public class BisqRiskAnalysis implements RiskAnalysis { | ||
| private static final Logger log = LoggerFactory.getLogger(DefaultRiskAnalysis.class); |
There was a problem hiding this comment.
Shouldn't the class passed to getLogger() be the current class, i.e. BisqRiskAnalysis.class?
There was a problem hiding this comment.
Ah yes, thanks! Will fix that
| @@ -0,0 +1,273 @@ | |||
| /* | |||
| * Copyright 2013 Google Inc. | |||
| * Copyright 2014 Andreas Schildbach | |||
There was a problem hiding this comment.
Just curious, should we have our own copyright as well? I.e. Copyright 2018 Bisq developers or similar?
There was a problem hiding this comment.
I am not sure how to deal with such. Probably both headers should work?
There was a problem hiding this comment.
If you mean to add our headers on top of the old ones, yes, that's how it's commonly done and what I was suggesting. E.g for Bitcoin Core:
Edit: Ah, I saw now how you did it in the commit. Sure, using the same header as for other Bisq files makes more sense than doing anything else for this particular file.
| if (tx.getConfidence().getSource() == TransactionConfidence.Source.SELF) | ||
| return Result.OK; | ||
|
|
||
| // For Bisq's use cases RBF is not considered risky |
There was a problem hiding this comment.
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..).
There was a problem hiding this comment.
Agree. Will remove it.
- Add Bisq license header - Remove commented out code - Fix Logger class reference
|
Post-merge utACK 5f46f28. The only new changes are the ones I requested. For future reference, maybe it would have been better to rebase the two commits into one before merging to |
ManfredKarrer
changed the title
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.