Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow downloading guardian config #4415

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

elsirion
Copy link
Contributor

@elsirion elsirion commented Feb 24, 2024

Fixes #4039 on the fedimintd side, also needs a UI feature.

@EthnTuttle, @Kodylow how hard would that be to integrate on the frontend side? It seems possible to download a file "from memory", although in the typical web tech hacky way 馃檲

For manual testing you can run

fedimint-cli --our-id 0 --password pass admin guardian-config-backup | jq -r .tar_archive_bytes | xxd -r -p  > backup.tar

it decodes the returned hex and writes it to backup.tar. Comparing the files they look reasonably similar (some differences due to non-pretty encoding):

$ ls -la target/devimint/fedimintd-0/
total 200
drwxr-xr-x  3 user users   4096 Feb 25 21:19 .
drwxr-xr-x 13 user users   4096 Feb 25 21:19 ..
-rw-r--r--  1 user users  59805 Feb 25 21:19 client.json
-rw-r--r--  1 user users 103889 Feb 25 21:19 consensus.json
drwxr-xr-x  2 user users   4096 Feb 25 21:42 database
-rw-r--r--  1 user users    898 Feb 25 21:19 local.json
-rw-r--r--  1 user users      4 Feb 25 21:19 password.private
-rw-r--r--  1 user users  10454 Feb 25 21:19 private.encrypt
-rw-r--r--  1 user users     22 Feb 25 21:19 private.salt

$ tar -tvf backup.tar 
---------- 0/0             628 1970-01-01 01:00 local.json
---------- 0/0          100143 1970-01-01 01:00 consensus.json
---------- 0/0              22 1970-01-01 01:00 private.salt
---------- 0/0           10454 1970-01-01 01:00 private.encrypt

TODO here: testing. I'm thinking of deleting the config of a guardian, replacing it with the tar archive versions and restarting it to see if it can start. I hope devimint supports such shenanigans.

@elsirion elsirion changed the title WIP: allow downloading guardian config Allow downloading guardian config Feb 25, 2024
@elsirion elsirion requested a review from dpc February 25, 2024 20:36
@EthnTuttle
Copy link
Contributor

EthnTuttle commented Feb 26, 2024

fedimint-cli --our-id 0 --password pass admin guardian-config-backup | jq -r .tar_archive_bytes | xxd -r -p > backup.tar

Yikes! bytes embedded in a JSON file. Not exactly how I'd serve a binary file but I don't see why this can't work in the UI. I assume it was easy to integrate with the existing websockets API instead of spinning up and entire web server?

@elsirion
Copy link
Contributor Author

elsirion commented Feb 26, 2024

I assume it was easy to integrate with the existing websockets API instead of spinning up and entire web server?

Yes, exactly that 馃槄 馃槶

And since this will be called once in a blue moon efficiency wasn't really a concern at all. It actually gets worse: in the hex-encoded tar archive there is the private.encrypted config which is itself hex-encoded for some reason 馃お

@@ -183,6 +189,7 @@ tests_to_run_in_parallel=(
"devimint_cli_test_single"
"load_test_tool_test"
"recoverytool_tests"
"guardian_backup"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradleystachurski /@dpc am I holding it correctly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is perfect. Do you know if this test takes a while? If so, we might want to move it higher up so parallel starts it sooner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, running it locally was under 30s I'd say

@elsirion elsirion force-pushed the 2024-02-backup-guardian branch 2 times, most recently from ea0aba6 to 2f0db52 Compare February 29, 2024 12:22
@elsirion elsirion marked this pull request as ready for review February 29, 2024 12:49
@elsirion elsirion requested review from a team as code owners February 29, 2024 12:49
@@ -89,6 +89,12 @@ function latency_test_restore() {
}
export -f latency_test_restore

function guardian_backup() {
# guardian-backup-test runs a degraded federation, so we need to override FM_OFFLINE_NODES
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤

@elsirion
Copy link
Contributor Author

@elsirion: bump API version!!!

This allows restoring single, completely lost guardians
(DB and config gone). If fewer or equal to f guardians
are lost restoring can happen fairly automatic as shown
in the devimint test.

For more than f guardians we'd need to implement a
special procedure that lets the restoring ones trust
the remaining ones for the correctness of consensus
items from the ongoing session.
@elsirion elsirion requested a review from maan2003 March 1, 2024 07:42
@@ -1812,6 +1813,142 @@ pub async fn recoverytool_test(dev_fed: DevFed) -> Result<()> {
Ok(())
}

pub async fn guardian_backup_test(dev_fed: DevFed, process_mgr: &ProcessManager) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test

Copy link
Member

@maan2003 maan2003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack devimint changes

Copy link
Member

@bradleystachurski bradleystachurski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Comment on lines +1823 to +1824
if VersionReq::parse("<0.3.0-alpha")?.matches(&fedimint_cli_version)
|| VersionReq::parse("<0.3.0-alpha")?.matches(&fedimintd_version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good candidate to tag with

// TODO(support:v0.2)

#4424

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do a follow-up for your nits :)

};

write_file("backup.tar", &backup_tar);
write_file("password.private", "pass".as_bytes());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] It might be more obvious how this filename is special using the pub const

write_file(
    fedimint_server::config::io::PLAINTEXT_PASSWORD,
    "pass".as_bytes(),
);

@@ -357,6 +357,12 @@ rec {
buildPhaseCargoCommand = "patchShebangs ./scripts ; ./scripts/tests/latency-test.sh";
};

guardianBackupTest = craneLibTests.mkCargoDerivation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For devimint tests should we continue adding these? I believe we're only using ciTestAllBase which will handle calling each individual test.

cc: @dpc

@elsirion elsirion added this pull request to the merge queue Mar 1, 2024
Merged via the queue into fedimint:master with commit b300ee8 Mar 1, 2024
20 checks passed
@elsirion elsirion deleted the 2024-02-backup-guardian branch March 1, 2024 21:37
@@ -208,7 +208,7 @@ impl ServerConfig {
pub fn supported_api_versions() -> SupportedCoreApiVersions {
SupportedCoreApiVersions {
core_consensus: CORE_CONSENSUS_VERSION,
api: MultiApiVersion::try_from_iter([ApiVersion { major: 0, minor: 1 }])
api: MultiApiVersion::try_from_iter([ApiVersion { major: 0, minor: 2 }])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need to bump the client core api version?

https://github.com/fedimint/fedimint/blob/427f09b/fedimint-client/src/lib.rs#L659

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, we'd do that once we drop support for 0.0 afaik. @dpc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Make guardian config+keys backupable via the admin UI
6 participants