Add support for multiple OPRF instances in a single server #192

DJAndries · 2023-08-25T04:45:10Z

This will allow us to have separate OPRF instances for each P3A cadence: monthly, weekly and daily.

DJAndries · 2023-08-25T20:27:20Z

This PR is blocked by https://github.com/brave/reviews/issues/1381 for the new calendar-duration crate.

Cargo.toml

rillian

We've previously made separate submissions endpoints for each P3A report type. Why combine the randomness servers for each cadence, rather than having separate deployments?

How would you update the PRIVATE_KEY env variable from the key sharing design in #175 for multiple instances.

src/main.rs

rillian · 2023-08-28T21:47:19Z

src/state.rs

+    fn calculate(base_time: OffsetDateTime, instance_epoch_duration: CalendarDuration) -> Self {
+        let now = time::OffsetDateTime::now_utc();
+        let mut elapsed_epoch_count = 0;
+        let mut next_rotation = base_time + instance_epoch_duration;
+        while next_rotation < now {
+            next_rotation = next_rotation + instance_epoch_duration;
+            elapsed_epoch_count += 1;
+        }


Hmm. If our goal is to align with periodic points on the calendar, maybe it would be less convoluted to have a Daily, Weekly, Monthly enum and calculate the delay to the next point from that, instead of relying on the right interaction with the base time?

The time::Date type has next_day() and next_occurance(Weekday) methods that might be helpful here. Looks like it doesn't provide exactly what we need for months, but the Month type has next() and previous() methods which wrap around properly, so a value could be constructed that way.

Hmm. If our goal is to align with periodic points on the calendar, maybe it would be less convoluted to have a Daily, Weekly, Monthly enum and calculate the delay to the next point from that, instead of relying on the right interaction with the base time?

i'm unsure how using an enum would simplify the calculation of the initial epoch, which was the code that you highlighted. previously we used arithmetic to calculate the starting epoch, but it seems that we can no longer do this with monthly epochs due to the fact that months vary in the amount of days. i switched to an iterative approach to account for this.

Also, switching from a duration string defined by the user to an enum will make testing short epoch duration difficult, which was a concern that you mentioned here: #50 (comment)

The ability to define a custom epoch duration provides this kind of flexibility.

The time::Date type has next_day() and next_occurance(Weekday) methods that might be helpful here. Looks like it doesn't provide exactly what we need for months, but the Month type has next() and previous() methods which wrap around properly, so a value could be constructed that way.

The new small crate calendar-duration provides an abstraction layer for this.

i'm unsure how using an enum would simplify the calculation of the initial epoch

Sorry, I don't mean that it simplifies the code. I meant an enum would simplify the interface. This change does two things. It adds a parser so someone using the command line doesn't have to figure out how many seconds a week is, or whatever. That's nice. Separately, it allows specifying durations over a month in a way that aligns with calendar dates, which isn't something that was possible before, because months aren't a fixed number of seconds.

But, to get what we want, which is to have epochs rotating at the zero hour UTC on daily, weekly, and monthly cadences, we still have to think carefully about whether the --epoch-base-time we pass is a Monday and the first day of a month. (It is. Did you do that on purpose?) Seems like it would be nicer to be able to just request that directly and let the code do the math.

an enum will make testing short epoch duration difficult

I think we could still include a Seconds(u64) variant in the enum for testing or other custom intervals.

Also, would this change support rotating epochs more frequently than the calendar measurement period like we've discussed doing with P3A? What arguments would I pass to rotate three times a month?

(It is. Did you do that on purpose?)

nope, that was all you 😃

I think we could still include a Seconds(u64) variant in the enum for testing or other custom intervals.

what benefit would this provide over just specifying --epoch-duration 30s or something similar?

Also, would this change support rotating epochs more frequently than the calendar measurement period like we've discussed doing with P3A? What arguments would I pass to rotate three times a month?

This is a good question. I'm afraid not. One idea for a future PR: we could rename --epoch-duration to --report-cadence-duration and add a switch called --epochs-per-report-period. we could perform a small amount of extra arithmetic to calculate the epoch; I expect that the burden wouldn't be significant

start.sh

DJAndries · 2023-08-29T23:47:26Z

Why combine the randomness servers for each cadence, rather than having separate deployments?

a few reasons:

when I added support for multiple cadences for json measurements in the client, i was encouraged to continue to use a single host during sec/privacy review. less hosts in the client makes audits easier
less devops overhead. this is especially important now that we are implementing a key sync mechanism, which increases the complexity of our deployments. given the limited human resources we have allocated to star + devops, it's ideal to keep our deployments as lean as possible
less attestation overhead in the client. the client only needs to attest one host, instead of three. this results in slightly less network/computation overhead + less state management in the client.
constellation-processors will follow the same pattern, for reasons similar to the above.

How would you update the PRIVATE_KEY env variable from the key sharing design in #175 for multiple instances.

left a comment in the linked issue with some questions. if we do decide to move forward with the env var, i have some ideas to solve this.

rillian · 2023-08-31T17:48:25Z

Why combine the randomness servers for each cadence, rather than having separate deployments?

Thanks. All those reasons make sense to me.

We could pass the instance name through an optional instance or cadence property on the submission, similar to how the client can request a specific epoch. That would avoid the need for a separate /instance/randomness endpoint, but it would have to be a query parameter on the GET /info which isn't any more tidy.

rillian

I think there's a bug with the currentEpoch field. The value returned with randomness requests doesn't match what's in the log output or the value returned from instances/main/info.

src/handler.rs

src/state.rs

…ion strings instead of seconds for epoch durations

…art.sh, misc. improvements

rillian

I think the design here isn't what we really want, but the implementation seems fine, so approving is a bases for future feedback as we gain experience with it.

rillian · 2023-09-26T23:01:10Z

src/handler.rs

-    Json(request): Json<RandomnessRequest>,
-) -> Result<Json<RandomnessResponse>, Error> {
+#[instrument(skip(state, request))]
+async fn randomness(


If there are no await calls in here, this function could be synchronous, handling the full request in a single future. Might improve throughput, but I didn't check.

src/state.rs

DJAndries · 2023-09-26T23:09:41Z

I think the design here isn't what we really want

Can you elaborate on this? Feedback is much appreciated. Especially before we use these endpoints in production.

rillian · 2023-09-26T23:13:08Z

Just what I was trying to say before about how the epoch durations are configured. We want to align rotations with calendar periods, so I suspect something that let us say "This one rotates Sunday midnight, this one rotates on the 1st, 5th, and 18th days of the month" would be a better API.

DJAndries · 2023-09-26T23:14:23Z

Just what I was trying to say before about how the epoch durations are configured. We want to align rotations with calendar periods, so I suspect something that let us say "This one rotates Sunday midnight, this one rotates on the 1st, 5th, and 18th days of the month" would be a better API.

Gotcha, will give this some more thought

DJAndries requested a review from a team as a code owner August 25, 2023 04:45

DJAndries force-pushed the multiple-instances branch 2 times, most recently from f360faf to b3a2eec Compare August 25, 2023 20:26

DJAndries commented Aug 25, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

rillian reviewed Aug 28, 2023

View reviewed changes

DJAndries force-pushed the multiple-instances branch from 8e58dbe to 9542cdd Compare August 29, 2023 23:21

DJAndries force-pushed the multiple-instances branch from 9542cdd to 9da0d57 Compare August 30, 2023 04:08

DJAndries force-pushed the multiple-instances branch 2 times, most recently from d003522 to 4e287db Compare September 20, 2023 23:34

rillian reviewed Sep 25, 2023

View reviewed changes

src/handler.rs Outdated Show resolved Hide resolved

src/handler.rs Show resolved Hide resolved

src/state.rs Show resolved Hide resolved

DJAndries added 2 commits September 25, 2023 16:29

Add support for multiple OPRF instances in a single server, use durat…

2c96333

…ion strings instead of seconds for epoch durations

Add "list instances" endpoint to improve discoverability, reformat st…

862f8a2

…art.sh, misc. improvements

DJAndries force-pushed the multiple-instances branch from 48c9253 to 862f8a2 Compare September 25, 2023 23:29

rillian approved these changes Sep 26, 2023

View reviewed changes

rillian merged commit 9396188 into main Sep 26, 2023

rillian deleted the multiple-instances branch September 26, 2023 23:08

Add support for multiple OPRF instances in a single server #192

Add support for multiple OPRF instances in a single server #192

Uh oh!

Conversation

DJAndries commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DJAndries commented Aug 25, 2023

Uh oh!

Uh oh!

rillian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rillian Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

DJAndries Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

rillian Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

DJAndries Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DJAndries commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rillian commented Aug 31, 2023

Uh oh!

rillian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rillian left a comment

Choose a reason for hiding this comment

Uh oh!

rillian Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DJAndries commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rillian commented Sep 26, 2023

Uh oh!

DJAndries commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DJAndries commented Aug 25, 2023 •

edited

Loading

DJAndries commented Aug 29, 2023 •

edited

Loading

DJAndries commented Sep 26, 2023 •

edited

Loading