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

Do not consider bitcoin-cli as a failed call if it returns error code 1 #2262

Merged

Conversation

Projects
None yet
2 participants
@NicolasDorier
Copy link
Contributor

commented Jan 13, 2019

It fixes a bad assumption in the code which consider error code of bitcoin-cli different than 0 as a transient issue.

If bitcoin-cli returns 1, then bcli_failure get called, which will try again every second for 1 minute then crash the process.

So what happen on a pruned node?
A gossip come, then c-lightning try to check if the channel exists by fetching the block via getblock.
bitcoin-cli, not having the block, returns "block-not-found" but also the error code of bitcoin-cli is 1.

The caller's callback process_block expects to process the case where the block is not found.

However, the callback never get called because bcli_failure keeps retrying the same command over and over.

This in turn flood the timer queue which make the node eventually crash. (or the 1 min timeout get reached and it kills the process)

Fix #2250

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

I am still a bit puzzled about why this happened only after the 0.6.3 update. (not isolated case, at least 3 different people reported it just after the 0.6.3 update)

I reviewed all the code which could have triggered this situation and it seemed to me this should have happen in 0.6.2 as well. But never had one report.

@cdecker cdecker requested a review from rustyrussell Jan 13, 2019

@rustyrussell rustyrussell force-pushed the btcpayserver:fix/inifinite-loop branch from ac1116a to 5cfa565 Jan 15, 2019

bitcoind: allow "getblock" to fail for txout lookup.
Apparently on pruned nodes it sometimes gives exit status 1?

@rustyrussell rustyrussell force-pushed the btcpayserver:fix/inifinite-loop branch from 5cfa565 to 503744d Jan 15, 2019

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Reviewed, it seems it should work.
Will try it now.

@NicolasDorier

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Seems to work!

@rustyrussell
Copy link
Contributor

left a comment

Ack 503744d

@rustyrussell rustyrussell merged commit a565915 into ElementsProject:master Jan 15, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.