search

bisq-network / projects

@bisq-network project management
https://bisq.wiki/Project_management
9 stars 2 forks source link

Introduce interface between Bisq and BitcoinJ #30

Closed dmos62 closed 4 years ago

dmos62 commented 4 years ago

Description

This is a proposal for introducing an explicit Java interface between Bisq and BitcoinJ, with the purpose of improving maintainability of our Bitcoin network code.

Rationale

In summary

Why is it important?
Why now? What happens if we wait?

In depth

Maintenance of Bitcoin network code

Bisq is currently tightly coupled with BitcoinJ, and our usage of it, being scattered throughout the rest of the code, is difficult to audit and to maintain. Abstracting our usage of BitcoinJ into an explicit interface would facilitate audits, maintenance of Bisq, maintenance of our BitcoinJ fork, and general comprehension of Bisq's internals.

Restoring maintainability of our BitcoinJ fork

BitcoinJ is a Bitcoin network client that is used by and tightly coupled with Bisq. Maintaining the BitcoinJ part of our codebase has proven challenging. Notable illustrations being that we're using a two year-old release of BitcoinJ (0.14.7, Mar 30, 2018) and that we have a dedicated role for its maintenance (which has not been filled since May, 2019).

Among other things, returning to beIng able to maintain our BitcoinJ fork (and thus being able to upgrade it) will allow us to support SegWit/Bech32 addresses, which is an often asked for feature.

Tight coupling and maintainability

Our BitcoinJ fork's maintenance involves understanding how we use BitcoinJ, which is made difficult by tight coupling. To illustrate what is meant by tight coupling here is the long list of unique BitcoinJ imports we use in Bisq (excluding tests, minor formatting added):

grep -r --include=\*\.java -E "import( static)* org.bitcoinj" ./*/src/main -h | sort -u

Output moved to pastebin.com for the sake of readability:
https://pastebin.com/raw/Jd0nA6Nk

And, below is the even longer list of (non-test) source files that import BitcoinJ classes (with minor formatting added):

grep -r --include=\*\.java -E "import( static)* org.bitcoinj" ./*/src/main -l | sort -u

Output moved to pastebin.com for the sake of readability:
https://pastebin.com/raw/Ap0wf3Pd

To maintain our BitcoinJ fork you'd have to make at least some kind of sense of the above usages of it, which is (I think evidently) intimidating and difficult. The primary way that the proposed interface would help is by structuring our Bisq usage.

Make sense of BitcoinJ usage by introducing an intermediary interface

The proposed interface would aim to hide the bulk of BitcoinJ specific logic and to centralize essential Bisq Bitcoin network logic.

BitcoinJ imports can be put into two categories: essential and auxiliary. I define auxiliary imports as those that serve to handle output from and pass input to the essential imports. The essential imports (such as Wallet, PeerGroup, BlockStore, net.discovery) are the exclusive focus of this refactor effort, in part due to the sheer number of aux imports, and, at the same time, because hiding essential imports behind an interface should be enough to see an appreciable improvement in maintainability.

In other words, the proposed interface would not hide all of the above listed imports, but only the most important ones. This also means that this wouldn't be a complete decoupling of BitcoinJ, though it would pave the way for one, if it were to ever happen.

Criteria for delivery

Measures of success

Risks

Our Bitcoin network code is one of the most sensitive areas of Bisq; however, the risk of introducing bad changes should be minimal since this is a refactoring effort (meaning rearranging code without changing algorithms).

Tasks

This refactor effort is exploratory by nature, so making a precise plan is not possible ahead of time. Currently the plan is to analyse BitcoinJ usage and abstract it away behind the new interface, in order of descending importance, until only the least critical BitcoinJ code is left.

Estimates

Budget not yet discussed.

dmos62 commented 4 years ago

Concrete plan

Note: below text is written in a tree-like fashion, which helps me structure my writing and navigate the text. Hopefully it doesn't look too outlandish.

Refactoring steps

For readability, I've bolded what you'd call the main variable names if this were code:

Illustrative code sketch

Below sketch illustrates what btc.core.btc.low.PeerGroup could look like.

package bisq.core.btc.low;

import org.bitcoinj.core.BlockChain;
import org.bitcoinj.core.NetworkParameters;
import org.bitcoinj.net.BlockingClientManager;

