Grin: Inconsistent error handling between api client and pool push handler

[heunglee]

Api client uses hyper client to post transactions to transaction pool and processes hyper::response for returning error. However the pool is on iron and encapsulates errors in IronError struct. For that matter, the wallet cannot get the right error messages sent from transaction pool.

The following two issues indicates the inconsistency of error handling:
https://github.com/mimblewimble/grin/issues/640
https://github.com/mimblewimble/grin/issues/616

Source codes]

Api client side:
pub fn post<'a, IN>(url: &'a str, input: &IN) -> Result<(), Error>
where
IN: Serialize,
{
let in_json = serde_json::to_string(input).map_err(|e| {
Error::Internal(format!("Could not serialize data to JSON: {}", e))
})?;
let client = hyper::Client::new();
let _res = check_error(client.post(url).body(&mut in_json.as_bytes()).send())?;
Ok(())
}

// convert hyper error and check for non success response codes
fn check_error(res: hyper::Result) -> Result {
if let Err(e) = res {
return Err(Error::Internal(format!("Error during request: {}", e)));
}
let mut response = res.unwrap();
match response.status.class() {
StatusClass::Success => Ok(response),
StatusClass::ServerError => {
Err(Error::Internal(format!("Server error: {}", err_msg(&mut response))))
}
StatusClass::ClientError => if response.status == StatusCode::NotFound {
Err(Error::NotFound)
} else {
Err(Error::Argument(format!("Argument error: {}", err_msg(&mut response))))
},
_ => Err(Error::Internal(format!("Unrecognized error."))),
}
}

handler of transaction pool side:
let res = self.tx_pool
.write()
.unwrap()
.add_to_memory_pool(source, tx);

    match res {
        Ok(()) => Ok(Response::with(status::Ok)),
        Err(e) => {
            debug!(LOGGER, "error - {:?}", e);
            Err(IronError::from(Error::Argument(format!("{:?}", e))))
        }
    }

Reproduction of inconsistency:
wallet side -
Heungs-iMac:wallet aewal$ grin wallet -p password receive -i send.txt
Jan 20 15:51:05.823 DEBG Using wallet seed file at: ./wallet.seed
Jan 20 15:51:05.900 INFO Acquiring wallet lock ...
Jan 20 15:51:05.900 DEBG Attempting to acquire wallet lock
Jan 20 15:51:06.672 INFO ... released wallet lock
Jan 20 15:51:07.882 DEBG Received txn and built output - Identifier(67a48886224345b4f669), Identifier(9da687568b1de317dc02), 1591
response --- Response { status: BadRequest, headers: Headers { Content-Length: 0
, Date: Sat, 20 Jan 2018 23:51:08 GMT
, }, version: Http11, url: "http://127.0.0.1:13413/v1/pool/push", status_raw: RawStatus(400, "Bad Request"), message: Http11Message { is_proxied: false, method: None, stream: Wrapper { obj: Some(Reading(SizedReader(remaining=0))) } } }
Jan 20 15:51:08.149 INFO Acquiring wallet lock ...
Jan 20 15:51:08.149 DEBG Attempting to acquire wallet lock
Jan 20 15:51:08.912 INFO ... released wallet lock
Error receiving transaction, the most likely reasons are:

• the transaction has already been sent
• your node isn't running or can't be reached

Detailed error: Node(Argument("Argument error: "))

transaction pool side:
Err(DuplicateOutput { other_tx: None, in_chain: true, output: Commitment(08c65bf5148bc33f515d23e3c59d9dc3741b9911dfdf011e935d9601b7c3b64ee4) })

[ignopeverell]

I think we should normalize everything on hyper.

[heunglee]

Thank you for letting me know your direction. Normalizing to hyper is " 一石二鳥 - getting two birds with one stone" (Korean old saying). Bumping hyper 0.11 will achieve solving this issue and adding graceful shutdown. I will get my hands on this issue. I also started rust implementation of radix tree algorithm for routing of url paths for hyper. It will provide benefits of grouping common paths and fast retrieval of handlers.

[heunglee]

@ignopeverell I have some questions. 1) Currently we do not support secured http between the wallet and the server. If the wallet is on the remote server, the secured http will increase the security on the communication between the server and the wallet. Do you think the secured http is mandatory or configurable? 2) Is it necessary to support IPv6 address? Currently the communication between the wallet and the server is done on local host. So it is not a matter. But if the communication takes place remotely, it may need routable IP address.

[ignopeverell]

All the server APIs should absolutely be over HTTPS when remote. Unfortunately there are a few usability issues around this (self-signed cert?) that add a lot of friction. Using localhost simplifies the picture quite a bit. So I would still take localhost as a given for now until we figure all of this out.

[kargakis]

Let's Encrypt does not support issuing certificates for public IPs, otherwise a grin installer could request LE certificates for anyone who wants to operate a public node w/o having to resort to dns fu.

[heunglee]

Impacted libraries on dropping iron:

