Skip to content

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

Merged
cdecker merged 1 commit intoElementsProject:masterfrom
ZmnSCPxj:bitcoin-try-timeout
Jul 18, 2019
Merged

lightningd/options.c: Add option for setting how long to keep trying bitcoin-cli command.#2780
cdecker merged 1 commit intoElementsProject:masterfrom
ZmnSCPxj:bitcoin-try-timeout

Conversation

@ZmnSCPxj
Copy link
Contributor

@ZmnSCPxj ZmnSCPxj commented Jun 29, 2019

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 😃

@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

ZmnSCPxj commented Jun 30, 2019

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

ZmnSCPxj commented Jul 16, 2019

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

ZmnSCPxj commented Jul 18, 2019

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

+1 more reviewer

@darosior darosior darosior approved these changes

Reviewers whose approvals may not affect merge requirements

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