feat: restart federation setup #3669

okjodom · 2023-11-21T02:16:07Z

After setting config gen params and before starting consensus, any peer can initiate a restart of the federation setup using restart_federation_setup rpc.

server-side behavior

This API sets the peer server in SetupRestarted status.
The Leader server waits for all peers to progress to SetupRestarted.
Leader then automatically makes progress to AwaitingPassword.
Followers watch leader status and when they see AwaitingPassword, they also make progress to the same state

All peers can then begin the setup process

suggested user experience

While the server is in any of these states SharingConfigGenParams | ReadyForConfigGen | ConfigGenFailed | VerifyingConfigs | VerifiedConfigs, the setup UI should watch peer information in consensus status. Should any peer be in SetupRestarted, the setup UI should immediately prompt the user to initiate restart by make a call to restart_federation_setup. Suggestion here it to make this some blocking but interactive UX

closes #3535

codecov · 2023-11-22T08:57:45Z

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (e882197) 57.13% compared to head (4471ada) 57.38%.
Report is 13 commits behind head on master.

❗ Current head 4471ada differs from pull request most recent head f82f234. Consider uploading reports for the commit f82f234 to get more accurate results

Files	Patch %	Lines
fedimint-server/src/config/api.rs	93.30%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3669      +/-   ##
==========================================
+ Coverage   57.13%   57.38%   +0.25%     
==========================================
  Files         193      193              
  Lines       42861    42778      -83     
==========================================
+ Hits        24487    24548      +61     
+ Misses      18374    18230     -144

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elsirion

Looks good, does anyone more familiar with the setup take a look? @justinmoon

fedimint-server/src/config/api.rs

elsirion · 2023-11-22T10:17:58Z

fedimint-server/src/config/io.rs

+    remove_existing_file(path.join(LOCAL_CONFIG).with_extension(JSON_EXT))?;
+    remove_existing_file(path.join(CONSENSUS_CONFIG).with_extension(JSON_EXT))?;
+    remove_existing_file(path.join(CLIENT_INVITE_CODE_FILE))?;
+    remove_existing_file(path.join(CLIENT_CONFIG).with_extension(JSON_EXT))?;
+    remove_existing_file(path.join(PRIVATE_CONFIG).with_extension(ENCRYPTED_EXT))?;


This is very brittle if we ever add any files. In another PR we should introduce a list of files that FM potentially creates and have a test that checks that data dirs contain only these and nothing more.

tracking here #3692

fedimint-server/src/config/api.rs

elsirion · 2023-11-24T13:45:57Z

CI is failing and it seems related to the PR

okjodom · 2023-11-24T16:04:39Z

@elsirion , now green on CI

elsirion

Looks good to me, but I'll leave @justinmoon a chance to take another look

fedimint-server/src/config/api.rs

elsirion · 2023-11-24T20:06:58Z

One concern is that we could accidentally delete important key material if we ever called these delete fns by accident somewhere. To reduce that problem I'd propose to chmod -w config files right before starting the consensus, so that deletes are prevented.

dpc · 2023-11-26T21:31:58Z

One concern is that we could accidentally delete important key material if we ever called these delete fns by accident somewhere. To reduce that problem I'd propose to chmod -w config files right before starting the consensus, so that deletes are prevented.

That's a good point, but probably best practice to keep the in-progress things in a temp directory (randomized) and rename it atomically on success.

okjodom · 2023-11-27T16:15:35Z

One concern is that we could accidentally delete important key material if we ever called these delete fns by accident somewhere. To reduce that problem I'd propose to chmod -w config files right before starting the consensus, so that deletes are prevented.

That's a good point, but probably best practice to keep the in-progress things in a temp directory (randomized) and rename it atomically on success.

marked as draft while applying this

elsirion · 2023-11-29T07:35:45Z

Are you on this @okjodom? @justinmoon should this get into 0.2?

okjodom · 2023-11-29T07:37:56Z

Are you on this @okjodom? @justinmoon should this get into 0.2?

Trying to debug the failing reconnect test now. Been up and down

I think an api changed?

okjodom · 2023-11-29T07:41:46Z

Ps, this was originally marked for 0.2.1
I bumped it after first round of reviews because thinking I'd be able to land it fast

okjodom · 2023-11-29T16:37:26Z

@elsirion, @justinmoon I'm tracking two issues as follow-ups to this. Take a look

okjodom · 2023-12-11T17:25:57Z

fixes #3788

