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

Remove bad chain alert partition check #8275

Merged
merged 1 commit into from Jul 6, 2016

Conversation

Projects
None yet
6 participants
@btcdrak
Member

btcdrak commented Jun 27, 2016

As per meeting 2016-03-31 https://bitcoincore.org/en/meetings/2016/03/31/#bad-chain-alerts

The partition checker was producing huge number of false-positives and was disabled in 0.12.1 on the understanding it would either be fixed in 0.13 or removed entirely from master if not.

Remove bad chain alert partition check
As per meeting 2016-03-31
https://bitcoincore.org/en/meetings/2016/03/31/#bad-chain-alerts

The partition checker was producing huge number of false-positives
and was disabled in 0.12.1 on the understanding it would either be
fixed in 0.13 or removed entirely from master if not.
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 27, 2016

Member

Duplicate of #8241?

Member

sdaftuar commented Jun 27, 2016

Duplicate of #8241?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 27, 2016

Member

You are also removing the tests, so something like #7568 would require to add them back later.

Member

MarcoFalke commented Jun 27, 2016

You are also removing the tests, so something like #7568 would require to add them back later.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jun 27, 2016

Member

Not a duplicate, this is deliberate and should replace #8241: we disabled the alerts for 0.12.1 and agreed the partition alert code would be removed 0.13.0 if it wasnt fixed satisfactorily in #7568.

Member

btcdrak commented Jun 27, 2016

Not a duplicate, this is deliberate and should replace #8241: we disabled the alerts for 0.12.1 and agreed the partition alert code would be removed 0.13.0 if it wasnt fixed satisfactorily in #7568.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 28, 2016

Member

Yes removing the code entirely is a more final option, though it could be re-added if something better pops up.

Which to choose that depends. If this is just due to lack of review time / too many high-priority issues pre-0.13, which was my guess, it makes sense to postpone the deadline to 0.14 and do #8241 in the meantime.

But if it is considered a bad, broken idea and no one is motivated to look at this at all, it's better to remove it and do this.

Member

laanwj commented Jun 28, 2016

Yes removing the code entirely is a more final option, though it could be re-added if something better pops up.

Which to choose that depends. If this is just due to lack of review time / too many high-priority issues pre-0.13, which was my guess, it makes sense to postpone the deadline to 0.14 and do #8241 in the meantime.

But if it is considered a bad, broken idea and no one is motivated to look at this at all, it's better to remove it and do this.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jun 29, 2016

Contributor

It's not like we're deleting it from git history, and disabled code has a tendency to break further anyway.

Concept ACK

Contributor

petertodd commented Jun 29, 2016

It's not like we're deleting it from git history, and disabled code has a tendency to break further anyway.

Concept ACK

@laanwj laanwj merged commit ab8be98 into bitcoin:master Jul 6, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jul 6, 2016

Merge #8275: Remove bad chain alert partition check
ab8be98 Remove bad chain alert partition check (BtcDrak)

@btcdrak btcdrak deleted the btcdrak:partitionalerts branch Dec 3, 2016

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