[WIP] Rework ResponseSendFundsOp

[pzmarzly]

As discussed in #208

EDIT: Closes #208 and closes #220

[pzmarzly]

I started working on implementing your feedback. When creating is_valid tests, I created a helper macro, only to later notice that something similar was also done before in components/stctrl/src/multi_route_util.rs.

I replaced that code to also use the new macro. It looks okay, but I'm not sure if introducing new global macro is good for readability (& complexity). I can revert 5fb773f and 2d65648 if you want (the latter was created because components/proto/src/funder/serialize.rs was affected by crypto_rand -> rand renaming).

[realcr]

It looks okay, but I'm not sure if introducing new global macro is good for readability (& complexity)

First I want to say that I appreciate the initiative (: . My opinion is against exporting the mock_friends_route macro from offst-app, because offst-app is the crate facing application developers. I think it is reasonable to leave the pk(i) mocking system we have at multi_route_util.rs the way it is.

[pzmarzly]

My opinion is against exporting the mock_friends_route macro from offst-app, because offst-app is the crate facing application developers.

How about marking mock_friends_route! as #[cfg(test)]?

[realcr]

How about marking mock_friends_route! as #[cfg(test)]?

If we have something like mock_friends_route! I agree that it should be marked as #[cfg(test)]. I am still against exporting it from offst-app though. I have to admit I also don't feel comfortable with exporting anything beginning with mock_* from offst-proto.

I had another idea, tell me what you think:

The functions is_valid_part and is_valid_internal don't really deal with PublicKey-s. You can make those two functions generic, working on a type T that implements Hash and Eq. Then you will not have to mock anything. You will be able to do all the tests with slices like &[1u32, 2, 3, 4].

EDIT: Formatting.

EDIT: About the design of the routes checks: I just read the current version from the last commit on this PR. Although it seems to do the correct thing, it was difficult for me to read. This could be a matter of taste of course, but I fear someone less smart than you will have to think a lot to understand the code.

Some examples for elements I believe are hard to read:

• is_min_unique(&seen)
• self.is_valid_internal(2, true)

Most likely if I ever look again on this code a year from now, it will be difficult to recall what the true in is_valid_internal means, and also, what is the meaning of 2?

I have some idea about how to make it simpler (At least to my taste). I give here some outline:

/// Check if no element repeats twice in the array
fn is_unique(array: &[T]) -> bool {
    // ....
}

pub fn is_valid_as_part(&self) -> bool {
    // Make sure that the array is not too long: Not over MAX_ROUTE_LENGTH - 1
    // check is_unique(...)
}

pub fn is_valid(&self) -> bool {
    // If first element equals the last element, consider only `array[1:...]`. Otherwise consider the full array
    // check is_valid_as_part(...)
}

EDIT: Did you figure out the issue with the destination node getting route that is of length > 0?

[pzmarzly]

I like your idea about is_unique. Though I'm not sure if making these functions generic is worth the effort. I think the code could stay as it was.

I don't think of myself as smart, especially when it comes to graphs, lists (routes), algorithms over them etc., so I appreciate your feedback a lot.

I'm having some problems with reverting commits, so I'm going to force-push this PR. I'm attaching 5fb773f in case it got lost somehow.

5fb773fda54d8a8675d9469ecc2e651f11252e30.patch

[pzmarzly]

I was thinking about documentation of pk_to_index, as it can be used for searching both in full routes and route fragments.

Currently:

/// Find the index of a public key inside the route.
/// source is considered to be index 0.
/// dest is considered to be the last index.

At first, I thought about replacing it with:

/// Find the index of a public key inside the route or route fragment.
/// In full route, source is considered to be index 0.
/// dest is always considered to be the last index.

Except that in case of a cycle, source can be the last index. Now I'm not sure what to do with these docs. I also suspect that may be a reason why a test may fail (if not now, then maybe in the future).

[pzmarzly]

Speaking of .is_empty() issue, I haven't found yet why route is never emptied. It's not easy for me when dealing with 50-element call stack. I added a dbg! to inspect a test here (branch pzmarzly:208-broken).

Running cargo test test_handler_pair_basic outputs:

running 1 test
test handler::tests::pair_basic::test_handler_pair_basic ... FAILED

failures:

---- handler::tests::pair_basic::test_handler_pair_basic stdout ----
[components/funder/src/handler/handle_friend.rs:147] &request_send_funds.route = FriendsRoute {
    public_keys: [
        PublicKey(
            [
                235, 245, 233, 123, 66, 22, 177, 135,
                216, 247, 124, 130, 115, 218, 247, 164,
                114, 156, 35, 129, 172, 121, 174, 47,
                55, 105, 217, 124, 94, 240, 157, 64,
            ],
        ),
    ],
}
thread 'handler::tests::pair_basic::test_handler_pair_basic' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `20`', components/funder/src/handler/tests/pair_basic.rs:690:17
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

If you want to look into it, maybe you know what key it is?

[realcr]

@pzmarzly : I think I understand the problem. When the transaction is first created (The first Request message), we actually have to remove two nodes: One node is ourselves, and then we also need to remove the next node.

For example, if the route is A -- B -- C, and we are A, we should be sending B the route C, and C should get an empty route.

This means that we should have this code:

    // Remove ourselves from the remaining route.
    route_tail.public_keys.remove(0);

    // Remove next node from the route:
    route_tail.public_keys.remove(0);

At the control_create_transaction_inner() function in handle_control.rs. Please look at the code above in this function, to make sure no panics are possible there. I didn't do it myself.

Note that after you will change this code you should stumble upon the next bug:

thread 'handler::tests::pair_basic::test_handler_pair_basic' panicked at 'internal error: entered unreachable code', components/funder/src/handler/sender.rs:169:23

To fix this bug, you should fix is_valid_part() so that:

assert_eq!(route![].is_valid_part(), true);

Though I'm not sure if making these functions generic is worth the effort. I think the code could stay as it was.

I agree, we can leave the code the way it is. But just for the sports, you should know that changing the functions you wrote to be generic is trivial. You should add a clause like where T: Eq + Hash, and use T in the signature instead of PublicKey. I think that besides those changes you don't need to do anything. You can then erase the helper macro.

Except that in case of a cycle, source can be the last index. Now I'm not sure what to do with these docs. I also suspect that may be a reason why a test may fail (if not now, then maybe in the future).

If the doc is confusing, we can begin by erasing it (: pk_to_index is just basic searching in an array, nothing more. We can even erase this function and just use plain vector searching if it turns out to be ergonomic enough. Is the pk_to_index function still being used at this point?

[pzmarzly]

pk_to_index is only used in verify_freezing_links, sub_frozen_credit and add_frozen_credit. I'm not sure if it should be - do these function receive a full route?

Speaking of generic is_unique(), I knew it would be easy. I had more concerns about is_valid, as it would stop taking &self. I was thinking about how to make it ergonomic. Making 2 sets of functions (is_valid_generic(T) and is_valid(&self)) and exporting these would be barely better than a macro. Exporting just generic version would require calling it via FriendsRoute::is_valid(friends_route.public_keys), which looks acceptable but I hoped it could be done better. Making a trait would also have some drawbacks. Maybe making impl From<FriendsRoute> for &[PublicKey] would be possible, so one could call FriendsRoute::is_valid(friends_route). But all of these look like hacks.

Thanks for looking into the non-empty route issue, I will fix it now.

[realcr]

pk_to_index is only used in verify_freezing_links, sub_frozen_credit and add_frozen_credit. I'm not sure if it should be - do these function receive a full route?

Those functions are legacy. We used to have a freeze guard, but we don't have it anymore. Try to comment out pk_to_index and see if the code compiles (:

FriendsRoute::is_valid(friends_route.public_keys), which looks acceptable

I even recommend to have is_valid as a separate function, outside of the FriendsRoute class. I'm cool with this kind of solution. It's easy to test, and we can always modify it later if we want to.

But I have enough for today.

You helped a lot. Making this feature work should give enhanced privacy, so it's pretty useful. This is not an urgent issue though, so you can keep working on it when you have the time. If you are stuck on something don't hesitate to send a message.

[pzmarzly]

On my PC tests pass, CI should be ready soon. TIL Github orders commits in PRs by their creation date, so even though the order is "Get cargo..." -> "Delete..." -> "Reword...", Github displays them here as they were before reordering.

Now all that seems to be left is refactoring is_valid and resolving that forward_request conversation.

[realcr]

@pzmarzly : Thanks, great work on this PR!