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.
Remove the dependence on the connection object by having the handler
pass in the peer's capabilities. This now allows unit testing of
buildGetDataResponse without any connection dependencies.
Move the logging that utilizes connection information into the request
handler. Now, buildGetDataResponse just returns whether or not the list
is truncated which will make it easier to test.
Changed the log to reference getDataResponse instead of getData. Now
that we might truncate the response, it ins't true that this is exactly
what the peer asked.
These are identical test cases to the requestHandler tests, but with much
fewer dependencies. The requestHandler tests will eventually be deleted,
but they are going to remain throughout development as an extra safety
net.
As part of changing the GetData path, we want to move all creation
and processing of GetData messages inside P2PDataStorage. This will allow
easier unit testing of the behavior as well as cleaner code in the
request handlers that can just focus on nonces, connections, etc.
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.
Problem: previously, in order to completely shut down a running
localnet, users had to attach to their 'localnet' screen and kill (^C)
each process, then quit and kill the entire screen session.
Solution: this change introduces an 'undeploy' target that automates
sending the ^C to each screen window followed by sending screen's 'kill'
command to any remaining windows, thus killing the entire 'localnet'
screen session.
The result is that users may now run the following two commands in
succession any number of times to bring their localnet up and down (to
'deploy' and 'undeploy' their localnet).
# bring up localnet
$ make deploy
# use localnet to test, develop, etc...
# bring down localnet
$ make undeploy
* Add zh-hant translations (Chinese Traditional)
* Add Chinese Traditional file to update_translations.sh script
ACKs for top commit:
@ripcurlx:
ACK 4dab4fc25e
* Fix language tags so script and regional variants work correctly
* Adjust update_translations.sh language tags for script/regional variants
ACKs for top commit:
@ripcurlx:
ACK 089232716d
In commit 5fb4b21 ("Refine deploy target..."), the 'build' target was
made normal, i.e. non-phony, but on further review it does in fact make
sense to declare 'build' phony, such that it is run no matter the status
of the root-level 'build' directory, but for different reasons.
Previously, we had been considering the presence of 'build' directory as
a reasonable proxy for determining whether the `./gradlew build` had
been run. If the directory was present, we considered the 'build' target
up-to-date. If not, then we would re-run `./gradlew build`. This is all
sensible enough, except for the fact that the root-level 'build'
directory has almost nothing to do with the actual output of `./gradlew
build`. Gradle does output 'build' directories, but in the respective
subdirectory for each module of the project. After `./gradlew build` has
been run, we would see a 'desktop/build' directory, a 'seednode/build'
directory and so forth. It just so happens that a root-level 'build'
directory was getting created at all due to idiosyncracies of a
particular Kotlin plugin.
This commit updates the makefile to better respect this reality by:
- preserving the 'build' target but marking it once again as PHONY
- introducing new 'seednode/build' and 'desktop/build' targets that
trigger './gradlew :seednode:build` and ./gradlew :desktop:build`
commands respectively.
- making 'build' depend on these two new targets
In light of this realization of flawed thinking about the root-level
build dir, this change also restores `make clean` to calling `./gradlew
clean` instead of `rm -rf build`.
Problem: Bitcoind Core v0.90.0 changed the default value of its
'peerbloomfilters' option from 1 to 0, now disabling them by default.
Bisq requires bloom filters be enabled on the Bitcoin node(s) it
communicates with, so users who are running >= v0.90 would get errors
when attempting to run `make bitcoind` with that target's current
recipe.
Solution: This change explicitly sets the 'peerbloomfilters' option to
1, ensuring it is enabled in any case. Note that this option has existed
in Bitcoin Core since v0.12.0, so there is no real concern for this new
option breaking users that are still on 0.18.x or even much earlier.
Problem: Prior to this change, it was necessary to first create and
attach to a screen session and then to run `make deploy` within it. This
meant extra steps for the user and was generally error-prone.
Solution: Usage of screen has been refined such that a screen session
named 'localnet' is created on the users behalf without any need to
attach to it. Individual node deployment targets such as `make
bitcoind`, `make alice`, et al. are issued to new windows within the
localnet screen session, and the user is free to attach or not whenever
they choose. The result is that a new user can clone the repository and
type nothing more than `make deploy` to get up and running with their
localnet.
This also reverts the changes in commit 97dd342e5 ("Make build target
phony") for the following reasons:
- As mentioned in that commit message, Gradle was not deleting the its
'build' directory when running `gradle clean`, meaning that the
'build' target was always up-to-date, even after running `make
clean`. This made it impossible to get a correct rebuild workflow. On
analysis, howewer, this situation was because of a badly behaving
Kotlin plugin not cleaning up after itself, leaving a subdirectory at
build/kotlin and preventing the build directory itself from being
deleted altogether. To address this, the `make clean` target has been
updated to `rm -rf build` instead of calling `build gradle`. While
it's a workaround until we back out the Kotlin changes that caused
this, it does have the added benefit of being faster than invoking
`gradle clean`.
- By making the 'build' target PHONY, this meant that `./gradlew build`
was getting invoked every time a dependent target was called. For
example, `make alice` depends on the 'setup' target, which in turn
depends on the 'build' target. When calling such targets in
isolation, this arrangement works out fine, because the phony 'build'
target always runs, invoking `./gradle build`, and the Gradle build
completes quickly assuming everything is up-to-date. The problem
arises when calling a number of these targets in rapid succession, as
we do when calling `make deploy` and running each individual node
target in its own screen window. This causes contention in two ways.
The first is that these multiple, simultaneous Gradle processes
compete for access to an available Gradle daemon, and because each
process needs its own, it ends up that as many Gradle daemons get
created as Bisq nodes we need to deploy (5 in total). This is a big
waste of time and resources. The second way it causes not only
contention but outright failure is that each of these builds are
operating in the same directory, and while most aspects of the build
are in fact up-to-date and therefore not modified in any way, there
are exceptions to this rule. The result is that build artifacts, e.g.
jars are getting deleted and rebuilt from underneath competing Gradle
processes, and all manner of chaos ensues, such as NoClassDefFound
errors and much more. This change (reverting 'build' back to a
normal, non-phony target) avoids these problems entirely. When
running `make deploy`, we run the 'build' target once as a function
of the 'deploy' target depending on it. At this point, the 'build'
directory exists, and all subsequent node deployment targets, e.g.
'alice', 'bob', etc do not re-run the build target because it is
up-to-date. For workflows where the user definitely wants to rebuild
prior to redeploying a given node, they can either run `make
clean-build`, or drop down to issuing Gradle build commands directly,
e.g. `./gradlew :desktop:build` followed by `make desktop`.
