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

peer+server: add new config option to optionally disable stall detection #1752

Merged
merged 1 commit into from
Oct 5, 2021
Merged

peer+server: add new config option to optionally disable stall detection #1752

merged 1 commit into from
Oct 5, 2021

Conversation

Roasbeef
Copy link
Member

In this commit, we add a new config options that allows one to start
btcd in an operating mode that disables the stall detection. This can
be useful in simnet/regtest integration tests settings where it's
important that btcd holds on to its possibly sole connection to the
only other node in the test harness.

A new config flag has been added to gate this behavior, which is off by
default.

@coveralls
Copy link

coveralls commented Sep 16, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 1296745038

  • 1 of 23 (4.35%) changed or added relevant lines in 3 files are covered.
  • 11 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-11.6%) to 52.965%
Changes Missing Coverage Covered Lines Changed/Added Lines %
peer/peer.go 1 4 25.0%
config.go 0 7 0.0%
server.go 0 12 0.0%
Files with Coverage Reduction New Missed Lines %
connmgr/connmanager.go 3 86.07%
peer/peer.go 8 75.49%
Totals Coverage Status
Change from base Build 1246015466: -11.6%
Covered Lines: 21085
Relevant Lines: 39809
💛 - Coveralls

@kcalvinalvin
Copy link
Collaborator

For user friendliness, I think it'd be better if the loadConfig() function had a check to error out if the DisableStallHandler flag is given when the node is on mainnet (since you'd never want this on for mainnet).

Fairly sure you wouldn't want this for testnet either..

guggero
guggero approved these changes Sep 16, 2021
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM, pending @kcalvinalvin's suggestion 🎉

@jcvernaleo
Copy link
Member

I agree with the check to disable this on mainnet. No opinion on testnet.

@guggero
Copy link
Collaborator

guggero commented Sep 20, 2021

We definitely need this to fix our itest flakes! I'm seeing this disconnect problem more often now.
cc @bhandras.

bhandras
bhandras approved these changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

peer/peer.go Outdated Show resolved Hide resolved
@jcvernaleo
Copy link
Member

Could you rebase this PR so it runs the correct actions?

@guggero
Copy link
Collaborator

guggero commented Sep 30, 2021

Hmm, this seems to block btcd completely since the stallControl channel isn't being drained. I have a fixed version here: https://github.com/guggero/btcd/tree/no-stall

@guggero guggero mentioned this pull request Sep 30, 2021
Merged
@Roasbeef Roasbeef force-pushed the config-disable-stall-handler branch from 361233c to ef3e88c Compare October 1, 2021 21:50
@Roasbeef
peer+server: add new config option to optionally disable stall detection
e98a1a1 
In this commit, we add a new config options that allows one to start
`btcd` in an operating mode that disables the stall detection. This can
be useful in simnet/regtest integration tests settings where it's
important that `btcd` holds on to its possibly sole connection to the
only other node in the test harness.

A new config flag has been added to gate this behavior, which is off by
default.
@Roasbeef Roasbeef force-pushed the config-disable-stall-handler branch from ef3e88c to e98a1a1 Compare October 1, 2021 21:56
@Roasbeef
Copy link
Member Author

Roasbeef commented Oct 1, 2021

@jcvernaleo @kcalvinalvin PTAL

Applied that fix, thanks @guggero!

guggero
guggero approved these changes Oct 5, 2021
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

tACK e98a1a1.

@Roasbeef Roasbeef merged commit e344999 into btcsuite:master Oct 5, 2021
@kcalvinalvin
Copy link
Collaborator

Post merge ACK e98a1a1b4cd7c59465a8f21bce80839b99a7337e. Github notifications are terrible...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhandras bhandras bhandras approved these changes

@guggero guggero guggero 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.
6 participants
@Roasbeef @coveralls @kcalvinalvin @jcvernaleo @guggero @bhandras