fedimint-server/src/config/api.rs

- adds a helper for asserting the config server is in one of several expected status

- add restart_federation_setup rpc used by federation members for restarting the setup process - leader waits for all peers to progess to SetupRestarted. it then progresses to AwaitingPassword - each follower wait for leader to progress to AwaitingPassword before proceeding to AwaitingPassword

- add tests that cover restart of federation setup mid-way - this test restarts the federation right after SharingConfigGenParams

elsirion

Looks mostly good, left some questions.

I don't think this is stability-relevant, we don't need to guarantee stability of the setup API, just need to release the admin UI in lockstep (but any reasonable deployment should use the admin UI that fits the Fedimint version).

I also didn't find any Rust API breakage, so in principle this could be backported to 0.2 if we saw the need (I'd like to avoid that though).

elsirion · 2024-01-11T10:13:43Z

fedimint-server/src/config/api.rs

+        let leader = {
+            let expected_status = [
+                ServerStatus::SharingConfigGenParams,
+                ServerStatus::ReadyForConfigGen,
+                ServerStatus::ConfigGenFailed,
+                ServerStatus::VerifyingConfigs,
+                ServerStatus::VerifiedConfigs,
+            ];
+            let mut state = self.require_any_status(&expected_status)?;
+
+            let cfg_staging_dir = self.data_dir.join(CONFIG_STAGING_DIR);
+            self.remove_staged_configs(&cfg_staging_dir)?;
+
+            state.status = ServerStatus::SetupRestarted;
+            info!(
+                target: fedimint_logging::LOG_NET_PEER_DKG,
+                "Update config gen status to 'Setup restarted'"
+            );
+            // Create a WSClient for the leader
+            state
+                .local
+                .clone()
+                .and_then(|local| local.leader_api_url.map(WsAdminClient::new))
+        };


What's the reason to put this into a separate block?

this allows us to acquire lock on self.state, use it and and releasing lock, before we call update_leader. update leader creates a separate lock on state

I didn't remember that require_any_status returned a mutex guard, makes sense!

elsirion · 2024-01-11T10:18:39Z

fedimint-server/src/config/api.rs

+    fn reset(&mut self) {
+        self.config = None;
+        self.peers = Default::default();
+        self.auth = None;
+        self.requested_params = None;
+        self.status = ServerStatus::AwaitingPassword;
+        self.local = None;
+
+        info!(
+            target: fedimint_logging::LOG_NET_PEER_DKG,
+            "Reset config gen state"
+        );
+    }


Do we need to reset any on-disk state? I think the PASSWORD file has been written already at that point?

at this point, PASSWORD or any other config file is already written to a file in a temporary dir. remove_staged_config() resets disk state by deleting the whole dir, and this function finally resets server state

justinmoon

LGTM

elsirion · 2024-01-15T06:26:33Z

Thx for bearing with us, the release took a lot of bandwidth and this PR a few iterations, but I think we arrived at a good result at the end!

okjodom force-pushed the restart-setup branch from f50471e to 1554057 Compare November 21, 2023 20:02

okjodom marked this pull request as ready for review November 21, 2023 20:29

okjodom requested review from a team as code owners November 21, 2023 20:29

elsirion previously approved these changes Nov 22, 2023

View reviewed changes

justinmoon reviewed Nov 22, 2023

View reviewed changes

fedimint-server/src/config/api.rs Outdated Show resolved Hide resolved

justinmoon reviewed Nov 22, 2023

View reviewed changes

fedimint-server/src/config/api.rs Show resolved Hide resolved

okjodom dismissed elsirion’s stale review via e55dd5a November 22, 2023 17:09

okjodom force-pushed the restart-setup branch from 1554057 to e55dd5a Compare November 22, 2023 17:09

okjodom requested review from justinmoon and elsirion November 22, 2023 17:29

okjodom force-pushed the restart-setup branch from e55dd5a to 0a7a8c7 Compare November 24, 2023 15:35

elsirion previously approved these changes Nov 24, 2023

View reviewed changes

fedimint-server/src/config/api.rs Outdated Show resolved Hide resolved

elsirion added the backport releases/v0.2 label Nov 24, 2023

okjodom marked this pull request as draft November 27, 2023 16:14

okjodom dismissed elsirion’s stale review via bec4281 November 27, 2023 17:05

okjodom force-pushed the restart-setup branch 3 times, most recently from beff7a8 to d065f9c Compare November 28, 2023 17:48

