Absolute Time Lock Limit

[johng]

As per #1513 I've added a field to queryroutes and sendpayment which limits the maximum total time funds can be locked in htlcs along a given route.

The main changes involve replacing the maximum fee parameter with a struct which contains both restrictions.

[joostjager]

Useful functionality to have and implemented the right way (inside findPath). I've left comments in the code.

General remarks:

• For messages, see the contributor guidelines. We prefix the messages with the subsystem (see history of master branch).

• Commits should be organized in either independent sets of changes or logic sets that build on top of eachother, ideally each one successfully compiling/running. For this PR, you could think of a commit structure and order like this (not to be taken literally as commit messages):

  • change final_cltv_delta default
  • naming test cases
  • move InsufficentCapacity Test into table driven test
  • QueryRoutes improvements to help texts
  • create PathRestrictions struct (with just fee limit)
  • add max_cltv_delay field for sendpayment and queryroutes
  • disqualify routes violating the max cltv in newRoute
  • take max cltv into account during path finding
    (add unit test changes/additions to the commit to which they apply)

[joostjager]

Changing ids in protos in not a good idea. People may have applications built against a previous version.

[joostjager]

Same comment on ids

[joostjager]

Comment incorrect

[joostjager]

Check sentence, maximum time lock for a route.

[joostjager]

Is it dangerous / do we cast somewhere to int?

[johng]

We don't do any casting from uints to ints. If we do in the future that will cause issues if the limit is sufficiently large but should be picked up by the test cases

[joostjager]

I think this test case is still valuable. It covers a specific detail on fees.

[joostjager]

If think the default expectedFailure (if not specified in the testcase) is ErrNoPathFound. So what happens here is that if an unexpected error occurs and it happens to be ErrNoPathFound, the test will pass?

[joostjager]

Check name

[joostjager]

I haven't looked at the code changes themselves yet, but the commit structure is not yet what it should be. You should use git rebase to get it right. Commits that fix problems that were introduced in earlier commits that are also part of this PR should be squashed together.

The end result should be a set of commits that build on top of each other and it should look like it was done first time right.

[joostjager]

The branch needs to be rebased onto master as well to fix conflicts.

[johng]

I don’t see how doing one big commit is going better than breaking it up. Especially if there are further comments. Can I not squash it after changes are approved?

…

On 29 Oct 2018, at 10:41, Joost Jager ***@***.***> wrote: I haven't looked at the code changes themselves yet, but the commit structure is not yet what it should be. You should use git rebase to get it right. Commits that fix problems that were introduced in earlier commits that are also part of this PR should be squashed together. The end result should be a set of commits that build on top of each other and it should look like it was done first time right. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[joostjager]

Breaking it up logically makes reviewing quicker, because it is easier to assess unrelated changes separately. Reviewing before clean up means that a second review (albeit shorted) is needed after clean up.

Also, when people look at a particular change in the future (after merge), they are directed to a logical commit of which that change was part. This helps to understand the intention of the change.

In this PR, there are so many commits, that is probably easier to start with all changes in the working directory and then stage and commit bit by bit (see structure that I suggested earlier). So rebuild the commit structure from scratch rather than trying to transform the current commits into the intended structure.

[joostjager]

There are some commits from others in the diff now that don't belong in this PR.

Also, not all of the changes that you first had are in?

W/r to commit structure, the idea is to group the changes functionally. Like in the example breakdown I gave above. So "giving test cases a name" is a commit that can exist in isolation. Also it is preferred to have commits building successfully.

[halseth]

I believe this PR solves what it set out to to, namely making it possible to specify a max cltv in order to avoid funds getting locked up for a long time.

However, this is not the optimal solution, as there might still be cases where a path does exist to the target with a satisfactory CLTV limit, but since it is not the cheapest one, it won't be found. To support this would be a non-trivial change to the shortest path algorithm though, so it is out of scope for this PR.

[halseth]

It is a bit hard to see what are the actual test additions here. Ideally the commits would be structured like

1. Modify the test data, any necessary refactoring and existing tests to work with this data.
2. Add the new tests.

[joostjager]

No recent activity on this PR. I reworked the changes on top of the current master branch, but unfortunately not much of the code remained.

#2640

[wpaulino]

Thanks for getting this started @johng! Superseded by #2640.