Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

watchtower/client: immediately negotiate session with first candidate tower #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

calvinrzachman
Copy link
Owner

@calvinrzachman calvinrzachman commented Oct 15, 2022

Whether or not we have any candidate towers, the client will request its session negotiator begin attempting to negotiate a session. While there are no candidate towers, the negotiator will just churn with exponential backoff. Upon addition of the first tower, we won't establish a usable session until after the exponential backup delay times out. In order to avoid waiting for exponential backoff, we should not begin session negotiation until we have a candidate tower. This way we can immediately negotiate a session with the first newly added tower and promptly get to backing up those payment channel state updates!

# A watchtower client will churn in its session negotiation loop even when it has no candidate towers
2022-10-13 15:36:44.773 [INF] WTCL: (legacy) Starting watchtower client
2022-10-13 15:36:44.774 [DBG] WTCL: (legacy) Starting session negotiator
2022-10-13 15:36:44.786 [INF] WTCL: (legacy) Requesting new session.
2022-10-13 15:36:44.786 [DBG] WTCL: (legacy) Dispatching session negotiation
2022-10-13 15:36:44.788 [DBG] WTCL: (legacy) Unable to get new tower candidate, retrying after 10s -- reason: exhausted all tower candidates
2022-10-13 15:36:44.786 [INF] WTCL: (legacy) Watchtower client started successfully
2022-10-13 15:36:44.794 [INF] WTCL: (anchor) Starting watchtower client
2022-10-13 15:36:44.795 [DBG] WTCL: (anchor) Starting session negotiator
2022-10-13 15:36:44.797 [INF] WTCL: (anchor) Watchtower client started successfully
2022-10-13 15:36:44.797 [INF] WTCL: (anchor) Requesting new session.
2022-10-13 15:36:44.799 [DBG] WTCL: (anchor) Dispatching session negotiation
2022-10-13 15:36:44.800 [DBG] WTCL: (anchor) Unable to get new tower candidate, retrying after 10s -- reason: exhausted all tower candidates
2022-10-13 15:36:54.794 [DBG] WTCL: (legacy) Unable to get new tower candidate, retrying after 20s -- reason: exhausted all tower candidates
2022-10-13 15:36:54.802 [DBG] WTCL: (anchor) Unable to get new tower candidate, retrying after 20s -- reason: exhausted all tower candidates
2022-10-13 15:37:14.803 [DBG] WTCL: (legacy) Unable to get new tower candidate, retrying after 40s -- reason: exhausted all tower candidates
2022-10-13 15:37:14.811 [DBG] WTCL: (anchor) Unable to get new tower candidate, retrying after 40s -- reason: exhausted all tower candidates
2022-10-13 15:37:54.799 [DBG] WTCL: (legacy) Unable to get new tower candidate, retrying after 1m20s -- reason: exhausted all tower candidates
2022-10-13 15:37:54.808 [DBG] WTCL: (anchor) Unable to get new tower candidate, retrying after 1m20s -- reason: exhausted all tower candidates
2022-10-13 15:39:14.800 [DBG] WTCL: (legacy) Unable to get new tower candidate, retrying after 2m40s -- reason: exhausted all tower candidates
2022-10-13 15:39:14.807 [DBG] WTCL: (anchor) Unable to get new tower candidate, retrying after 2m40s -- reason: exhausted all tower candidates
2022-10-13 15:41:54.810 [DBG] WTCL: (legacy) Unable to get new tower candidate, retrying after 5m0s -- reason: exhausted all tower candidates
2022-10-13 15:41:54.818 [DBG] WTCL: (anchor) Unable to get new tower candidate, retrying after 5m0s -- reason: exhausted all tower candidates

# Add a tower, but notice that the client does not promptly begin session negotiation
$ lncli wtclient add 033e98efe2d4d1fc1cfa4992e98564b275bdfddf38387c7cc295954a58862da721@172.31.0.2

... 5 minutes later
2022-10-13 15:46:54.818 [DBG] WTCL: (legacy) Attempting session negotiation with tower=033e98efe2d4d1fc1cfa4992e98564b275bdfddf38387c7cc295954a58862da721

While this PR is not strictly necessary for correctness, it may provide a rather small quality of life improvement in the scenario in which a user is adding a first tower. Let me know if this is something others think is worth adding or if the small behavior change is not worth the addition of another similarly structured select{} statement.

Thanks!

See here for alternative way to achieve the same thing, possibly with an easier to read diff, but which does add a somewhat odd method to the SessionNegotiator interface.

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

@github-actions
Copy link

Pull reviewers stats

Stats of the last 30 days for lnd:

User Total reviews Time to review Total comments

@calvinrzachman
Copy link
Owner Author

calvinrzachman commented Oct 15, 2022

