BIP90: Make buried deployments slightly more easily extensible

jtimon commented at 12:17 pm on September 30, 2017: contributor

Also make it more similar to BIP9’s implementation.

jtimon force-pushed on Sep 30, 2017

in src/consensus/params.h:71 in 4a878203d8 outdated

61      * Examples: 1916 for 95%, 1512 for testchains.
62      */
63     uint32_t nRuleChangeActivationThreshold;
64     uint32_t nMinerConfirmationWindow;
65+    /** BIP90: Block height at which buried deployments becomes active */
66+    int buried_deployments[MAX_BURIED_DEPLOYMENTS];

jtimon commented at 4:21 pm on September 30, 2017:

uint32_t instead of int?

jtimon force-pushed on Sep 30, 2017

fanquake added the label Consensus on Oct 1, 2017

in src/consensus/params.h:60 in c12ae89a3a outdated

61      * Examples: 1916 for 95%, 1512 for testchains.
62      */
63     uint32_t nRuleChangeActivationThreshold;
64     uint32_t nMinerConfirmationWindow;
65+    /** BIP90: Block height at which buried deployments becomes active */
66+    uint64_t buried_deployments[MAX_BURIED_DEPLOYMENTS];

ryanofsky commented at 3:05 pm on October 2, 2017:

Seems like it would fit in better with existing code to use int for block heights instead of uint64_t. Also would avoid signed/unsigned comparisons.

ryanofsky commented at 3:08 pm on October 2, 2017: member

utACK c12ae89a3a898fe7778ef29c15527e698bf33b5b

TheBlueMatt commented at 7:48 pm on October 3, 2017: member

This feels like entirely needless code churn. This doesn’t change the memory layout but add one extra entry of padding, arrays are probably just more brittle than named variables (now you’re actually doing pointer offset and can lookup wrong), and doesn’t seem to provide any benifit aside from fewer entries in the chainparams, which I dont think is a useful goal.

ryanofsky commented at 8:10 pm on October 3, 2017: member

I think the change is fine, but I agree with Matt in not seeing any obvious advantages to this approach. @jtimon, I wonder if there are any concrete advantages you would cite, or if you see this as mostly a stylistic change.

Also, it might be helpful in reviewing these PRs to have a better idea of how you want the final public libconsensus interface to look. If you had a document listing the different header files and describing what kinds of functions they expose, that would help put these changes in context.

jtimon commented at 2:49 am on October 4, 2017: contributor

I agree this is not necessary. It is meant to make the code clearer and simplify bip90 related changes like #11398 If it doesn’t, then we shouldn’t do this. I honestly think using an array the way we’re doing it here posses any risk, but if it does, perhaps we should consider changing vDeployments (bip9/bip8) to named variables too, to avoid those risks. I think making the code for bip90 more consistent with the code for bip9 is a good think but perhaps this inconsistency is preferred for some reason I’m missing?

Also, it might be helpful in reviewing these PRs to have a better idea of how you want the final public libconsensus interface to look. If you had a document listing the different header files and describing what kinds of functions they expose, that would help put these changes in context.

I am no longer working on libconsensus. Of all my opened PRs, I only used #8498 (well, some older version) in my longest branch for libconsensus.
Regarding no documentation, I assume you may have missed some of the following links:

#6714 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2016-January/012235.html https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2016-October/013204.html #5946 https://github.com/jtimon/bitcoin/commits/0.12.99-consensus

I have more links if you’re interested and of course feel free to ask any questions you may have about my abandoned work on irc, the mailing list or by mail, whatever. But this PR doesn’t seem the right place.

ryanofsky commented at 8:52 am on October 4, 2017: member

Wow, those libconsensus docs are really interesting, and I hadn’t seen them before. It probably is worth discussing more, but I guess not here (I didn’t realize this PR was only intended be a standalone cleanup).

I think we need to a better job reviewing cleanup PR’s like this. It seems like when a PR making a stylistic change gets posted, there’s either a contentious debate with ACKs and NACKs and exaggerated comments about why the change is wonderful or terrible, or people withhold opinions and ignore the PR because they don’t want to risk starting an unproductive debate.

