Have a dummy in-RAM coordinator for the functional tests #402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
darosior
force-pushed
the
dummy_coordinator
branch
2 times, most recently
from
028fe41 to
f86fd92
Compare
|
Master, This branch, |
|
TODO: update the README for the tests |
darosior
force-pushed
the
dummy_coordinator
branch
4 times, most recently
from
daa2f85 to
919f725
Compare
darosior
commented
May 12, 2022
darosior
force-pushed
the
dummy_coordinator
branch
3 times, most recently
from
ee11b60 to
4ce781c
Compare
Using a single Postgre backend and a regular coordinator per instance worked for a time, but it's getting to its limits. The functional tests, when ran in parrallel, put an unreasonable load on the Postgre backend. This leads to spurious failures and timeouts that are hard to deal with in the `coordinatord` or `revaultd` binaries themselves without optimizing for functional tests.. Which isn't a realistic load. I tried to implement a cache to get_sigs requests using a proxy between the revaultds and the Coordinator in a single instance. Although it (partially) solved the timeouts in the RPC commands that hit the coordinator, it slowed tests down by a lot. Instead, here we implement a fully in-RAM coordinator for the lowest possible response time. After all, we don't need to persist the data for the functional tests and we can afford a few more MB of RAM usage. Of course, we keep the possibility of running using the real coordinator, but we would probably not do so in the CI that is already too slow. 3 tests would be skipped, though. But not the most important ones, and we don't only rely on CI anyways. As a by-product, we can now run the functional tests without having to start a Postgre backend beforehand. I think it can help new contributors a lot.
We can afford shorter intervals with the dummy coordinator.
darosior
force-pushed
the
dummy_coordinator
branch
from
4ce781c to
2179813
Compare
darosior
commented
May 19, 2022
Comment on lines
+196
to
+218
| if method == "sig": | ||
| # FIXME: the field name is a misimplementation! | ||
| # TODO: mutex | ||
| if params["id"] not in self.sigs: | ||
| self.sigs[params["id"]] = {} | ||
| self.sigs[params["id"]][params["pubkey"]] = params["signature"] | ||
| # TODO: remove this useless response from the protocol | ||
| resp = {"result": {"ack": True}, "id": request["id"]} | ||
| self.send_msg(client_fd, client_noise, json.dumps(resp)) | ||
|
|
||
| elif method == "get_sigs": | ||
| # FIXME: the field name is a misimplementation of the protocol! | ||
| txid = params["id"] | ||
| sigs = self.sigs.get(txid, {}) | ||
| resp = {"result": {"signatures": sigs}, "id": request["id"]} | ||
| self.send_msg(client_fd, client_noise, json.dumps(resp)) | ||
|
|
||
| elif method == "set_spend_tx": | ||
| for outpoint in params["deposit_outpoints"]: | ||
| self.spend_txs[outpoint] = params["transaction"] | ||
| # TODO: remove this useless response from the protocol | ||
| resp = {"result": {"ack": True}, "id": request["id"]} | ||
| self.send_msg(client_fd, client_noise, json.dumps(resp)) |
There was a problem hiding this comment.
Need to update the messages after revault_net update!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#393 further increases the load on the coordinators and the Postgre backend. I have considered a number of things (a thread pool in the coordinator, polling vaults in parrallel in the sigfetcher, a cache for
get_sigsresult in the functional tests, ...). I think in the end we just have to face the fact that our protocol is (purposefully) inefficient and signature fetching is slow. Still, this is a small improvement on the state of things that helps to prevent timeouts in #393.Using a single Postgre backend and a regular coordinator per instance
worked for a time, but it's getting to its limits. The functional tests,
when ran in parrallel, put an unreasonable load on the Postgre backend.
This leads to spurious failures and timeouts that are hard to deal with
in the
coordinatordorrevaultdbinaries themselves withoutoptimizing for functional tests.. Which isn't a realistic load.
I tried to implement a cache to get_sigs requests using a proxy between
the revaultds and the Coordinator in a single instance. Although it
(partially) solved the timeouts in the RPC commands that hit the
coordinator, it slowed tests down by a lot.
Instead, here we implement a fully in-RAM coordinator for the lowest
possible response time. After all, we don't need to persist the data for
the functional tests and we can afford a few more MB of RAM usage.
Of course, we keep the possibility of running using the real
coordinator, but we would probably not do so in the CI that is already
too slow. 3 tests would be skipped, though. But not the most important
ones, and we don't only rely on CI anyways.
As a by-product, we can now run the functional tests without having to
start a Postgre backend beforehand. I think it can help new contributors
a lot.