IPFS MVP #4545

maciejhirsz · 2017-02-14T19:04:33Z

Listening on port 5001 for IPFS-compatible calls as listed in #4172, supported thus far:

eth-block
eth-block-list
eth-tx
eth-state-trie

NikVolf · 2017-02-14T21:06:37Z

no tests ;)

NikVolf · 2017-02-14T21:07:23Z

ipfs/src/handler.rs

+
+   #[test]
+	fn test_get_param() {
+		let query = "foo=100&bar=200&qux=300";


can't it be encoded like
foo%20bar=200?

indentation on #[test] seen here is also wrong

@NikVolf I've switched the function to be private, so that it can only be consumed within the module.

I think that as long as the consumer is aware that no URL encoding or decoding is performed it should be quite fine. It's unlikely that this crate will ever have a need for URL-encoded query strings by the virtue of all things IPFS going through multibase, specifically base58btc. Even if it ever does, it shouldn't be difficult to replace this by an external crate that can do it.

NikVolf · 2017-02-14T21:08:58Z

ipfs/src/lib.rs

+
+				res.headers_mut().set(ContentLength(reason.len() as u64));
+				res.headers_mut().set(ContentType("text/plain".parse()
+					.expect("Static content type; qed")));


i don't think that any request handling that can be triggered by arbitrary third party should ever contain expect or unwrap

But yeah... that's surely cant fail

Still there should be test that triggers this execution path (implementation of the ContentType::from_str can change and the test should catch it)

I can change that to use a macro for mime types.

NikVolf · 2017-02-14T21:28:38Z

parity/run.rs

@@ -420,6 +421,10 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc<RotatingLogger>) -> R
 	};
 	let signer_server = signer::start(cmd.signer_conf.clone(), signer_deps)?;

+


redundant empty line

NikVolf · 2017-02-14T21:30:57Z

ipfs/src/lib.rs

+
+			listening
+		})
+		.map_err(Into::into)


with the introduction of Carrier maybe construction a-la

Ok( hyper::Server::http(&addr)? .handle(move |_| IpfsHandler::new(client.clone())) .map(|(listening, srv)| { ::std::thread::spawn(move || { srv.run(); }); listening })? )

is better?

tomusdrw

Couple of grumbles.

tomusdrw · 2017-02-15T09:06:34Z

ipfs/src/lib.rs