2022-10-14 20:49:49.876 [INF] WTCL: (legacy) Watchtower client starting
2022-10-14 20:49:49.877 [DBG] WTCL: (legacy) Starting session negotiator
2022-10-14 20:49:49.878 [INF] WTCL: (legacy) Watchtower client started successfully
2022-10-14 20:49:49.880 [INF] WTCL: (anchor) Watchtower client starting
2022-10-14 20:49:49.878 [DBG] WTCL: (legacy) No candidate towers. Waiting for tower before requesting session.
2022-10-14 20:49:49.881 [DBG] WTCL: (anchor) Starting session negotiator
2022-10-14 20:49:49.883 [INF] WTCL: (anchor) Watchtower client started successfully
2022-10-14 20:49:49.883 [DBG] WTCL: (anchor) No candidate towers. Waiting for tower before requesting session.

# NOTE: Observe the time gap where our dispatcher loop blocks awaiting the first candidate tower to be added. 
# Also observe how we handle things differently depending on whether the tower has a previously established
# session we can use.

$ lncli wtclient add 032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438@172.27.0.3

# Following the tower add, this code runs without delay.
2022-10-14 20:51:26.609 [DBG] WTCL: (legacy) Added new tower: 032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438
2022-10-14 20:51:26.610 [INF] WTCL: (legacy) Requesting new session.
2022-10-14 20:51:26.612 [INF] WTCL: (legacy) Client stats: tasks(received=0 accepted=0 ineligible=0) sessions(acquired=0 exhausted=0)
2022-10-14 20:51:26.612 [DBG] WTCL: (legacy) Dispatching session negotiation
2022-10-14 20:51:26.618 [DBG] WTCL: (legacy) Attempting session negotiation with tower=032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438
2022-10-14 20:51:26.618 [DBG] WTCL: (anchor) Added new tower: 032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438
2022-10-14 20:51:26.623 [INF] WTCL: (anchor) Requesting new session.
2022-10-14 20:51:26.625 [INF] WTCL: (anchor) Client stats: tasks(received=0 accepted=0 ineligible=0) sessions(acquired=0 exhausted=0)
2022-10-14 20:51:26.625 [DBG] WTCL: (anchor) Dispatching session negotiation
2022-10-14 20:51:26.635 [DBG] WTCL: (anchor) Attempting session negotiation with tower=032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438
… establishes a session

$ lncli wtclient remove 032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438

2022-10-14 20:52:47.766 [INF] WTCL: (legacy) Client stats: tasks(received=0 accepted=0 ineligible=0) sessions(acquired=1 exhausted=0)
2022-10-14 20:52:47.767 [INF] WTCL: (anchor) Client stats: tasks(received=0 accepted=0 ineligible=0) sessions(acquired=1 exhausted=0)
2022-10-14 20:53:33.138 [DBG] WTCL: (legacy) No candidate towers. Waiting for tower before requesting session.
2022-10-14 20:53:33.143 [DBG] WTCL: (anchor) No candidate towers. Waiting for tower before requesting session.

# Re-add a tower we have previously negotiated with.
$ lncli wtclient add 032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438@172.27.0.3

2022-10-14 20:51:26.618 [DBG] WTCL: (legacy) Added new tower: 032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438
2022-10-14 20:51:26.618 [DBG] WTCL: (anchor) Added new tower: 032c9ad617366ed428d8156d985f048c79e1bc239788a8efd89bdf27a5f738d438
2022-10-14 20:55:47.852 [DBG] WTCL: (legacy) Using previously negotiated session(s) for tower: 032c9ad617366ed428d
2022-10-14 20:55:47.858 [DBG] WTCL: (legacy) Loaded next candidate session queue id=02f61fcf13b8f32200d69260c9e9e0d6cdd25c4b8b74c55605b7fe85a2408be4a1
2022-10-14 20:55:47.863 [INF] WTCL: (legacy) Client stats: tasks(received=0 accepted=0 ineligible=0) sessions(acquired=1 exhausted=0)
2022-10-14 20:55:47.867 [DBG] WTCL: (anchor) Using previously negotiated session(s) for tower: 032c9ad617366ed428d
2022-10-14 20:55:47.868 [DBG] WTCL: (anchor) Loaded next candidate session queue id=032b5342d40de2680e94b787754125ce2972c7af73e4e11d1981f8c309ca728c6a

# Continues dumping stats periodically

2022-10-14 20:55:47.869 [INF] WTCL: (anchor) Client stats: tasks(received=0 accepted=0 ineligible=0) sessions(acquired=1 exhausted=0)
2022-10-14 20:56:47.759 [INF] WTCL: (legacy) Client stats: tasks(received=0 accepted=0 ineligible=0) sessions(acquired=1 exhausted=0)

…ed tower

Whether or not we have any candidate towers, the session negotiator
will begin attempting to negotiate a session. While there are no
candidate towers, the negotiator will just churn with exponential
backoff. Upon addition of the first tower, we won't establish a usable
session until after the exponential backup delay times out.
In order to avoid waiting for exponential backoff, we should not begin
session negotiation until we have a candidate tower. This way we can
immediately negotiate a session with the first newly added tower and
promptly get to backing up those payment channel state updates!
@calvinrzachman calvinrzachman force-pushed the immediate-first-session branch from 610ab51 to 23df7e0 Compare October 16, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant