Replace tail recursion of the play() method with an ordinary loop, to
prevent a new open JAR resource InputStream + sound file OutputStream
(which were created every 4 minute playback) from accumulating on the
stack, closing them inside the loop instead. (This also prevents
eventual stack overflow.)
Also tidy up FileUtil.resourceToFile and put the JAR URL InputStream in
a try-with-resources block, to ensure that it doesn't leak either.
Previously, Travis CI was failing non-deterministically due to a race
condition in which a thread was started in order to call the blocking
ServerSocket.accept() method, and sometimes the subsequent attempt by
LocalBitcoinNode.detectAndRun() to connect to that socket's port would
occur before the thread had actually called the accept() method.
This commit simplifies the approach by removing the thread entirely. As
it turns out, calling accept() is not necessary; simply constructing a
new ServerSocket() binds to and listens on the given port, such that a
subsequent attempt to connect() will succeed.
Previously ConfigTests constructed Config instances with string-based
options, e.g.:
Config config = new Config("--appName=My-Bisq");
The advantage here is clarity, but the downside is repetition of the
option names without any reference to their corresponding Config.*
constants.
One solution to the problem would be to format the option strings using
constants declared in the Config class, e.g.:
Config config = new Config(format("--%s=My-Bisq", APP_NAME));
but this is verbose, cumbersome to read and write and requires repeating
he '--' and '=' option syntax.
This commit introduces the Opt class and the opt() and configWithOpts()
methods to ConfigTests to make testing easier while using constant
references and preserving readability. e.g.:
Config config = configWithOpts(opt(APP_NAME, "My-Bisq"));
In the process of making these changes a bug was discovered in the
monitor submodule's P2PNetworkLoad class and that has been fixed here as
well.
This change also required introducing several option name constants that
had not previously been extracted in order to be referenced within
ConfigTests. For consistency and completeness, all additional option
names that did not previously have a contstant now have one.
This new class encapsulates all functionality related to detecting a
local Bitcoin node and reporting whether or not it was detected.
Previously this functionality was spread across the Config class
(formerly BisqEnvironment) with its mutable static
isLocalBitcoinNodeRunning property and the BisqSetup class with its
checkIfLocalHostNodeIsRunning method. All of this functionality now
lives within the LocalBitcoinNode class, an instance of which is wired
up via Guice and injected wherever necessary.
Note that the code for detecting whether the node is running has been
simplified, in that it is no longer wrapped in its own dedicated Thread.
There appears to be no performance benefit from doing so, and leaving it
in place would have made testing more difficult than necessary.
Several methods in BisqSetup have also been refactored to accept
callbacks indicating which step should be run next. This has the effect
of clarifying when the step2()-step5() methods will be called.
Previously this static property had been managed within
BaseCurrencyNetwork itself and was accessed directly by callers. Now it
is managed within Config, made private and accessed only via the
new and well-documented baseCurrencyNetwork() method. The same goes for
baseCurrencyNetworkParameters().
It is unfortunate that we must retain these mutable static fields and
accessors, but after trying to eliminate them entirely, this approach is
the lesser of two evils; attempting to use a Config instance and
instance methods only ends up being quite cumbersome to implement,
requiring Config to be injected into many more classes than it currently
is. Getting access to the BaseCurrencyNetwork is basically a special
case, and treating it specially as a static field is in the end the most
pragmatic approach.
Prior to this commit, the way that the appDataDir and its subdirectories
were created was a haphazard process that worked but in a fragile and
non-obvious way. When Config was instantiated, an attempt to call
btcNetworkDir.mkdir() was made, but if appDataDir did not already exist,
this call would always fail because mkdir() does not create parent
directories. This problem was never detected, though, because the
KeyStorage class happened to call mkdirs() on its 'keys' subdirectory,
which, because of the plural mkdirs() call ended up creating the whole
${appDataDir}/${btcNetworkDir}/keys hierarchy. Other btcNetworkDir
subdirectories such as tor/ and db/ then benefited from the hierarchy
already existing when they attempted to call mkdir() for their own dirs.
So the whole arrangement worked only because KeyStorage happened to make
a mkdirs() call and because that code in KeyStorage happened to get
invoked before the code that managed the other subdirectories.
This change ensures that appDataDir and all its subdirectories are
created up front, such that they are guaranteed to exist by the time
they are injected into Storage, KeyStorage, WalletsSetup and TorSetup.
The hierarchy is unchanged, structured as it always has been:
${appDataDir}
└── btc_mainnet
├── db
├── keys
├── wallet
└── tor
Note that the tor/ subdirectory actually gets deleted and re-created
within the TorSetup infrastructure regardless of whether the directory
exists beforehand.
Option handling is now the responsibility of the Config class. JOpt's
OptionParser is no longer passed down to BisqExecutable subclasses'
doExecute method, as they can now rely on the Config abstraction.
Previously, certain exceptions e.g. IllegalArgumentException would
result in the Bisq process exiting with an stack trace. This change
broadens exception handling during argument parsing such that all
Throwable subclasses are caught and the Bisq process exits gracefully
with a simple error message.
In previous commits, BisqEnvironment functionality has been fully ported
to the new, simpler and more type-safe Config class. This change removes
BisqEnvironment and all dependencies on the Spring Framework Environment
interface that it implements.
The one exception is the pricenode module, which is separate and apart
from the rest of the codebase in that it is a standalone, Spring-based
HTTP service.
Note that this change makes the user-facing change of renaming
the 'numConnectionForBtc' (singular 'Connection') to
'numConnectionsForBtc' (plural 'Connections'). It is presumed that not
many users are relying on this option for day-to-day operations, and the
singular version was pretty clearly a typo / oversight.
Note that the default value of 600 advertised in BisqExecutable's option
handling was incorrect. The actual value had since become 1200 MB. This
correct default is now reflected in Config's option handling.
This was added way back in 8f8866da and has since fallen out of use
entirely.
Removing it now because it depends on BisqEnvironment.appName, which
will be moved to Config in the next commit.
NOTE: This removes entirely the old BisqExecutable.appDataDir method
implemented for the v0.5.3 hotfix that renames the data dir from 'bisq'
to 'Bisq'. See a7f3d68cb for details.
Set OptionParser.allowsUnrecognizedOptions(true) to make sure
BisqEnvironment doesn't fail while options are still being transferred
one-by-one to Config.
Prior to this commit, BisqExecutable has been responsible for parsing
command line and config file options and BisqEnvironment has been
responsible for assigning default values to those options and providing
access to option values to callers throughout the codebase.
This approach has worked, but at considerable costs in complexity,
verbosity, and lack of any type-safety in option values. BisqEnvironment
is based on the Spring Framework's Environment abstraction, which
provides a great deal of flexibility in handling command line options,
environment variables, and more, but also operates on the assumption
that such inputs have String-based values.
After having this infrastructure in place for years now, it has become
evident that using Spring's Environment abstraction was both overkill
for what we needed and limited us from getting the kind of concision and
type saftey that we want. The Environment abstraction is by default
actually too flexible. For example, Bisq does not want or need to have
environment variables potentially overriding configuration file values,
as this increases our attack surface and makes our threat model more
complex. This is why we explicitly removed support for handling
environment variables quite some time ago.
The BisqEnvironment class has also organically evolved toward becoming a
kind of "God object", responsible for more than just option handling. It
is also, for example, responsible for tracking the status of the user's
local Bitcoin node, if any. It is also responsible for writing values to
the bisq.properties config file when certain ban filters arrive via the
p2p network. In the commits that follow, these unrelated functions will
be factored out appropriately in order to separate concerns.
As a solution to these problems, this commit begins the process of
eliminating BisqEnvironment in favor of a new, bespoke Config class
custom-tailored to Bisq's needs. Config removes the responsibility for
option parsing from BisqExecutable, and in the end provides "one-stop
shopping" for all option parsing and access needs.
The changes included in this commit represent a proof of concept for the
Config class, where handling of a number of options has been moved from
BisqEnvironment and BisqExecutable over to Config. Because the migration
is only partial, both Config and BisqEnvironment are injected
side-by-side into calling code that needs access to options. As the
migration is completed, BisqEnvironment will be removed entirely, and
only the Config object will remain.
An additional benefit of the elimination of BisqEnvironment is that it
will allow us to remove our dependency on the Spring Framework (with the
exception of the standalone pricenode application, which is Spring-based
by design).
Note that while this change and those that follow it are principally a
refactoring effort, certain functional changes have been introduced. For
example, Bisq now supports a `--configFile` argument at the command line
that functions very similarly to Bitcoin Core's `-conf` option.
Problem: a stack trace was being thrown during daemon startup from
BisqDaemonMain.onSetupComplete when it attempted to start a
second BisqGrpcServer and failed to bind to the already-bound port.
The first BisqGrpcServer is started in
BisqDaemonMain.onApplicationStarted much earlier in the startup process.
Solution: remove the second attempt to start the server by removing
BisqDaemonMain's implementation of onSetupComplete, and in turn remove
the now-obsolete bisqGrpcServer field as well.
This change also eliminates the BisqGrpcServer.blockUntilShutdown
method, which in turn called the underlying grpc server's
awaitTermination method. As the comment there explained, this was
thought to be necessary because grpc "does not use daemon threads by
default", but this is actually incorrect. According to the grpc Javadoc
at [1], "Grpc uses non-daemon Threads by default and thus a Server will
continue to run even after the main thread has terminated."
[1]: https://git.io/JePjn
The :grpc module will soon be renamed to :daemon. These two modules
represent two separate and equal modes of running bisq, either as a
desktop GUI or as a daemon. They are both applications, and one should
not depend on the other as it would be illogical and confusing to model
things that way. The reason for the current dependency from :desktop to
:grpc is because :grpc is the home of BisqGrpcServer. This change moves
this class up to :core, in a new bisq.core.grpc package, such that both
the :desktop and :daemon applications can pull it in cleanly.
The CoreApi 'facade' that BisqGrpcServer uses to abstract away bisq
internals has been moved from bisq.core to bisq.core.grpc as well and
for the same reasons detailed in 8b30c22d6.
This change also renames the Java package for generated grpc types from
bisq.grpc.protobuf to bisq.core.grpc (the same new package that
BisqGrpcServer and CoreApi now live in). Again, this is for reasons of
cohesion: BisqGrpcServer is the only user of these grpc-generated types,
and they should logically live in the same package (even if they
physically live in separate source dirs at the build level).
There are two structural / organizational reasons for this move:
1. References from one package to another should always be upward or
lateral, never downward, as the latter causes package cycles (aka
'tangles') which damage the suppleness and understandability of a large
codebase. Prior to this change the high-level bisq.core.CoreModule
class imported many classes from child packages like
bisq.core.{btc,dao,user,util}, etc. By moving CoreModule down into the
'.app' package, it can reference all these other packages as siblings
instead of doing so as a parent.
2. the bisq.core.desktop and bisq.core.app packages are the only
locations that reference the CoreModule class. By moving the class
into bisq.core.app, greater cohesion is acheived, again making the
codebase that much easier to read and understand.
Relevant issue thread: #3753
Currently the daily burnt BSQ chart under 'DAO -> Facts and Figures' is
distorted by outliers. This introduces a 'Zoom to inliers' toggle (off
by default), which, when toggled on, effectively zooms the chart to
inliers, thus removing the distortion. Also, a moving average is
plotted, to further improve the chart's readibility.
The chart is also changed from an area chart to a line chart, on the
presumption that it was an area chart for cosmetic reasons, but now that
there are two series in it (the moving average was added) an area chart
makes less sense.
Another noteworthy change is that the other chart in the screen, monthly
issued BSQ, has its Y axis set to start at zero, so as to improve
readability. This might seem outside the scope of this commit, but the
other changes involved some refactoring, which involved cleaning up some
duplicated logic, which involved configuring both of these charts
together, which involved forcing zero to be on the axis.
This implementation mixes some plotting logic (responsible for zooming
in on inliers) into the view logic, because I opted to implement said
zooming as an external manipulation of a chart's axis. I chose this in
favor of implementing a new Chart, because it would have required
including multiple large classes (relevant JavaFX's classes can't be
ergonomically extended) to the code base. I presumed that my chosen
solution will be easier to maintain.
I am not entirely happy with this choice and can see myself introducing
some plotting-related classes to encapsulate creating charts like these,
thus unmixing plotting logic from view logic. In the meantime this is a
working solution, and I plan to continue working on these charts in the
near future.
If the user entered an invalid hostname for a custom BTC node, such as a
V3 onion address, after restarting Bisq they would be presented with an
error due to the node being unreachable and unable to continue nor
correct the config.
So now a warning message will be shown in this situation informing the
user of an invalid config and once they restart their client they will
connect to the provided BTC nodes.
Fixes#3137
The previous commit introduces the BisqGrpcServer as a proof of concept,
but it is not yet ready for production use. This commit removes the
`--desktopWithGrpcApi` option that starts the gRPC server until such
time that it is production-ready.
This change also removes the `--desktopWithHttpApi` option for starting
an HTTP API server. The option has been in place for some time, but it
was 'false advertising' in the sense that nothing actually happened if
the user specified it, because there is in fact no HTTP API
implementation to be started.
Note that when the gRPC API option is reintroduced, it will be renamed
to `--rpcserver` or similar, following the convention in Bitcoin Core.
This commit introduces a new `grpc` module including the following key
types:
- BisqGrpcServer: The API implementation itself, along with generated
gRPC Response/Reploy types defined in grpc/src/main/proto/grpc.proto.
- BisqGrpcServerMain: A 'headless' / daemon-like entry point for
running a Bisq node without the JavaFX desktop UI.
- BisqGrpcClient: A simple, repl-style client for the API that allows
the user to exercise the various endpoints as seen in the example
below.
In the `desktop` module, the BisqAppMain class has been modified to
start a BisqGrpcServer instance if the `--desktopWithGrpcApi` option has
been set to `true`.
In the `core` module, a new `CoreApi` class has been introduced
providing a kind of comprehensive facade for all Bisq functionality to
be exposed via the RPC API.
How to explore the proof of concept:
1. Run the main() method in BisqAppMain providing
`--desktopWithGrpcApi=true` as a program argument or alternatively, run
the main() method in BisqGrpcServerMain, where no special option is
required. In either case, you'll notice the following entry in the log
output:
INFO bisq.grpc.BisqGrpcServer: Server started, listening on 8888
2. Now run the main() method in BisqGrpcClient. Once it has started up
you are connected to the gRPC server started in step 1 above. To
exercise the API, type `getVersion` via stdin and hit return. You
should see the following response:
INFO bisq.grpc.BisqGrpcClient - 1.2.4
Likewise, you can type `getBalance` and you'll see the following
response:
INFO bisq.grpc.BisqGrpcClient - 0.00 BTC
and so forth for each of the implemented endpoints. For a list of
implemented endpoints, see BisqGrpcServer.start().
Note once again that the code here is merely a proof of concept and
should not be considered complete or production-ready in any way. In a
subsequent commit, the `--desktopWithGrpcApi` option will be disabled in
order to avoid any potential production use.
The content of this commit is the result of squashing a number of
commits originally authored by chimp1984 in the `chimp1984` fork's `grpc`
branch.
Co-authored-by: Chris Beams <chris@beams.io>