import bisq.core.btc.nodes.ProxySocketFactory;
import bisq.core.btc.nodes.LocalBitcoinNode;

import java.net.Proxy;
import java.net.InetSocketAddress;

import com.runjva.sourceforge.jsocks.protocol.Socks5Proxy;

/* Below code is a sketch of what btc.low can look like.
 * To make it short and simple, it presumes that our BitcoinJ usage is only
 * what is in the bisq.core.btc.WalletConfig::createPeerGroup method.
 * This sketch tries to demonstrate how logic is moved into btc.low: you start
 * out with a lot of BitcoinJ-specific logic, and end up with a
 * BitcoinJ-agnostic endpoint and a clear view of what BitcoinJ endpoints are
 * used under the sheets.
 * The code would definitely be simplified more after moving, but that's
 * outside the scope of this sketch.
 */

public class PeerGroup {

    private final org.bitcoinj.core.PeerGroup bitcoinjPeerGroup;

    private PeerGroup(org.bitcoinj.core.PeerGroup bitcoinjPeerGroup) {
        this.bitcoinjPeerGroup = bitcoinjPeerGroup;
    }

    /* This method is a copy-paste of
     * bisq.core.btc.WalletConfig::createPeerGroup (except for the signature
     * and using locally defined constructors). It serves only demo purposes
     * and code quality inside it not representative of standards set for this
     * project.
     */
    private static PeerGroup createPeerGroup(
            Socks5Proxy socks5Proxy,
            NetworkParameters params,
            BlockChain vChain,
            LocalBitcoinNode localBitcoinNode
    ) {
        PeerGroup peerGroup;
        // no proxy case.
        if (socks5Proxy == null) {
            peerGroup = this(params, vChain);
        } else {
            // proxy case (tor).
            Proxy proxy = new Proxy(Proxy.Type.SOCKS,
                    new InetSocketAddress(
                        socks5Proxy.getInetAddress().getHostName(),
                        socks5Proxy.getPort()));

            ProxySocketFactory proxySocketFactory =
                new ProxySocketFactory(proxy);
            // We don't use tor mode if we have a local node running
            BlockingClientManager blockingClientManager =
                config.ignoreLocalBtcNode ?
                new BlockingClientManager() :
                new BlockingClientManager(proxySocketFactory);

            peerGroup = this(params, vChain, blockingClientManager);

            blockingClientManager.setConnectTimeoutMillis(TOR_SOCKET_TIMEOUT);
            peerGroup.setConnectTimeoutMillis(TOR_VERSION_EXCHANGE_TIMEOUT);
        }

        if (!localBitcoinNode.shouldBeUsed())
            peerGroup.setUseLocalhostPeerWhenPossible(false);

        return peerGroup;
    }

    /* Proxy all PeerGroup methods that Bisq uses.
     * These would probably be "protected" and be in a separate class, which
     * would be subclassed by this one, for the sake of separating the used
     * BitcoinJ endpoints and their usage.
     */

    private PeerGroup(
            NetworkParameters params,
            BlockChain vChain
    ) {
        org.bitcoinj.core.PeerGroup bitcoinjPeerGroup =
            new org.bitcoinj.core.PeerGroup(params, vChain);
        this(bitcoinjPeerGroup);
    }

    private PeerGroup(
            NetworkParameters params,
            BlockChain vChain,
            BlockingClientManager
            blockingClientManager
    ) {
        org.bitcoinj.core.PeerGroup bitcoinjPeerGroup =
            new org.bitcoinj.core.PeerGroup(
                    params, vChain, blockingClientManager);
        this(bitcoinjPeerGroup);
    }

    private void setUseLocalhostPeerWhenPossible(boolean bool) {
        this.bitcoinjPeerGroup.setUseLocalhostPeerWhenPossible(bool);
    }

    private void setConnectTimeoutMillis(int timeout) {
        this.bitcoinjPeerGroup.setConnectTimeoutMillis(timeout);
    }

}
ghubstan commented 4 years ago

@dmos62 , Just want to express some enthusiasm for this project... Really, really big thumbs up from me.

The newly forming gRPC/Core API is going to help in terms of quick dev/test iterations -- faster than re-starting the UI after small changes. It's a little too early today, and the priority for the API is to ship the simplest reference impl asap, but even that will help speed up functional testing.

