Discussion: Consensus: There's only one type of consensus flags

jtimon commented at 4:54 pm on March 31, 2016: contributor

There should be only policy and consensus validation flags. We can separate script, locktime, tx, header and block level, or we can just wait to run out of bits to think about it. Most of the text is in the commit messages.

For more context on “unifying consensus flags”, see (on jtimon/jt): 942c2ee * Consensus: Separate GetScriptFlags() from ConnectBlock and make IsSuperMajority static 217ed51 * Consensus: Move bip34 to VerifyTx() (s/ContextualCheckBlock/CheckTxCoinbase) 51b213f * Consensus: Move BIP30 code to GetConsensusFlags and Consensus::VerifyTx a69268b * Libconsensus-p3B: expose bitcoinconsensus_verify_tx()

Consensus: There's only one type of consensus flags 7d58ce5bad

Example: Next standard flag

We don't need separate flag arguments [trust me, Consensus::VerifyBlock() will be ugly enough already
without 6 different types of flags as parameters already] for different validation levels.
Uniform naming patterns for the constants should be more than enough for this.

Fortunately, we redundantly prefixed all the consensus flags with "SCRIPT_", and "LOCKTIME_" respected the proposed naming pattern (just one in the an infinite space of bike-shedding possibilities to freely explore without universal optimization function).
That means stablishing the naming pattern VALIDATIONLEVEL_* is free in disruption terms.
I hope the pattern VALIDATIONLEVEL_<FEW_DESCRIPTIVE_WORDS_FOR_FEATURE>_* will be uncontroversial as well.
Then I conclude with the suggestion, replace the asterisk with the bip number, why not?

22a8005502

laanwj added the label Brainstorming on Apr 2, 2016

laanwj added the label Consensus on Apr 2, 2016

sdaftuar commented at 3:24 pm on April 6, 2016: member

No strong feelings, but this seems reasonable to me. (I was never sure I understood how these flags were supposed to work, so this seems clearer than before.)

utACK

jtimon commented at 11:35 pm on April 6, 2016: contributor

thank you! To be clear, I don’t think the second commit should be merged. It would be one thing we could replace with a flag for policy-only segwit or something in the future, just an example. I’ll leave it for now, but it’s not intended to be merged. Next comment I remove it.

jtimon commented at 2:43 pm on April 14, 2016: contributor

Should I remove the second commit already or just close the PR for lack of interest?

sipa commented at 2:51 pm on April 14, 2016: member

It feels like a layer violation to let the script intepreter define the flags that block-level consensus can use (including things that have no effect on script validation).

jtimon commented at 4:55 pm on April 14, 2016: contributor

@sipa Thank you for bringing that up, that’s the concern I expected. In the same way, when/if we create tx validation flags for say, tx, header, block… It will feel like a layer violation that the block validation flags are in the same enum as the tx validation flags, won’t it? So the real question is, how many different enums do we want for consensus validation flags?

To me, the answer is one. The layer we should be most concerned about is the consensus layer. Would you feel better if all the flags were moved from script/interpreter to consensus/flags.h or something similar? Although it seems the right thing to do, it doesn’t feel like useful disruption, the priority is moving consensus code out of the consensus module to the consensus module. Moving things that are already in the consensus module already doesn’t seem very helpful at this point.

Anyway, 1 or not, I need to know what the number will be. With BIP68/BIP112/BIP113 I originally suggested that we at least called them tx validation flags instead of locktime flags, but I got an answer along the lines “this kind of detailed can be bike-shed later”. I agreed and patiently waited for many months. Well, I’m trying to get an answer again now.

