Clean up create_onion_keys_callback_generic
#3668
Conversation
There's no reason for anything outside of `onion_utils` to call an `_inner` method, so it shouldn't be pub.
Onion keys construction can only fail if we have a key which is the inverse of a hash containing a public key which we generated (even indirectly). Because we prefer to panic if our RNG creates an insecure key (as it indicates operating is incredibly unsafe), we thus don't actually need to return an error from `construct_onion_keys_generic_callback`, which we clean up here.
The onion keys building logic is rather ancient and predates the `-> impl Trait` syntax in Rust, and thus used a callback. Here we move it to an `impl Iterator` which is much Rustier and cleans up the code a bit.
Now that `create_onion_keys_generic` returns an iterator rather than relying on a callback, making `process_onion_failure_inner` chain the non-trampoline and trampoline iterators is trivial. We do so here, avoiding the extra vec allocation.
|
👋 Thanks for assigning @arik-so as a reviewer! |
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
| @@ -2163,8 +2149,7 @@ mod tests { | |||
| let route = Route { paths: vec![path], route_params: None }; | |||
|
|
|||
| let onion_keys = | |||
| super::construct_onion_keys(&secp_ctx, &route.paths[0], &get_test_session_key()) | |||
| .unwrap(); | |||
| super::construct_onion_keys(&secp_ctx, &route.paths[0], &get_test_session_key()); | |||
There was a problem hiding this comment.
This diff and below in 7f22871 belong in a prior commit
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
| ) | ||
| .expect("Route we used spontaneously grew invalid keys in the middle of it?"); | ||
| // if we have Trampoline hops, the blinded hops are part of the inner Trampoline onion | ||
| let nontrampoline_bp = |
There was a problem hiding this comment.
this is not a blinding_point, it's the blinded_tail. Also, you think it makes sense to unify the terminology between non_trampoline_ and/or outer?
| let unblinded_hops_iter = hops.iter().map(|h| (h.node_pubkey(), Some(h))); | ||
| let blinded_pks_iter = blinded_tail | ||
| let unblinded_hops = hops.iter().map(|h| (h.node_pubkey(), Some(h))); | ||
| let blinded_pks = blinded_tail | ||
| .map(|t| t.hops.iter()) |
There was a problem hiding this comment.
eh, variable names are cheap, we can call these pubkeys. It'll help readers coming from code bases where the counterpart is private_key and not secret_key.
| inner_session_priv.expect("Trampoline hops always have an inner session priv"); | ||
| Some(construct_onion_keys_generic(secp_ctx, hops, blinded_tail, inner_session_priv).map( | ||
| |(shared_secret, _, _, route_hop_option, _)| { | ||
| (route_hop_option.map(|th| ErrorHop::TrampolineHop(th)), shared_secret) | ||
| }, |
There was a problem hiding this comment.
if we're renaming this, let's also do the closure parameter rh, or leave the outer as trampoline_hop
It should return an iterator, not rely on callbacks (I think it predates
impl Iteratorlol) and doesn't need to return aResult.