Maybe a better way to handle cleanup changes would be to do a noncommital round of polling before asking for hard ACKs and NACKs, asking maybe 4 or 5 developers to take an initial look at the change and write a sentence saying whether they like, dislike, or are indifferent to it. Apache’s voting system could be useful for this:

+0: ‘I don’t feel strongly about it, but I’m okay with this.’ -0: ‘I won’t get in the way, but I’d rather we didn’t do this.’ -0.5: ‘I don’t like this idea, but I can’t find any rational justification for my feelings.’ ++1: ‘Wow! I like this! Let’s do it!’ -0.9: ‘I really don’t like this, but I’m not going to stand in the way if everyone else wants to go ahead with it.’

This way people would feel more free to comment on cleanup PRs without forming hard opinions or getting sucked into debate, so cleanup authors could get quicker feedback on their PRs, and make faster decisions on whether to close, improve, or move forward with their changes, and there could be less of a cleanup PR backlog.

TheBlueMatt commented at 4:29 pm on October 4, 2017: member

Heh, alright, I’m a -0 on this one =D.

ryanofsky commented at 5:43 pm on October 4, 2017: member

I’m +0. I like the deployments being grouped together and not sitting top level consensus params. I might put them in a struct instead of an array, but I mostly just think the change is harmless and not significant.

sdaftuar commented at 4:58 pm on October 5, 2017: member

-0 is a pretty good summary of my view as well

jtimon commented at 2:46 pm on October 15, 2017: contributor

@ryanofsky thanks for the suggestion and thanks everyone for the feedback. But I don’t think it should deter discussion. I’m really interested on why people see the buried blocks enum and array here differently from versionbits’ enum and array. I really see them similar.

ryanofsky commented at 2:36 pm on October 16, 2017: member

It’s a minor change which isn’t an unambiguous improvement and doesn’t have a broader impact, so I wouldn’t expect it to attract a lot of positive feedback. I like that the change groups deployments and would merge it even though I think grouping them under a struct would be a better than grouping them in an array. Arrays with constant offsets are more verbose than structs, less flexible, and also brittle like matt pointed out. I’d probably use a struct for vb deployments as well, or anything else I didn’t think code would frequently be iterating over.

MarcoFalke commented at 6:28 pm on November 10, 2017: member

Needs rebase if still relevant

BIP90: Make buried deployments slightly more easily extensible 2193faab15

jtimon force-pushed on Nov 11, 2017

jtimon commented at 7:49 pm on November 11, 2017: contributor

Rebased

ryanofsky commented at 8:53 pm on January 8, 2018: member

utACK 2193faab150a8373da94d171cd459d58bb87af97

MarcoFalke commented at 0:45 am on January 23, 2018: member

Note that this conflicts with #11739, which removes BIP16Height.

in src/consensus/params.h:18 in 2193faab15

12@@ -13,6 +13,15 @@
13 
14 namespace Consensus {
15 
16+enum BuriedDeploymentPos
17+{
18+    DEPLOYMENT_BIP16,

MarcoFalke commented at 0:49 am on January 23, 2018:

Note that the title says BIP90, but BIP90 never mentions BIP16. So either the code needs to change or the title.

jtimon commented at 2:23 am on January 23, 2018:

Couldn’t bip90 change as it does with new additions like hopefully csv and segwit?

EDIT: assuming BIP16Height is not removed (still reading #11739 )

MarcoFalke commented at 7:30 pm on January 26, 2018:

No, I’d rather not change BIPs to be a running target. The previous release implements BIP 90, so you can’t change it after it is published.

jtimon commented at 2:55 am on January 23, 2018: contributor

Yeah, this should probably be rebased on top of #11739

jtimon commented at 4:53 am on March 29, 2018: contributor

Closing. But not for lack of enthusiasm, the next thing, I don’t remember the name. More reasoned NACKS always welcomed. But no concept ACK either for very long, let’s close this. I’ve seen at least 2 PRs more interesting in this space to review that may still be open when you read this.

jtimon closed this on Mar 29, 2018

MarcoFalke locked this on Sep 8, 2021

BIP90: Make buried deployments slightly more easily extensible #11426