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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Fog view client #3396

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fog view client #3396

wants to merge 2 commits into from

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Jun 21, 2023

I created this client in order to dump summary stats from the fog-view response.

I got the following results:

$ ./target/docker/debug/fog_view_client --fog-view fog-view://fog.test.mobilecoin.com
...
2023-06-21 23:43:15.601404798 UTC INFO Highest processed block count: 1768001, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:37
2023-06-21 23:43:15.601854476 UTC INFO Num rng records: 31, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:38
2023-06-21 23:43:15.602249593 UTC INFO Num rng decommissioned: 23, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:39
2023-06-21 23:43:15.602408386 UTC INFO Num missed block events: 2, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:40
2023-06-21 23:43:15.602500398 UTC INFO Total missed blocks: 18, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:42

$ ./target/docker/debug/fog_view_client --fog-view fog-view://fog.prod.mobilecoinww.com
...
2023-06-21 23:44:14.992531319 UTC INFO Highest processed block count: 1653414, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:37
2023-06-21 23:44:14.992923765 UTC INFO Num rng records: 21, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:38
2023-06-21 23:44:14.993302427 UTC INFO Num rng decommissioned: 18, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:39
2023-06-21 23:44:14.993638321 UTC INFO Num missed block events: 0, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:40
2023-06-21 23:44:14.993893326 UTC INFO Total missed blocks: 0, mc.module: fog_view_client, mc.src: fog/view/connection/src/bin/client.rs:42

If I understand right, this means that mc-mainnet fog has never had a missed block event. That is frankly amazing.

TODO: I would like to be able to hit signal fog this way too.

@cbeck88 cbeck88 self-assigned this Jun 21, 2023
nick-mobilecoin
nick-mobilecoin reviewed Jun 22, 2023
View reviewed changes
Copy link
Collaborator

@nick-mobilecoin nick-mobilecoin left a comment

Choose a reason for hiding this comment

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

This change should probably be based only against master. Currently this PR is trying to both merge 4.1.0 to master as well as bring in this new client.

Mostly nit comments

fog/view/connection/src/bin/client.rs
Comment on lines +27 to +28
// Logging must go to stderr to not interfere with STDOUT
std::env::set_var("MC_LOG_STDERR", "1");
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ What is the negative affect this prevents? I commented it out and saw nothing negative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I was thinking to make the final log lines be a json blob on stdout, but I ended up not doing that.
I still think it's usually good for tools to send logging to stderr.

fog/view/connection/src/bin/client.rs
Comment on lines +92 to +96
//let mr_signer_verifier =
// mc_fog_view_enclave_measurement::get_mr_signer_verifier(None);

let mut verifier = Verifier::default();
verifier.debug(DEBUG_ENCLAVE); //.mr_signer(mr_signer_verifier);
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔧 Looks like there is some stray debug code here

Suggested change
//let mr_signer_verifier =
// mc_fog_view_enclave_measurement::get_mr_signer_verifier(None);
let mut verifier = Verifier::default();
verifier.debug(DEBUG_ENCLAVE); //.mr_signer(mr_signer_verifier);
let mut verifier = Verifier::default();
verifier.debug(DEBUG_ENCLAVE);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we can do this -- honestly it may be better to try to ignore attestation as much as possible here.

fog/view/connection/src/bin/client.rs

match grpc_client.request(0, 0, vec![]) {
Ok(resp) => {
log::info!(logger, "Got response:\n{:?}", resp);
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙃 nit

Suggested change
log::info!(logger, "Got response:\n{:?}", resp);
log::info!(logger, "Got response:\n{resp:?}");

fog/view/connection/src/bin/client.rs
.iter()
.map(|range| range.end_block - range.start_block + 1)
.sum();
log::info!(logger, "Total missed blocks: {}", total_missed_blocks);
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙃

Suggested change
log::info!(logger, "Total missed blocks: {}", total_missed_blocks);
log::info!(logger, "Total missed blocks: {total_missed_blocks}");

fog/view/connection/src/bin/client.rs
log::info!(logger, "Total missed blocks: {}", total_missed_blocks);
}
Err(err) => {
log::error!(logger, "Got error:\n{}", err);
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙃

Suggested change
log::error!(logger, "Got error:\n{}", err);
log::error!(logger, "Got error:\n{err}");

fog/view/connection/src/bin/client.rs
.build(),
);

log::info!(logger, "Fog view attestation verifier: {:?}", verifier);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log::info!(logger, "Fog view attestation verifier: {:?}", verifier);
log::info!(logger, "Fog view attestation verifier: {verifier:?}");

fog/view/connection/src/bin/client.rs
Comment on lines +34 to +35
match grpc_client.request(0, 0, vec![]) {
Ok(resp) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

💭 This is good enough and seems to be inline with other outputs, but I'm curious who the intended audience is.
The reason is that the resp, while plain text, is not very ergonomic for human scanning as it is one line that is 7000 characters long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can make it debug log instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nick-mobilecoin nick-mobilecoin nick-mobilecoin left review comments

@joekottke joekottke Awaiting requested review from joekottke

@holtzman holtzman Awaiting requested review from holtzman

@eranrund eranrund Awaiting requested review from eranrund

@dolanbernard dolanbernard Awaiting requested review from dolanbernard

At least 1 approving review is required to merge this pull request.

Assignees

@cbeck88 cbeck88

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cbeck88 @nick-mobilecoin