It seems the return value is used nowhere and can just as well be removed.

Done

This is not the only case where we can get back a channel update. For example, FailIncorrectCltvExpiry. I think we should try to update additionalEdges also in those cases.

Can the update maybe be done inside applyChannelUpdate ?

Done.

There are currently two ways of writing these kind of path finding tests. One is the way you did it, using the json file. The other option is used in for example TestChannelUpdateValidation. It allows programmatically building the test graph and generates some values like the keys by itself (as the actual value isn't important for the tests, it just needs a value). You could consider using that instead. I think it is clearer than keep adding new channels to the json file.

I think that you will still have the problem of needing a private key even if you use TestChannelUpdateValidation approach, as the validation of the error message is part of the tested code. Anyhow, I like the approach of using the basic graph in many tests all based on a non trivial topology. It requires the writer of the test to rethink whether the logic indeed covers all scenarios, beyond the simplest topology needed for testing the particular feature or fix she was aiming to do.

That's okay, just wanted to mention the option.

This line and others too exceed the max line width of 80 chars (project coding standard).

The log line is of 77 width. I believe I kept the <80 width everywhere.

I have it at 96 chars. Do you have tab size 8?

tab size 4. Using vscode.

The standard is 80 chars max @ tab size 8.

Sorry about that. Fixed.

Why is this added here? Validation is also done inside r.applyChannelUpdate.

I agree this is ugly. My excuse it that I'm trying to minimize the changes I do in code to those mandatory for this PR. Therefore, if validation fails, processChannelUpdateAndRetry should immediately return, and should not update paySession at all (neither update hints or report edge policy failure), so the check must be there. However we can not rely on the error returned by applyChannelUpdate, as it may well error in other scenarios (i.e. not validation errors) which are perfectly ok, and do require retries. Initially I thought of doing the validation much earlier, i.e. before the switch on the error types, but this looks like a change that is too major for this PR; hence my feeling was that adding validation here is the right way to go, with only bad consequence being that its checked twice.

I think it is ok to not update the payment session if the graph update fails. ReportEdgeFailure is important to call in case of a validation error. If we get an incorrect update we don't want to try that channel anymore.

Ok. I removed the extra validation and returned if applyChannelUpdate fails after the edge is pruned.

I'm uncomfortable with the handling of error messages that fail in signature validation, as they do prune edges. Doesn't that allow an attacker to send error messages and cause the routing to fail?

You've added it in here, but for other cases where r.applyChannelUpdate is called directly, the payment session isn't updated.

My idea is that the hints are updated only when a retry is aimed at. It now handles all those cases (not only insufficient fees). Otherwise the vertex or channel is removed completely, and there is no need for the update. I thought that is the whole purpose of defining processChannelUpdateAndRetry (which includes the paySession in its closure).
Said in another way, it doesn't seem right to me to plug it into applyChannelUpdate, exactly because this function is called for changing database edges/vertexes (and don't have paySession in closure).

Yes, I think you're right. All other calls to applyChannelUpdate prune immediately, so those edges will never show up in subsequent payment attempts anymore.

That's okay, just wanted to mention the option.

Are you updating the channel in both directions? I think you may also need to match the pubkey in additional edges to update only a single direction.

The channel is updated only in one direction. That is, the node generates an error on the outgoing channel policy, and you update the hints accordingly. Am I missing something here?

I mean, suppose you have two additional edges relating to the same channel id but in opposite direction. They will both be updated?

Yes. I changed it such that it takes direction into account too, such that policy will be updated only if channel ID and direction both match.
I think this also fixes a general hidden bug in routing through hint hops, not related to this PR, as additionalEdges policies didn't take into account direction. That is, the Flags were not initialized in NewPaymentSession, and routing assumes the direction is initialized in all edge policies.

New line after wrapped line

Done

failedEdge.direction can be used directly.

Not sure. failedEdge direction is unit8 while Flags are unit16.

Ok, you may need to do a type cast, but the if statement is not necessary.

Done

remove newline

Done

Capital at beginning of sentence. Period at end.

Fixed

Is this a fix in itself? Ideally this could be yet another separate commit, but if you find it too small for that, it's fine with me.

I had to add it as otherwise you would get errors on applyChannelUpdate when trying to update private channels hops.

I think maybe the proper fix here is something like this: ad92e5c (feel free to cherry-pick it into this PR if needed).

I believe then you wouldn't have to flip the AssumeChannelValid in the config during tests?

Created #2549

This can be removed now that #2549 is merged.

This vars are introduced because of line length I assume. But maybe that indicates that now is the time to extract this closure to a separate function.

Yes. But I don't get much if I move it to a separate function defined within the closure, as the names are so long here that it doesn't make it much better. I rather leave it as is. Defining a separate function altogether for an internal code that is called once is not something I personally prefer.

There are plenty of internal functions that are called only once, it does help to make the code more understandable. Inline functions can be useful especially if a lot of local vars are bound.

How about just creating a function taking failedEdge and update and returning the ChannelEdgePolicy? Or better, updating the signature of UpdateEdgePolicy to take failedEdge and update. Also consistent with other pay session funcs that take an edgeLocator too.

Yes, this is the way to go. Thanks!

Line length.

Fixed

This was indeed not set, but I think it didn't have impact before, because that flag was never read right?

I think it did have impact, i.e. it was an open bug, but the situations where it matters were rare enough such that it wasn't noticed. I haven't fully thought this out. What happened was that when I tried to fix UpdateEdgePolicy to consider direction as well, following your comment, the (new) test failed, and adding direction here fixed it.

Yes, it was an omission that surfaced when you fixed UpdateEdgePolicy. But I think that before that, the situations where it mattered where non-existent. Just interested to know whether that was true. In any case, it is fixed now.

Is this still needed?

Excellent test description! 👍

move defer cleanUp() after error check

not needed anymore I think

nit: missing period.

just return non-pointer value.

also better to take non-pointer for the edgeLocator here.

This can be removed now that #2549 is merged.

same comment wrt cleanup

can be removed