+		match *self.out() {
+			OctetStream(ref bytes) => {
+				// Nothing to do here
+				let _ = transport.write(&bytes);


Write might not succeed in a single attempt. It will return how many bytes were written, you should keep a pointer and write the rest in next invocation of the handler.

tomusdrw · 2017-02-15T09:06:42Z

ipfs/src/lib.rs

+			},
+			NotFound(reason) | Bad(reason) => {
+				// Nothing to do here
+				let _ = transport.write(reason.as_bytes());


Same as above.

tomusdrw · 2017-02-15T09:08:24Z

ipfs/src/handler.rs

+		let block_id = BlockId::Hash(hash);
+		let block = self.client.block_header(block_id).ok_or(Error::BlockNotFound)?;
+
+		self.out = Out::OctetStream(block.into_inner());


Why not return Out instead? Methods wouldn't have to be &mut and assignment would happen in a single place.

tomusdrw · 2017-02-15T09:09:05Z

ipfs/src/handler.rs

+		}
+	}
+
+	fn get_block(&mut self, hash: H256) -> Result<()> {


get_ prefix is not necessary in all methods.

tomusdrw · 2017-02-15T09:13:50Z

ipfs/src/lib.rs

+
+impl Handler<HttpStream> for IpfsHandler {
+	fn on_request(&mut self, req: Request<HttpStream>) -> Next {
+		if *req.method() != Method::Get {


Do we know from what context this server will be used?

Currently any website can access this API and trigger arbitrary load on the node, might be worth to limit access somehow, or disallow Origin header if it's not mean to be read by websites.

tomusdrw · 2017-02-15T09:15:49Z

ipfs/src/handler.rs

+		let tx_id = TransactionId::Hash(hash);
+		let tx = self.client.transaction(tx_id).ok_or(Error::TransactionNotFound)?;
+
+		self.out = Out::OctetStream(rlp::encode(tx.deref()).to_vec());


tx.deref() -> &*tx

tomusdrw · 2017-02-15T09:18:19Z

ipfs/src/handler.rs

+/// Get a query parameter's value by name.
+pub fn get_param<'a>(query: &'a str, name: &str) -> Option<&'a str> {
+	query.split('&')
+		.find(|part| part.starts_with(name) && part[name.len()..].starts_with("="))


Will panic when part == name, no?

@tomusdrw no, it will create an empty slice and starts_with can't panic. I added a test for it now just in case.

tomusdrw · 2017-02-15T09:19:55Z

ipfs/src/handler.rs

+			Err(err) => {
+				self.out = err.into();
+
+				Next::write()


Seems like Next::write() is the only result of this function, maybe it's possible to use and_then instead of conditionals?

arkpar · 2017-02-15T14:09:50Z

There should also be a CLI option disabling the server. I also suggest it should be disabled by default as most people don't really need this and it only increases attack surface.

rphmeier · 2017-02-15T14:11:25Z

Agreed, the overhead of starting up the server here shouldn't be incurred unless in-use (and AFAIK Metamask will be the one user of this feature)

NikVolf · 2017-02-15T17:11:57Z

ipfs/Cargo.toml

+ethcore-util = { path = "../util" }
+rlp = { path = "../util/rlp" }
+hyper = { default-features = false, git = "https://github.com/ethcore/hyper" }
+cid = "~0.2.0"


i believe we use like cid = "0.2" everywhere which is almost equivalent to cid = "~0.2.0" ?

maciejhirsz · 2017-02-15T18:34:17Z

Think I got most of the grumbles so far, TODO:

CLI switch, disabled by default.
Origin header checking.

This is done by requests sending CID with raw binary codec (0x55). Note: this functionality is exactly the same as fetching state-trie due to how db internals work in Parity atm.

NikVolf · 2017-02-16T20:18:48Z

looks cool to me
other guys have to look also

maciejhirsz added 3 commits February 14, 2017 19:30

Squashed

3c63470

Lock file

9256aa7

Added missing GPL header

2ee2d2e

maciejhirsz added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 14, 2017

NikVolf added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 14, 2017

NikVolf reviewed Feb 14, 2017

View reviewed changes

tomusdrw reviewed Feb 15, 2017

View reviewed changes

Tests and grumbles

eb32733

NikVolf reviewed Feb 15, 2017

View reviewed changes

maciejhirsz added 2 commits February 15, 2017 18:26

No .expect on mime types

d005410

Write output as chunks

9cfa278

maciejhirsz added 4 commits February 15, 2017 20:29

Adding CLI flags for IPFS

451cf42

Added CLI flags

ad8e3f0

Rename parity-ipfs to parity-ipfs-api

c4b4a22

Only allow requests from Origin 127.0.0.1

8d6275b

maciejhirsz added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 16, 2017

Added support for contract code by hash.

da696e4

This is done by requests sending CID with raw binary codec (0x55). Note: this functionality is exactly the same as fetching state-trie due to how db internals work in Parity atm.

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 17, 2017

rphmeier merged commit 39237e9 into master Feb 17, 2017

rphmeier deleted the mh-ipfs branch February 17, 2017 16:20

5chdn mentioned this pull request Jul 12, 2017

ipfs-api - does not respond to eth-storage-trie CID #6043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IPFS MVP #4545

IPFS MVP #4545

maciejhirsz commented Feb 14, 2017

NikVolf commented Feb 14, 2017

NikVolf Feb 14, 2017

rphmeier Feb 15, 2017

maciejhirsz Feb 15, 2017

NikVolf Feb 14, 2017

NikVolf Feb 14, 2017 •

edited

Loading

maciejhirsz Feb 14, 2017

NikVolf Feb 14, 2017

NikVolf Feb 14, 2017 •

edited

Loading

tomusdrw left a comment

tomusdrw Feb 15, 2017

tomusdrw Feb 15, 2017

tomusdrw Feb 15, 2017

tomusdrw Feb 15, 2017

tomusdrw Feb 15, 2017

tomusdrw Feb 15, 2017

tomusdrw Feb 15, 2017

maciejhirsz Feb 15, 2017

tomusdrw Feb 15, 2017

arkpar commented Feb 15, 2017

rphmeier commented Feb 15, 2017

NikVolf Feb 15, 2017

maciejhirsz commented Feb 15, 2017 •

edited

Loading

NikVolf commented Feb 16, 2017

		@@ -420,6 +421,10 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc<RotatingLogger>) -> R
		};
		let signer_server = signer::start(cmd.signer_conf.clone(), signer_deps)?;

IPFS MVP #4545

IPFS MVP #4545

Conversation

maciejhirsz commented Feb 14, 2017

NikVolf commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikVolf Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikVolf Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Feb 15, 2017

rphmeier commented Feb 15, 2017

Choose a reason for hiding this comment

maciejhirsz commented Feb 15, 2017 • edited Loading

NikVolf commented Feb 16, 2017

NikVolf Feb 14, 2017 •

edited

Loading

NikVolf Feb 14, 2017 •

edited

Loading

maciejhirsz commented Feb 15, 2017 •

edited

Loading