It won’t be hard (only painful and ugly) to separate the parameter flags in libconsensus’ VerifyTx() ( see https://github.com/jtimon/bitcoin/commit/c90f08f97a0965868dd1ff502e79f22f3ad7e787 ) into 3 different parameters: scriptFlags, lockTimeFlags and txFlags.

GetConsensusFlags() (see https://github.com/jtimon/bitcoin/commit/d3cf04f547d8d2e6eefc38858dd931a4985955f0 ) can also be easily separated in 3 or more functions (it just becomes the opposite of elegance and stops being practical).

If the first commit gets merged as it is, great. If it doesn’t, but I get an answer to my question, “how many types of consensus flags we need if 1 is not enough?”, also great. I will personally argue in favor of at few as possible. So if 1 (this PR ) is nacked, I will argue in favor of scriptFlags and nonScriptConsensusFlags as the next best thing (ie 2).

sipa commented at 2:53 pm on June 2, 2016: member

@jtimon That’s an interesting discussion, and it is related to the question of how the code should be layered a) by (script,tx,block,blockchain) or b) by (primitives,serialization,validation/consensus,signing), as all combinations of these are possible. If we primarily organize by b, then it makes sense to have only one set of consensus flags. But what if we want to extend the usage of scripts beyond transactions? (for example, signing messages with script). It would be very much be a layer violation if the message signing/validation logic needs to be aware of block-level consensus flags like BIP34.

jtimon commented at 7:34 pm on July 16, 2016: contributor

Should I close for now, until #8345 gets some review?

jtimon commented at 7:37 pm on July 16, 2016: contributor

The only short term solution I see to the “layer violation” concern I see is moving the flags outside script/interpreter, maybe to consensus/flags.h.

sipa commented at 8:04 pm on July 16, 2016: member

I think we just need two types of flags: consensus flags and script flags.

The consensus logic (above script) computes the script flags to pass down, but they’re otherwise independent.

jtimon commented at 9:16 am on July 17, 2016: contributor

But why? What’s so wrong about separating the flags out of interpreter (to say consensus/flags.h)? What’s so cool about having the flags separated (in two or more, but as said before I prefer 2 over any greater number, just prefer 1). That will mean that (at least) verifyTx will have to take two parameters: scriptFlags, restOfConsensusFlags. Why? Really, is there an advantage I am missing?

sipa commented at 10:33 am on July 17, 2016: member

No, VerifyTx would just take consensusflags. These would include things like BIP16, BIP30, BIP34, BIP65, BIP66, CSV, …

VerifyTx would translate those to the corresponding script verify flags; namely the things that are already script verification flags. Not all consensus flags translate to script verification flags (BIP30 and BIP34 do not for example); some would translate to multiple (standardness could be implemented that way).

The reason is to keep the script interpreter independent from the rest of consensus. At some point we may want to use it for other things like signed messages.

jtimon commented at 10:57 am on July 17, 2016: contributor

Ok, so we whould have consensus flags and script flags and most of them would be duplicated in both. Then we can have a translator function or something that gives you the corresponding script flags for some given consensus flags. Is that your proposed solution? Seems unnecessarily complicated, but perhaps only because I may still not be understanding the concern, so let’s make sure I got that right.

Let’s imagine we are using the interpreter for signed messages already. Let’s also imagine that we move all the existing flags (both the script and the locktime ones) to consensus/flags, without unifying them or anything. Your concern would be that we have LOCKTIME_VERIFY_SEQUENCE and LOCKTIME_MEDIAN_TIME_PAST in there because they are not used by interpreter.o? Is that really a problem? There would be no functionality in consensus/flags.h, only flags and documentation. Why is it so bad that interpreter doesn’t use all the flags in consensus/flags.h ?

sipa commented at 11:01 am on July 17, 2016: member

Yes, that is my proposed solution.

Just because both the script interpreter and the block/tx validation logic are part of consensus does not mean there can not be any clean abstraction between them. And currently, script is one of the few well-modularized parts of the code already; there is no need to break that abstraction.

jtimon commented at 11:20 am on July 17, 2016: contributor

I understand the general point, I guess I just don’t consider that what I describe in my example counts as breaking that abstraction. The script part would not be depending on any functionality, only flags, constants that may or may not use. Anyway, I’ll think more about your proposed solution, maybe code it to see what it looks like. But I have a strong feeling that I won’t like it.

jtimon commented at 12:23 pm on December 3, 2016: contributor

Replaced by #9271

jtimon closed this on Dec 3, 2016

DrahtBot locked this on Sep 8, 2021

Discussion: Consensus: There’s only one type of consensus flags #7779