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

feat: retrieval server & client setup #654

Closed
wants to merge 2 commits into from
Closed

Conversation

cernicc
Copy link
Member

@cernicc cernicc commented Dec 20, 2024

Description

Setup the retrieval server and client. They encapsulate the logic needed to successfully transfer files between them. The current blockstore used is in memory and doesn't have a concept of car format. The idea is to swap the server's and client's blockstore to the implementation specific for them.

The example initializes server and client and transfers a single block between them.

Checklist

  • Make sure that you described what this change does.
  • Have you tested this solution?

@cernicc cernicc added the enhancement New feature or request label Dec 20, 2024
@cernicc cernicc added this to the Phase 3 milestone Dec 20, 2024
@cernicc cernicc self-assigned this Dec 20, 2024
@th7nder th7nder changed the title feat: retreival server & client setup feat: retrieval server & client setup Dec 20, 2024
Copy link
Contributor

@th7nder th7nder left a comment

Choose a reason for hiding this comment

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

I lack completeness in this PR, there is a lot of todos, the functionality is unclear, lots of scenarios are unhandled.
This feels like a prototype for trying out how to transfer a 1 packet via beetswap and libp2p, not something that we can merge into 'production code.'

I'd need to know the strategy on how are we going to implement this whole thing before I agree to merge it.

E.g. the server.rs looks like it'll be gone anyways, or not present in the production code -> because that's something that should be implemented on the Storage Provider Server.

client.rs looks like a candidate to be a separate CLI, cause I suppose we're going to run it like:
polka-fetch <CID> <STORAGE_PROVIDER_NODE_ADDR>

I assume if we'd like to start writing production code and merging it, we should:

  1. setup CLI polka-fetch package and define it's funcionalities.
  2. add libP2P server to storage provider node, so we can dial it
  3. dial from polka-fetch to storage provider node
  4. Add beetswap protocol support to Server and client, prove that they can connect and download a single file (hardcoded data always returned)
  5. Add ability to storage provider server to return a CAR archive identified by CID (I guess use local indexer).
  6. Call polka-fetch with those multiple CIDs and verify those archives are downloaded properly.

(I just thought on the fly about it, nothing binding)
So very similar to the list of tasks that you wrote in #634.

tracing = { workspace = true }

[dev-dependencies]
# multihash = "0.19.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# multihash = "0.19.3"

@@ -136,6 +142,7 @@ pallet-market = { path = "pallets/market", default-features = false }
pallet-proofs = { path = "pallets/proofs", default-features = false }
pallet-randomness = { path = "pallets/randomness", default-features = false }
pallet-storage-provider = { path = "pallets/storage-provider", default-features = false }
polka-index = { path = "storage/polka-index" }
Copy link
Contributor

Choose a reason for hiding this comment

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

What is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

empty?

use std::{str::FromStr, sync::Arc, time::Duration};

use anyhow::Result;
use blockstore::{
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, didn't know eiger had this library


// Setup server
let server = Server::new(blockstore)?;
let listener: Multiaddr = format!("/ip4/127.0.0.1/tcp/8989").parse()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let listener: Multiaddr = format!("/ip4/127.0.0.1/tcp/8989").parse()?;
let server_address: Multiaddr = format!("/ip4/127.0.0.1/tcp/8989").parse()?;

.with_behaviour(|_| Behaviour {
bitswap: beetswap::Behaviour::new(blockstore),
})
.expect("infallible")
Copy link
Contributor

Choose a reason for hiding this comment

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

because?

bitswap: beetswap::Behaviour::new(blockstore),
})
.expect("infallible")
.with_swarm_config(|c| c.with_idle_connection_timeout(Duration::from_secs(60)))
Copy link
Contributor

Choose a reason for hiding this comment

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

magic number

self.queries.insert(query_id, payload_cid);

// Pin cancellation future
pin_mut!(cancellation);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? What about CancellationTokens?

@cernicc cernicc marked this pull request as draft December 20, 2024 13:49
@cernicc cernicc closed this Dec 20, 2024
@cernicc cernicc deleted the feat/retrieval_libp2p branch December 20, 2024 15:32
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.

2 participants