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

Add DA api for explorer and remove unused API #576

Closed
wants to merge 3 commits into from

Conversation

al8n
Copy link
Contributor

@al8n al8n commented Feb 5, 2024

No description provided.

@al8n al8n added the enhancement New feature or request label Feb 5, 2024
@al8n al8n self-assigned this Feb 5, 2024
@al8n al8n requested a review from bacv February 5, 2024 05:02
use nomos_storage::backends::sled::SledBackendSettings;
use nomos_storage::{backends::sled::SledBackend, StorageService};

#[derive(Services)]
struct Explorer {
log: ServiceHandle<Logger>,
storage: ServiceHandle<StorageService<SledBackend<Wire>>>,
da: ServiceHandle<DataAvailability>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there a da service here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in da_blobs fn, we need to connect to the DataAvailability service.

...

let relay = handle.relay::<DataAvailability>().connect().await?;

...

Copy link
Collaborator

Choose a reason for hiding this comment

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

This do not work as you think. The node explorer should not have this service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks, you are right. In this case, we should fetch the DA blobs from persistent storage. Currently, all of the DA blobs seem to be in the memory pool.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to think about that indeed. Blobs can be retrieve from the node directly for now. But at some point we should derivate into something like storage.

Copy link
Member

Choose a reason for hiding this comment

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

The blobs at the moment are stored in BlobCache which implements DaBackend. It has get/add/remove functions. It seems that this was created with an idea to be replaced with other storage. Maybe we could add a sled DaBackend there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could add a sled DaBackend there?

Yeah, I think so. This is the only way to offload blobs query from the consensus layer to the explorer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to check. But in this case we could have a backend that keeps the memmory cache and the sled one. But definitely not do over this pr.

@danielSanchezQ
Copy link
Collaborator

So, right now this is a kind of proxy for the node api call right? I think we should probably wait till we have the storage draft and then consider from there.

@al8n
Copy link
Contributor Author

al8n commented Feb 6, 2024

So, right now this is a kind of proxy for the node api call right? I think we should probably wait till we have the storage draft and then consider from there.

Right now, we can fetch blocks directly from explorer, but cannot query da blobs by explorer.

@danielSanchezQ
Copy link
Collaborator

So, right now this is a kind of proxy for the node api call right? I think we should probably wait till we have the storage draft and then consider from there.

Right now, we can fetch blocks directly from explorer, but cannot query da blobs by explorer.

yes, so there is no point on having this until we use the storage itself.

@al8n
Copy link
Contributor Author

al8n commented Feb 6, 2024

I think we should probably wait till we have the storage draft and then consider from there.

Ok, I will still use the consensus layer to fetch the DA blobs for the demo. What do you think @bacv?

@bacv
Copy link
Member

bacv commented Feb 6, 2024

Ok, I will still use the consensus layer to fetch the DA blobs for the demo. What do you think @bacv?

Yes, i think for this pr lets leave it as is.

Copy link
Collaborator

@danielSanchezQ danielSanchezQ left a comment

Choose a reason for hiding this comment

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

@al8n This is previous to the rockdb backend. Do not know if we should merge this or the other to see if everything works as expected.

@al8n al8n closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants