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.
This reverts commit 26c053dae8 because
Kotlin compilation slows down the build, was applied too broadly to all
modules instead of just the one that needed it, and most importantly
because we never actually went ahead with converting anything of
importance to Kotlin. The commit being reverted was basically a demo,
converting a single test type to show what kind of difference it would
make.
We already have a garbage collection thread that runs every minute
to clean up items. Doing it again during onDisconnect is an unnecessary
optimization that adds complexity and caused bugs.
For example, the original implementation did not handle the sequence
number map correctly and was removing entries during a stream iteration.
This also reduces the complexity of testing. There is one code path
responsible for reducing ttls and one code path responsible for
expiring entries. Much easier to reason about.
1. Remove delete during stream iteration
2. Minimize branching w/ early returns for bad states
3. Use stream filter for readability
4. Implement additional checks that should be done when removing entries
Before refactoring the function ensure the tests cover all cases. This
fixes a bug where the payload ttl was too low in some instances causing
backDate to do no work when it should.
We had a small memory leak in the code base. Namely, there have been some
threadpools in use but not shutdown when they have been no longer needed.
Result was that the threads and the parent threads have been kept alive
which lead to hundreds of stale threads over the course of several days.
isDataOwner is used when deciding how many peer nodes should receive
a BroadcastMessage. If the BroadcastMessage originated
on the local node it is sent to ALL peer nodes with a small delay.
If the node is only relaying the message (it originated on a different
node) it is sent to MAX(peers.size(), 7) peers with a delay that is
twice as long.
All the information needed to determine whether or not the
BroadcastMessage originated on the local node is available at the final
broadcast site and there is no reason to have callers pass it in.
In the event that the sender address is not known during broadcast (which
is only a remote possibility due to how early the local node address
is set during startup) we can default to relay mode.
This first patch just removes the deep parameters. The next will remove
everything else. There is one real change in LiteNodeNetworkService.java
where it was using the local node when it should have been using the
peer node. This was updated to the correct behavior.
Now that more callers have moved internal, the public facing API
can be cleaner and more simple. This should lead to a more maintainable
API and less sharp edges with future development work.
Now that the only user is internal, the API can be made private and the
tests can be removed. This involved adding a few test cases to
processGetDataResponse to ensure the invalid hash size condition was
still covered.
The only two users of this constructor are the fromProto path which
now creates an empty Capabilities object similar to GetDataResponse.
The other internal usage of Capabilities.app which is initialized to empty.
The only two users of this constructor are the fromProto path which
already creates an empty Capabilities object if one is not provided and
the internal usage of Capabilities.app which is initialized to empty.
Remove the @Nullable so future readers aren't confused.
Checking for null creates hard-to-read code and it is simpler to just
create an empty set if we receive a pre-v0.6 GetDataResponse protobuf
message that does not have the field set.
Write a few integration test that exercises the exercise interesting
synchronization states including the lost remove bug. This fails
with the proper validation, but will pass at the end of the new feature
development.
Previously, multiple handlers needed to signal off one global variable.
Now, that this check is inside the singleton P2PDataStorage, make it
non-static and private.
Now that we want to unit test the GetData path which has different
behavior w.r.t. broadcasts, the tests need a way to verify that
state was updated, but not broadcast during an add.
This patch changes all verification function to take each state update
explicitly so the tests can do the proper verification.
Introduce a generic function that can be used to filter
Map<ByteArray, PersistableNetworkPayload> or
Map<ByteArray, ProtectedStorageEntry>.
Used to deduplicate the GetData code paths and ensure the logic is the
same between the two payload types.
Move the capability check inside the stream operation. This should
improve performance slightly, but more importantly it makes the
two filter functions almost identical so they can be combined.
The appendOnlyDataStoreService and map already have unique keys that
are based on the hash of the payload. This would catch instances
where:
PersistableNetworkPayload
- None: The key is based on ByteArray(payload.getHash()) which is the
same as this check.
ProtectedStorageEntry
- Cases where multiple PSEs contain payloads that have equivalent
hashCode(), but different data.toProtoMessage().toByteArray().
I don't think it is a good idea to keep 2 "unique" methods on
payloads. This is likely left over from a time when
Payload hashCode() needed to be different than the hash of
the payload.