okjodom force-pushed the restart-setup branch from d065f9c to 7ad169a Compare November 29, 2023 08:28

elsirion removed the backport releases/v0.2 label Nov 29, 2023

okjodom mentioned this pull request Nov 29, 2023

Safely remove staged config artifacts when we start consensus #3788

Open

okjodom force-pushed the restart-setup branch from 7ad169a to c4447a1 Compare November 29, 2023 16:07

okjodom marked this pull request as ready for review November 29, 2023 16:36

okjodom force-pushed the restart-setup branch 2 times, most recently from 4471ada to e7dbb10 Compare December 10, 2023 15:56

elsirion reviewed Dec 12, 2023

View reviewed changes

fedimint-server/src/config/api.rs Outdated Show resolved Hide resolved

fedimint-server/src/config/api.rs Outdated Show resolved Hide resolved

okjodom added 2 commits December 12, 2023 04:17

refactor: expect one of several expected status

5249de8

- adds a helper for asserting the config server is in one of several expected status

okjodom force-pushed the restart-setup branch from e7dbb10 to 6de7e8d Compare December 13, 2023 11:49

okjodom added 2 commits December 13, 2023 04:07

test: peers can restart federation setup

6632312

- add tests that cover restart of federation setup mid-way - this test restarts the federation right after SharingConfigGenParams

fix: update tests to cover VerifiedConfigs status

f82f234

okjodom force-pushed the restart-setup branch from 6de7e8d to f82f234 Compare December 13, 2023 12:11

okjodom requested a review from elsirion January 7, 2024 21:38

elsirion approved these changes Jan 11, 2024

View reviewed changes

justinmoon approved these changes Jan 11, 2024

View reviewed changes

elsirion added this pull request to the merge queue Jan 15, 2024

Merged via the queue into fedimint:master with commit 9ec5418 Jan 15, 2024
18 checks passed

okjodom mentioned this pull request Jan 22, 2024

Restart federation flaky? #4099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: restart federation setup #3669

feat: restart federation setup #3669

okjodom commented Nov 21, 2023 •

edited

codecov bot commented Nov 22, 2023 •

edited

elsirion left a comment

elsirion Nov 22, 2023

okjodom Nov 22, 2023

elsirion commented Nov 24, 2023

okjodom commented Nov 24, 2023

elsirion left a comment

elsirion commented Nov 24, 2023

dpc commented Nov 26, 2023

okjodom commented Nov 27, 2023

elsirion commented Nov 29, 2023

okjodom commented Nov 29, 2023

okjodom commented Nov 29, 2023

okjodom commented Nov 29, 2023

okjodom commented Dec 11, 2023

elsirion left a comment •

edited

elsirion Jan 11, 2024

okjodom Jan 11, 2024

elsirion Jan 15, 2024

elsirion Jan 11, 2024

okjodom Jan 11, 2024

justinmoon left a comment

elsirion commented Jan 15, 2024

feat: restart federation setup #3669

feat: restart federation setup #3669

Conversation

okjodom commented Nov 21, 2023 • edited

codecov bot commented Nov 22, 2023 • edited

Codecov Report

elsirion left a comment

Choose a reason for hiding this comment

elsirion Nov 22, 2023

Choose a reason for hiding this comment

okjodom Nov 22, 2023

Choose a reason for hiding this comment

elsirion commented Nov 24, 2023

okjodom commented Nov 24, 2023

elsirion left a comment

Choose a reason for hiding this comment

elsirion commented Nov 24, 2023

dpc commented Nov 26, 2023

okjodom commented Nov 27, 2023

elsirion commented Nov 29, 2023

okjodom commented Nov 29, 2023

okjodom commented Nov 29, 2023

okjodom commented Nov 29, 2023

okjodom commented Dec 11, 2023

elsirion left a comment • edited

Choose a reason for hiding this comment

elsirion Jan 11, 2024

Choose a reason for hiding this comment

okjodom Jan 11, 2024

Choose a reason for hiding this comment

elsirion Jan 15, 2024

Choose a reason for hiding this comment

elsirion Jan 11, 2024

Choose a reason for hiding this comment

okjodom Jan 11, 2024

Choose a reason for hiding this comment

justinmoon left a comment

Choose a reason for hiding this comment

elsirion commented Jan 15, 2024

okjodom commented Nov 21, 2023 •

edited

codecov bot commented Nov 22, 2023 •

edited

elsirion left a comment •

edited