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.

Sign up for GitHub

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

Jump to bottom

lightningd/options.c: Add option for setting how long to keep trying bitcoin-cli command. #2780

Merged
merged 1 commit into from
Jul 18, 2019
Merged

lightningd/options.c: Add option for setting how long to keep trying bitcoin-cli command. #2780

merged 1 commit into from
Jul 18, 2019

Conversation

ZmnSCPxj
Copy link
Contributor

Requested in: #2779

@ZmnSCPxj ZmnSCPxj force-pushed the bitcoin-try-timeout branch from bf9cd43 to 5c0b9e5 Compare June 29, 2019 19:32
@ZmnSCPxj ZmnSCPxj requested a review from cdecker June 29, 2019 19:33
cdecker
cdecker requested changes Jun 29, 2019
View reviewed changes
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a bit more work, since it now crashes quite a few tests and I have a minor name change proposal.

We could also add --bitcoin-rpcclienttimeout that passes through to bitcoin-cli. That was requested in #2778. It doesn't make sense that we are strict about the 1 minute failure timeout, but happily sit there waiting for 15 minutes when the backend hangs 😃

lightningd/options.c Outdated Show resolved Hide resolved
@ZmnSCPxj ZmnSCPxj force-pushed the bitcoin-try-timeout branch from 5c0b9e5 to 5167f35 Compare June 30, 2019 01:36
@ZmnSCPxj ZmnSCPxj mentioned this pull request Jun 30, 2019
Closed
@ZmnSCPxj ZmnSCPxj force-pushed the bitcoin-try-timeout branch from 5167f35 to 67997a2 Compare June 30, 2019 10:00
@ZmnSCPxj
Copy link
Contributor Author

Previous failures were caused by #2782, now that #2783 has been merged, should be fixed.

@ZmnSCPxj ZmnSCPxj requested a review from cdecker July 1, 2019 09:29
@ZmnSCPxj
Copy link
Contributor Author

PING

rustyrussell
rustyrussell approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@rustyrussell rustyrussell left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, but I'd like to see us also hand that argument through to '-rpcclienttimeout' as well, as @cdecker points out. That can obv be a separate commit.

@ZmnSCPxj
Copy link
Contributor Author

I like it, but I'd like to see us also hand that argument through to '-rpcclienttimeout' as well, as @cdecker points out. That can obv be a separate commit.

Should we consider the time that has already passed, then subtract that from the configured value, before passing to -rpcclienttimeout?

cdecker
cdecker approved these changes Jul 18, 2019
View reviewed changes
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 67997a2

@cdecker cdecker mentioned this pull request Jul 18, 2019
Closed
@cdecker cdecker merged commit bb30104 into ElementsProject:master Jul 18, 2019
@ZmnSCPxj ZmnSCPxj deleted the bitcoin-try-timeout branch July 20, 2019 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdecker cdecker cdecker approved these changes

@rustyrussell rustyrussell rustyrussell approved these changes

@darosior darosior darosior approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ZmnSCPxj @cdecker @rustyrussell @darosior