Skip to content

Consensus: Modified BIP8 implementation#10462

Closed
jtimon wants to merge 5 commits intobitcoin:masterfrom
jtimon:b15-bip8-min
Closed

Consensus: Modified BIP8 implementation#10462
jtimon wants to merge 5 commits intobitcoin:masterfrom
jtimon:b15-bip8-min

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented May 26, 2017

This is @shaolinfry 's implementation of bip8, but rebased, with some minor nits by myself freely applied, some squashes and with some sensible renames left out for later/before (whatever is decided, that's just self-documentation; orthogonal. Didn't separate s/BIP9SoftForkDescPushBack/VBSoftForkDescPushBack/ because I don't think it's worth it: it helps for review IMO and it's minimal).

Please no merge before we decide what to do with my new TODO comment lines (I want to add something to bip8 for warnings), but I think review for bip8 as it is now can begin at any time.
Kudos again to @shaolinfry not just for the idea but also for the cleanness: I enjoyed reviewing and rebasing this.

EDIT:

This modifies the original BIP specification to enable warnings for unkown deployments being locked in by bip8 timeout as proposed in https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-May/014417.html

Dependencies:

@jtimon jtimon changed the title BIP8 implementation NOMERGE: Consensus: BIP8 implementation May 26, 2017
@shaolinfry
Copy link
Contributor

@jtimon It was not squashed because that harms review, people need to see each atomic change. Squash can be trivial after the code review. FWIW I have deliberately held back on any PRs to Bitcoin Core until nearer the time. I don't think "NOMERGE" pull requests are helpful, better to just have a serious pull request.

@jtimon
Copy link
Contributor Author

jtimon commented May 27, 2017

It was not squashed because that harms review, people need to see each atomic change. Squash can be trivial after the code review.

I can separate it back, but I disagree that having this in a single commit hurts review or that the commit doesn't represent an atomic change. And it's still quite simple.

FWIW I have deliberately held back on any PRs to Bitcoin Core until nearer the time.

What time? BIP8 is a generic mechanism that can be used for any future deployment.

I don't think "NOMERGE" pull requests are helpful, better to just have a serious pull request.

I think this PR is very serious as it provides the implementation for the current bip8 specification.
I added "NOMERGE" because of the TODOs, because I want bip8's specification to change (and then change this PR to add the suggested addition), please, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-May/014417.html

@jtimon jtimon changed the title NOMERGE: Consensus: BIP8 implementation Consensus: Modified BIP8 implementation May 27, 2017
@jtimon
Copy link
Contributor Author

jtimon commented May 27, 2017

@shaolinfry updated the code to include my proposed modifications to bip8, updated OP and tittle.
If you like the idea, I'm happy to help modifying the specification, otherwise I guess I will create a competing an extension one.

" },\n"
" }, ...\n"
" ],\n"
" \"bip8_softforks\": { (object) status of BIP9 softforks in progress\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: should be BIP8 softforks

@shaolinfry
Copy link
Contributor

This modification doesnt make sense and I already went through a similar iteration during development.

The warning system will already notify users of unknown bits being signalled, and considering UASF deployment will be for a year. For upgraded clients they will also additionally emit warnings once LOCKED_IN has been reached. Unknown bits is also backwards compatible with BIP9 aware nodes.

Asking miners to set a bit in order to trigger warnings seems fraught with trouble as well as for false triggering. I dont think it would work for parallel deployments.

Soft forks are always deployed with considerable planning, and ecosystem collaboration, we cannot rely just on technical means, and actually, too many error messages or false warning just numbs users into ignoring them. We already say that happen with the chain split detection system which was removed a while ago.

@jtimon
Copy link
Contributor Author

jtimon commented Jun 5, 2017

Thanks for the review!

The warning system will already notify users of unknown bits being signalled

Only if the same bit is active for 95% of a diff adjustment period (ie only when an unkown bip9 deployment seems to have locked in).

and considering UASF deployment will be for a year.

This depends on the concrete deployment, bip8 is general like bip9.

For upgraded clients they will also additionally emit warnings once LOCKED_IN has been reached. Unknown bits is also backwards compatible with BIP9 aware nodes.

Only if they lock in by bip9, not by bip8 (what this modification covers).

Asking miners to set a bit in order to trigger warnings seems fraught with trouble as well as for false triggering. I dont think it would work for parallel deployments.

If they don't signal, the warning after lock in via bip8 won't be shown, but that's no worse than the current bip8 without this modification.
False signaling is more concerning, probably the code should allow for bip8 warnings to be disabled optionally.
False signaling is also possible today (although requires 95% to trigger the warning), so perhaps we should also add an option to disable bip9 warnings.

I think there's little incentive for miners to false signal here (or with plain bip9) though.

@jtimon
Copy link
Contributor Author

jtimon commented Jun 24, 2017

Closing. I will maybe reopen when @shaolinfry opens his own PR with master...shaolinfry:bip-uaversionbits

@jtimon jtimon closed this Jun 24, 2017
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants