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

Default -whitelistforcerelay to off #15193

Merged
merged 1 commit into from Jan 24, 2019

Conversation

Projects
None yet
8 participants
@sdaftuar
Copy link
Member

sdaftuar commented Jan 17, 2019

No one seems to use this "feature", and at any rate the behavior of relaying transactions when they violate local policy is error-prone, if we ever consider changing the ban behavior of our software from one version to the next.

Defaulting this to off means that users who use -whitelist won't be unexpectedly surprised by this interaction. If anyone is still relying on this feature, it can still be explicitly turned on.

@sdaftuar

This comment has been minimized.

Copy link
Member Author

sdaftuar commented Jan 17, 2019

ping @gmaxwell

@fanquake fanquake added the Validation label Jan 17, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 17, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15169 (WIP: Parallelize CheckInputs() in AcceptToMemoryPool() by sdaftuar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake fanquake changed the title Default -whitelistforcelay to off Default -whitelistforcrelay to off Jan 18, 2019

@fanquake fanquake changed the title Default -whitelistforcrelay to off Default -whitelistforcerelay to off Jan 18, 2019

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 18, 2019

Needs release note.

No one seems to use this "feature"

From what I vaguely remember bitonic.nl (Dutch exchange) uses, or at least used this.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Jan 19, 2019

Concept ACK. If we think there is still use we could add a FORCETX network message that is functionally identical to a TX message, but triggers the forced behaviour (and the user gets to live with the consequences) and is only permitted for WLed peers. I'm not aware of any continued dependency on this behaviour but we can check more.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 20, 2019

utACK

Concept ACK. If we think there is still use we could add a FORCETX network message

Well this PR doesn't remove the functionality it only disables it by default, so IMO there is no requirement to provide an alternative here even if it is still used.

Just need to be clear about it in the release notes (could even add a line to contact someone of us if they're still using this functionality)

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-01-forcerelayoff branch to a36d97d Jan 22, 2019

@sdaftuar

This comment has been minimized.

Copy link
Member Author

sdaftuar commented Jan 22, 2019

Updated with a release note.

@Joukehofman

This comment has been minimized.

Copy link

Joukehofman commented Jan 23, 2019

From what I vaguely remember bitonic.nl (Dutch exchange) uses, or at least used this.

We use bitcoin nodes as relay, but since we don't want them to relay transactions that violate the standard rules, we don't use this feature as default.

Edit: oh, just heard that we did for a while to mitigate an other problem that has since been resolved.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 23, 2019

utACK a36d97d

@promag

This comment has been minimized.

Copy link
Member

promag commented Jan 23, 2019

utACK a36d97d, previous behavior is still possible.

@laanwj laanwj merged commit a36d97d into bitcoin:master Jan 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 24, 2019

Merge #15193: Default -whitelistforcerelay to off
a36d97d Default -whitelistforcerelay to off (Suhas Daftuar)

Pull request description:

  No one seems to use this "feature", and at any rate the behavior of relaying transactions when they violate local policy is error-prone, if we ever consider changing the ban behavior of our software from one version to the next.

  Defaulting this to off means that users who use -whitelist won't be unexpectedly surprised by this interaction.  If anyone is still relying on this feature, it can still be explicitly turned on.

Tree-SHA512: 52650ad464a728d1648f496751e3f713077ea3a1de7278ed03531b2e8723e63cf2f6f41b56c98c0f73ffa22c36e01d9170b409ab452c737aca35b7ecd7a6b448
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment