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

Explictly check at startup that we don't support bitcoind's blocksonly option. #3889

Merged
merged 4 commits into from
Aug 3, 2020
Merged

Explictly check at startup that we don't support bitcoind's blocksonly option. #3889

merged 4 commits into from
Aug 3, 2020

Conversation

darosior
Copy link
Contributor

This adds a sanity check function to bcli which we could extend with other sanity checks in the future.

This was brought up by @ZmnSCPxj.

@darosior darosior requested a review from cdecker as a code owner July 29, 2020 17:41
@ZmnSCPxj
Copy link
Contributor

bfc73eb in #3870 also does a getnetworkinfo, but rather than a separate call, it replaces the echo in wait_for_bitcoind, what do you think? Since we need to call getnetworkinfo anyway I think replacing the echo would be better (though this is arguably a tiny optimization).

@ZmnSCPxj
Copy link
Contributor

Also we should probably update README.md as well, nobody reads anything in doc/ anyway.

@darosior
Copy link
Contributor Author

I updated the README in the first commit.

Regarding replacing echo, I agree it's cleaner after having read your commit. Will amend!

@darosior @ZmnSCPxj
bcli: poll getnetworkinfo at startup
b934a92 
This allows us to kill two birds with one stone: once connected, we use
the output of the successful call to do some sanity checks (only
checking the version for now, but more are yet to come!).

Changelog-Added: We now explicitly check at startup the version of our default Bitcoin backend (bitcoind).
Co-Authored-by: ZmnSCPxj <zmnscpxj@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@darosior
Copy link
Contributor Author

Adapted part of bfc73eb in b934a92 and built upon it for transaction relay sanity checks. Added a trivial check for bitcoind minimal supported version too. Also, this is a bit more restrictive than your initial version, as we fail to start if the call fails. I think it makes sense to not go further if a simple getnetworkinfo does not succeed.

ZmnSCPxj
ZmnSCPxj reviewed Jul 31, 2020
View reviewed changes
Copy link
Contributor

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

Mostly OK, minor misspelling in docs though.

README.md Outdated Show resolved Hide resolved
contrib/pyln-testing/pyln/testing/utils.py Show resolved Hide resolved
@ZmnSCPxj ZmnSCPxj mentioned this pull request Jul 31, 2020
Merged
@darosior
Copy link
Contributor Author

Corrected the grammar error.

ZmnSCPxj
ZmnSCPxj approved these changes Jul 31, 2020
View reviewed changes
Copy link
Contributor

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

ACK b2833f4

niftynei
niftynei approved these changes Jul 31, 2020
View reviewed changes
Copy link
Contributor

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

A few minor comments, otherwise lgtm.

plugins/bcli.c Outdated Show resolved Hide resolved
plugins/bcli.c Outdated Show resolved Hide resolved
plugins/bcli.c

if (!tx_relay)
plugin_err(p, "The 'blocksonly' mode of bitcoind, or any option "
"deactivating transaction relay is not supported.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe offer a suggestion of how to fix it? i.e.

Suggested change
"deactivating transaction relay is not supported.");
"deactivating transaction relay is not supported. Remove -blocksonly from your bitcoind config");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems a bit redundant ?..

The 'blocksonly' mode of bitcoind [...] is not supported. Remove -blocksonly from your bitcoind config

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree as well.

darosior added 3 commits August 2, 2020 13:08
@darosior
bcli: explicitly check at startup that bitcoind does relay transactions
e7227eb 
We've had problems with blocksonly in the past and bitcoind still allows
to use outdated (thus potentially dangerous estimates) while running
bitcoin with -blocksonly.

ZmnSCPxj mentionned that we still don't document this, but i figured
that in this specific case an explicit check and error seems preferable.

Changelog-Added: We now explicitly check at startup that our default Bitcoin backend (bitcoind) does relay transactions.

Proposed-by: ZmnSCPxj <zmnscpxj@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@darosior
pyln: allow to signal may_fail in get_node()
4679e11 
This or `expect_fail`, as it implies that, well, the node may fail.

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@darosior
pytest: test the blocksonly sanity checkin bcli
0049402 
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
ZmnSCPxj
ZmnSCPxj approved these changes Aug 2, 2020
View reviewed changes
Copy link
Contributor

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

ACK 0049402

plugins/bcli.c

if (!tx_relay)
plugin_err(p, "The 'blocksonly' mode of bitcoind, or any option "
"deactivating transaction relay is not supported.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree as well.

@rustyrussell rustyrussell merged commit 3aad86f into ElementsProject:master Aug 3, 2020
@darosior darosior deleted the no_blocksonly branch August 20, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZmnSCPxj ZmnSCPxj ZmnSCPxj approved these changes

@niftynei niftynei niftynei approved these changes

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

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
@darosior @ZmnSCPxj @niftynei @rustyrussell