• api - api/Cargo.toml
• wallet - wallet/Cargo.toml

Files depending on iron:

• api/src/rest.rs
• wallet/src/handlers.rs
• api/src/handlers.rs
• api/src/lib.rs
• wallet/src/receiver.rs

hyper version to bump to:
v0.11.19 (2018-02-21) or later.
The final version will be TBD.

[heunglee]

@ignopeverell
Hyper 0.11 + changed the way of processing requests significantly because of moving on async world. The new way requires using Service trait to precess the incoming requests. Service trait has call method that contains all endpoints and corresponding handling operations. That means no router of endpoints is provided. That is why I started developing radix tree based router to plug into hyper. However, I found that the design of hyper 0.11+ makes using custom router very difficult. (It could result from my limited rust knowledge. But I could not have luck to find how to use custom router.)

It is time to look around for alternatives.
option 1 - just implementing all endpoints in call method of Service trait.
reference example) https://github.com/hyperium/hyper/blob/master/examples/server.rs
option 2 - using another web framework.
They claim using async mode like hyper and have built-in router feature.
reference) https://github.com/gotham-rs/gotham

RE: graceful shutdown
hyper provides graceful shutdown feature based on future. It is limited and hard to understand how to use (no example either). Based on my preliminary research, gotham-rs does not have explicit shutdown functionality.

RE: https
hyper has a way to provide https with hyper-tis library.
gotham seems not support https yet.

@ignopeverell What is your thought? I am okay with both options.

[ignopeverell]

Looks like Gotham is built on hyper, so we'd just be adding more layers for our relatively simple use case. So I'd go with option 1, which basically looks like a programatic router. But there are no simple routers available with hyper 0.11+?

[heunglee]

Short answer is no.

The design of hyper 0.11+ makes plugging any router for use in any (tuple) struct implementing Service trait very difficult. AFAIK, some struct impl Service trait works well without fields. But to use any router in call method, a simple way is to set a router in the struct. Setting a router in the struct causes lifetime issue or a compiler error like:
error[E0525]: expected a closure that implements the Fn trait, but this closure only implements FnOnce
--> api/src/server.rs:116:45
|
116 | let server = Http::new().bind(addr, || Ok(HyperServer::new(router))).unwrap();
| ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| the requirement to implement Fn derives from here
|
note: closure is FnOnce because it moves the variable router out of its environment
--> api/src/server.rs:116:68
|
116 | let server = Http::new().bind(addr, || Ok(HyperServer::new(router))).unwrap();
|

Also whenever it receives an incoming request, hyper makes new copy of Service to serve. That means any router set in the struct needs be copied as well. (But not 100% sure.) I don't think it is very efficient way to do.

So far I have not seen any router for hyper 0.11+ and experience hard time to use my router with hyper.
Until we find a router for hyper 0.11+ or a way to use my router, we can hard code the endpoints in call method.

[heunglee]

I just found an experimental router for hyper 0.11.
https://github.com/ubnt-intrepid/hyper-router

I will look into it and see I can borrow their idea or use theirs.

[heunglee]

Hyper http::bind needs NewService trait to bind socket address to TcpListner. Whenever new http request comes in, hyper gets service clone from NewService and calls "call" method of cloned Service.

impl NewService for Router
where
R: RouteRecognizer,
{
type Request = Request;
type Response = Response;
type Error = HyperError;
type Instance = RouterService;

fn new_service(&self) -> io::Result<Self::Instance> {
    Ok(RouterService { inner: **self.inner.clone**() })
}

}

Radix tree based router of my own has many node structures of a tree and it is problematic to clone the entire nodes for each request. On the other hand, ubnt-intrepid's experimental hyper router just uses hashmap of http method and Vec of a structure with regex'ed path and corresponding handler as router data storage. So it is fine to clone for small number of endpoints.

So I need drop radix tree based router and use ubnt-intrepid's router. Since it is experimental, I am not sure how much it is stable. If it doesn't fit, then I will go back to the option 1 above.

[heunglee]

Summary of migration status:
https://github.com/ubnt-intrepid/hyper-router
The experimental hyper router uses depreciated future type, BoxFuture and they don't maintain the repo. So I did not want to use the router. Instead I found they have active web framework development on the top of hyper 0.11+, https://github.com/ubnt-intrepid/susanoo by chance.
The repo has also radix tree based router implementation (which encouraged me to try my radix tree based router) as well as regex based one. It is good reference for me to make radix tree based router work. I learn and borrow the usage of some data structures and designs to work with hyper. So far I still have stiff learning curve on trait with type parameters, their lifetimes and Fn that does not allow mutating state. To satisfy the traits and functions provided by hyper in grin api, I am still working on the pipeline from thread spawning for ApiServer and calling static functions in closure of Fn() to match lifetime and traits of hyper. Hopefully, I can pass compilation and get to the runtime tests in this weekend.

[heunglee]

For preparation of testnet2, this issue is closed and will be addressed in https://github.com/mimblewimble/grin/issues/876.