cbeams commented 4 years ago

@dmos62, what do you think about putting together an initial PR that demonstrates a single such refactoring that you think is demonstrative of the value that can be provided? I'm generally for this effort, but I'd like to have a look at a working example before signing off on a larger scale effort. My main concern here is that I'm not sure the "fully transparent wrap" approach you detail above will actually add much value. If we do nothing more than a 1:1 wrap of a subset of the BitcoinJ API, what do we really gain? I'd rather see this play out in code than discuss it further here, so if you'd like to take a stab at it, please go ahead.

Also, what is the significance of "low" in your proposed bisq.core.btc.low package? Do you mean "low-level bitcoin operations", perhaps? In any case, I don't think this is an intention-revealing name. Perhaps better naming will avail itself as we see and review an actual working example.

dmos62 commented 4 years ago

@cbeams I agree a PR example is a good idea.

The 1:1 wrap part is just to get the foundation for where to move BitcoinJ-specific logic. I.e. the refactoring happens on top of that. You'd move the logic in one set of commits (like in the sketch) and then you'd most likely simplify it in another set of commits (not in the sketch).

At first I was thinking of calling it btc.client, since it would handle all Bitcoin networking, but then realised that actually the bisq.core.btc package is our Bitcoin client, and the proposed package would be the lowest-level of that client, hence the bisq.core.btc.low. It's low-level in that it hides BitcoinJ logic that the rest of the btc package shouldn't care about and exposes the basic interface that the rest of the btc package will use. It makes sense to me at this point, but that might change and I don't have strong feelings either way.

sqrrm commented 4 years ago

I like the idea of starting by just doing a transparent wrap and continue incrementally from there. One of the biggest issues will likely be keeping refactoring PRs small enough that they are easy enough to review and merge. I think the suggested route sounds good, but a sample PR will make it even clearer.

oscarguindzberg commented 4 years ago
dmos62 commented 4 years ago

@oscarguindzberg I think there's potential for collaboration and see these projects working in complement to each other. More specifically, I can see this project facilitating the more interesting Bisq-side changes required for adopting SegWit and BitcoinJ 0.15.x in general.

Paving the way for an alternative library is more of a nice to have at this point. My priority with this project at this time is making our Bitcoin net code easier to maintain and audit. It just so happens that this is what's needed for switching out BitcoinJ as well.

dmos62 commented 4 years ago

Status update:

While I believe that this project is very valuable in the medium to long term, and I very much enjoy working on it, I'm putting it on hold, because I feel there are short-term Bisq responsibilities that I should move my attention to. Follow a few notes I'd like to share.

This project is relatively slow to implement. To illustrate, I've spent 2.5 weeks refactoring logic surrounding PeerGroup, which is the simplest and least critical part of our Bitcoin code (that's also why I chose to start with it), and that's about 50% finished (intuition).

Whereas I started out thinking that this project would be low-hanging-fruit-only type of task, I soon realised that the really tangible increase in maintainability is from diving deep into Bisq code. Parts other than PeerGroup-related logic might be different, but probably not.

I've discovered a pretty complicated algorithm (which decides how we source peers) and I've not yet decided how to approach its audit and eventual refactor. I've created a TLA+ state machine of it (took about a week), but this algo has so many initial states that it's hard to define the properties that the algorithm should satisfy. For example, if a local bitcoin node is provided and custom nodes are provided too and we're in regtest, what sourcing method should be used? What about DAO regtest? I'm not really sure. And those are not all the input variables. Defining what to do for Mainnet is not that hard, but the rest is complicated, because it requires knowing a lot about Bisq. So that's the last thing I was working on.

Project's commit structure is such that it can be merged in installations and read one by one. I took care to have most if not all commits do only one thing, because for a reviewer 2 commits doing 2 simple things can be a lot simpler to review than those 2 commits squashed into 1.

I plan to now move to doing some PR reviewing and urgent bugfixing. I've considered continuing work on this refactor part-time, but I don't see myself being able to multitask this.

I'll PR the refactor to make my progress available.

cbeams commented 4 years ago

Thanks for the update, @dmos62. I'm closing this as was:deferred as that seems most fitting (definition). Let me know if that doesn't make sense to you.