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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nomos-cli] Add command to disseminate data #400

Merged
merged 4 commits into from
Sep 18, 2023
Merged

[nomos-cli] Add command to disseminate data #400

merged 4 commits into from
Sep 18, 2023

Conversation

zeegomo
Copy link
Contributor

@zeegomo zeegomo commented Sep 13, 2023

Add nomos-cli to hold various utilities for the node.
To start, this commit adds a command to disseminate some
data through the network and build a certificate of correct
dispersal for inclusion in a block.

The idea is to grow this command into an interactive one covering the whole DA and transaction submit process emulating an EZ

Still haven't tested it but not sure we have the corresponding server component in place

@zeegomo zeegomo self-assigned this Sep 13, 2023
@zeegomo zeegomo added this to the DA milestone Sep 13, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch has no changes to coverable lines.

Files Changed Coverage
nomos-cli/src/cmds/disseminate/mod.rs 0.00%
nomos-cli/src/cmds/mod.rs 0.00%
nomos-cli/src/main.rs 0.00%
nomos-da/full-replication/src/lib.rs 0.00%
nomos-services/data-availability/src/lib.rs ø

📢 Thoughts on this report? Let us know!.

Comment on lines 107 to 114
loop {
if let Some(cert) = da.certify_dispersal() {
terminal_cmd_done();
return Ok(cert);
}
da.recv_attestation(attestations.next().await.unwrap());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably clearer to call recv_attestation prior to certify_dispersal? It's not very important, though.

Comment on lines +45 to +46
tracing::subscriber::set_global_default(tracing_subscriber::FmtSubscriber::new())
.expect("setting tracing default failed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be on the main function of the binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure everybody is interested in logs. For example, the complete interactive version of this command probably wants to use a different UI without printing node logs. Also, I'm using logs here to catch whatever the node is printing, a command that does not have a running node maybe does not even use tracing logs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, makes sense, thanks!

Comment on lines 99 to 101
for blob in blobs {
adapter.send_blob(blob).await.unwrap();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can send then all at once (they may be a lot of chunks eventually), something like:

let tasks: FuturesUnordered<_> = blobs.map(|blob| tokio::spawnadapter.send_blob(blob))).collect()
future::try_join_all(tasks).await;

Comment on lines +116 to +118
// To interact with the network service it's easier to just spawn
// an overwatch app
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if it would have been enough to spawn a NetworkBackend instead.

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 think the Libp2pAdapter likes a NetworkService

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes indeed, likes it very much!

Add nomos-cli to hold various utilities for the node.
To start, this commit adds a command to disseminate some
data through the network and build a certificate of correct
dispersal for inclusion in a block
@zeegomo zeegomo merged commit d672be3 into master Sep 18, 2023
12 checks passed
@jakubgs jakubgs deleted the nomos-cli branch February 15, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execution zones simulator: Rs+Kzg computed locally and network dissemination of blobs
3 participants