Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Persist client state #497

Merged
merged 1 commit into from
Aug 21, 2020
Merged

Persist client state #497

merged 1 commit into from
Aug 21, 2020

Conversation

Robert-Steiner
Copy link
Contributor

@Robert-Steiner Robert-Steiner commented Aug 17, 2020

Why do we need to de/serialize the client state?

Screen Shot 2020-08-17 at 14 23 49

In a mobile environment, we have no control over how long an app (and its data) stays in memory. The operating system can, for example, decide to terminate our app to reduce battery consumption. However, we are dependent on the client not losing its state and being restarted every time. The reason for this can be seen in the diagram above. Let us imagine that the client does not keep its state but is restarted every time. This circumstance is not a problem for the update client, but for the sum client. If we have a long running update phase, the state of the sum client must be kept in ram of the phone until the coordinator has moved into the sum2 phase. However, if the sum client was restarted before reaching the sum2 phase, the sum client cannot complete the task of the sum2 phase. The reason for this lies in the ephemeral secret keys that are regenerated every round as soon as a client transforms into a sum client. For this reason there must be the possibility that the state of the client can be de/serialized and, for example, re/stored from/on a local database/disk.

What changed:

  • In my first try, I wanted to add the de/serialize methods to the ClientState struct and only de/serialize the Participant<_> struct and the round_params. However this didn't work out that well because serde doesn't store any type information about the struct.
// so
bincode::serialize(&participant).unwrap();
// works well but 
let participant = bincode::deserialize(&participant).unwrap();
// does not because a type annotation was needed
// but we don't know what type we deserialize. it could be Participant<Awaiting>, Participant<Sum>, Participant<Update> or Participant<Sum2>
  • One solution is to wrap it in an enum (you can see the solution in the first commit). However, I didn't want to create another enum just for the de/serialization. So my solution was to move the Proxy and the local/global model out of the ClientState and use the ClientStateMachine enum to de/serialize the participant state and the round parameters.

  • moved the get_global_model method out of the ClientState because this function does not depend on the client state.

  • added a LocalModel trait to abstract the implementaion how the ClientState gets the local model

  • added an experimental incomplete desktop client. I wanted to test how the LocalModel trait would work in an async desktop client. I will remove it before merging.

  • removed the runtime field in the MobileClient struct. I don't think that it makes sense in the mobile client to keep the tokio runtime in memory. The app will probably not stay open so long that the runtime will be used a second time. So we only spawn one whenever we need one.

Future improvments

  • The ability to get the information which task the participant performs (sum, update, sum2, none)
    • with this information it is possible to load the local model only if the participant is an update participant
    • the implementation could look similar to the PhaseName in the state machine
  • Notify the user of the sdk when a new global model is available.
  • Error handling

more ideas in the code comments

@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #497 into master will increase coverage by 0.43%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #497      +/-   ##
==========================================
+ Coverage   55.61%   56.05%   +0.43%     
==========================================
  Files          67       67              
  Lines        3231     3206      -25     
==========================================
  Hits         1797     1797              
+ Misses       1434     1409      -25     
Impacted Files Coverage Δ
rust/src/certificate.rs 33.33% <ø> (ø)
rust/src/client/mobile_client/client.rs 0.00% <0.00%> (ø)
rust/src/client/mobile_client/mod.rs 0.00% <0.00%> (ø)
...t/src/client/mobile_client/participant/awaiting.rs 76.47% <ø> (ø)
rust/src/client/mobile_client/participant/mod.rs 0.00% <ø> (ø)
rust/src/client/mobile_client/participant/sum.rs 25.00% <ø> (ø)
rust/src/client/mobile_client/participant/sum2.rs 17.85% <ø> (ø)
...ust/src/client/mobile_client/participant/update.rs 38.46% <ø> (ø)
rust/src/crypto/sign.rs 86.66% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dbfe4d...c78a3f3. Read the comment docs.

rust/src/client/experimental/client.rs Outdated Show resolved Hide resolved
rust/src/client/experimental/client.rs Outdated Show resolved Hide resolved
@Robert-Steiner Robert-Steiner marked this pull request as ready for review August 18, 2020 08:03
Copy link
Contributor

@little-dude little-dude left a comment

Choose a reason for hiding this comment

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

Looks very good to me. I think it's a bit weird to pass this proxy around as an argument but I don't see a better solution.

rust/src/client/mobile_client/client.rs Outdated Show resolved Hide resolved
rust/src/client/mobile_client/mod.rs Show resolved Hide resolved
rust/src/client/mobile_client/mod.rs Show resolved Hide resolved
Copy link
Contributor

@little-dude little-dude left a comment

Choose a reason for hiding this comment

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

I have some concerns about storing the client state on the mobile devices though. Mobile devices offer large attack surfaces and (in my opinion) cannot be trusted. I'm not sure what the threats are exactly but we should evaluate the consequences of this data being accessed or manipulated. The PET protocol mentions "honest but curious" participants, and contains assertions about the participants behaving honestly in some places, but I'm not we can make such assertions if we store sensitive data on the phones.

@Robert-Steiner Robert-Steiner force-pushed the persist-client-state branch 3 times, most recently from 3c52059 to 8f75130 Compare August 20, 2020 12:36
@Robert-Steiner Robert-Steiner merged commit 4eca568 into master Aug 21, 2020
@Robert-Steiner Robert-Steiner deleted the persist-client-state branch August 21, 2020 13:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants