< luke-jr>
of course not, we didn't even do rc1 yet
< phantomcircuit>
luke-jr, branched i mean
< phantomcircuit>
words
< btcdrak>
not yet
< jtimon>
NicolasDorier: When it's complete because it does too many things at once and when it's not complete because it's not complete? come on, make up your mind...
< btcdrak>
jtimon: my observation about ISM logic in versionbits.cpp is that it's got nothing to do with versionbits.cpp. its like you're stuffing a piece of code in there because it doesnt have anywhere else to live.
< gmaxwell>
ISM code should all leave now.
< btcdrak>
right, and now it seems useless to preserve that logic since we can just hard code it
< gmaxwell>
I was planning on doing that as soon as we branch.
< btcdrak>
the softforks have happened and are long since burred deep.
< btcdrak>
gmaxwell: +1
< jtimon>
btcdrak: and which of the alternatives I offereded you suggest?
< gmaxwell>
I have the patch done already (this is why I was checking to find out where the last couple softforks activated a month or so ago)-- or I think I have it, might have been in that repo I corrupted.
< jtimon>
hardcoded or not, it has to be somewhere
< gmaxwell>
jtimon: there will be no ISM code.
< jtimon>
I'm open ofr almost anything but main.cpp, please tell me what you like
< gmaxwell>
The code will be gone, it doesn't go anywhere.
< gmaxwell>
jtimon: we have soffork activations coded in chain paramters. (vDeployments) the places that call ISM are just changed to check that.
< gmaxwell>
just elements of consensus. consensus.BIP65Height yadda yadda.
< jtimon>
yeah, so hardcoding a height for activation, what I was saying
< btcdrak>
jtimon: for flags, why not in consensus.h?
< gmaxwell>
(which also makes block processing a bit faster, as the ISM checks are surprisingly slow)
< jtimon>
btcdrak: because that would be more of a "layer violation" concern when we have verifyBlock and a bunch of other functions in consensus.h
< jtimon>
btcdrak: why not consensus/flags.h ?
< jtimon>
gmaxwell: we still need to maintain the check I was talking about, right? (although it can be simplified to use the hardcoded heights too)
< jtimon>
btcdrak: can you respond to the getflags question? do you mind if that is in versionbits or do you prefer some other place? (not, I won't count "wherever, just not there" nor "let's leave that code in main for now" as answers)
< jtimon>
note
< gmaxwell>
jtimon: yes the if statement at the top changes, e.g. into nheight >= consensusParams.bipblahblahHeight -- the check itself would logically remain in ContextualCheckBlock.
< jtimon>
it's so hard to get negative feedback on this on a timely manner...we always need more reviewers, but specially of the type that can be less nice and more direct...
< btcdrak>
jtimon: I dont have any objection to consensus/flags.h - i only have an objection to stuffing unrelated things into units which have a specific purpose. versionbits.cpp is clearly for BIP9 logic. I would have thought this is self evident :-p :)
< jtimon>
gmaxwell: fair enough, I can do that
< jtimon>
the goal was getting issupermajority out of main, destroying it counts ;)
< gmaxwell>
well I think I've already done it, just need to find the code in a corrupted repository.
< jtimon>
gmaxwell: oh, if you've done it no need for me to do it again, please ping me on the PR
< jtimon>
well, if it takes you too long to PR it, I will rewrite it ;)
< gmaxwell>
the only thing that takes more than 10 minutes is finding the precise heights to set it.
< btcdrak>
I think we should update softfork BIPs directly with activation heights.
< btcdrak>
something like
< btcdrak>
==Activation==
< btcdrak>
This BIP was activated on mainnet at height #nnnnnn
< jtimon>
btcdrak: your feedback would be much more useful if you proposed an alternative or at least chose one of the alternatives suggested to you. Where do you think GetFlags should be ? is header_verify.cpp ok?
< gmaxwell>
in prior places where we hardcoded softforks we actually bakcdated them.
< jtimon>
it seems different people find different preferences (some times incompatible between them) "self evident" it would all be much easier if we all verbalize what is "self evident" to us but doesn't seem to be for other people
< jtimon>
btcdrak: I would like to do that encapsulated inside Consensus::GetFlags from now on
< jtimon>
gmaxwell: can we have an array in Consensus::Params with its own little struct ala vDeployments instead of having two new fields for every old deployment?
< jtimon>
say, vPastDeployments
< jtimon>
btcdrak: oh, you meant in the BIP's doc, nevermind then, agreed
< jtimon>
gmaxwell is the hash of the block really necessary here? In a super-reorg where that block is changed, shouldn't it be activated anyway?