Introduce Consensus::GetFlags() and hide IsSuperMajority()

jtimon commented at 1:58 pm on July 16, 2016: contributor

Shares the first commit with #8337 Encapsulate all the consensus activation checks there by unifying script and locktime consensus flags.

It also introduces an incomplete Consensus::VerifyTx to be exposed once completed. I’m in the process of completing Consensus::VerifyTx() again in https://github.com/jtimon/bitcoin/commits/0.12.99-consensus (or you can look at an older branch in https://github.com/jtimon/bitcoin/commits/jt ).

If this is too much for a single PR, I can create another one with a subset of the commits to merge that first. If unifying the script and locktime flags still feels like a layer violation to @sipa (ie versionbits doesn’t need to know about script/interpreter.h), maybe moving all the flags to consensus/flags.h before or after this would help.

This introduces a circular dependency, but can be trivially solved merging #8329 before or after doing this. Ping @NicolasDorier @kanzure

MOVEONLY: IsSuperMajority() to consensus/versionbits.o bb6a3d4bf3

Refactor: Introduce Consensus::GetFlags() b793c61697

Refactor: BIP34: Introduce Consensus::VerifyTx() with BIP34

- The check is moved from main::ContextualCheckBlock() to
main::ConnectBlock(), calling Consensus::VerifyTx() from
consensus/tx_verify.cpp in its main loop.

- TX_COINBASE_VERIFY_BIP34 consensus flag is created.

- One more call to IsSuperMajority() hidden below Consensus::GetFlags() in versionbits

c0e6ca830c

Consensus: Refactor: Move BIP30 check to Consensus::VerifyTx()

- It remains being called only from ConnectBlock()

- TX_VERIFY_BIP30 consensus flag is created.

f255e7adb1

Refactor: BIP113: Move from main::ContextualCheckBlock() to Consensus::VerifyTx()

- LOCKTIME_MEDIAN_TIME_PAST is unified with the rest of consensus flags

- The flag calculations are moved to Consensus::GetFlags()

- The checks is moved from main::ContextualCheckBlock() to
main::ConnectBlock(), calling Consensus::VerifyTx() from
consensus/tx_verify.cpp in its main loop.

d41d400e9a

Consensus: Refactor: BIP68: Unify LOCKTIME_VERIFY_SEQUENCE in GetFlags() 6c6199be3c

fixup! style: shorten Consensus::GetFlags() db9f0af487

Introduce VerifyBlockVersion() to make IsSuperMajority() static

This way IsSuperMajority() is completely hidden in versionbits.

8c893cfcb9

MarcoFalke added the label Refactoring on Jul 16, 2016

btcdrak commented at 2:18 pm on July 16, 2016: contributor

Why are you putting ISM code in versiobits.cpp?

NicolasDorier commented at 2:30 pm on July 16, 2016: contributor

I would prefer you break in smaller PR. I like some commits, and not other, I’m using less disruptive code on #8339. But I’d like to introduce them in smaller PR as well.

Our work is overlapping, so I think it is important to just merge what is common to both of us, and for commits which are in disagreement that we propose the competitive PR and ask reviews.

jtimon commented at 5:00 pm on July 16, 2016: contributor

@btcdrak To hide all consensus rule changes activation code in one place and also create GetFlags() as a function candidate to be exposed in libconsensus alongside VerifyScript(). Another option would be to put that code in a deployments module that depends on versionbits similar to what @CodeShark attempted in the past. Yet another option would be to put it in header_verify.cpp (see #8337 ). But if we want to expose GetFlags() in libconsensus, it simply can’t stay in main.cpp. I’m happy to hear other suggestions. @NicolasDorier Sounds like a plan. I’m also happy to put together a PR with the commits we both like so that we can have that as a more stable common base. But for doing that I need to know which commits do you like. It would be also helpful for me to know what you don’t like about the ones you don’t like, since that may allow me to fix them. Or maybe I can explain the motivation for the things you don’t like.

NicolasDorier commented at 2:33 am on July 17, 2016: contributor

I think the MOVEs should be done. (not limited to this PR) Agree with b793c61697e0afef9ee3f203e4207b7e78f12f28 but it is not complete. For example nLockTimeFlags |= LOCKTIME_VERIFY_SEQUENCE; is still outside. (maybe fixed in later commits, but I think it should be done in one)

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

I think the MOVEs should be done. (not limited to this PR) Agreed, but since people were asking to do things before moving things…

Agree with b793c61 but it is not complete.

Agreed, that’s done in https://github.com/bitcoin/bitcoin/pull/8345/commits/6c6199be3cf9c912f0c9f861c290719414552d6d

jtimon commented at 2:18 pm on July 21, 2016: contributor

Like in #8344 (see #8344 (comment) ), many parts of this PR become useless if we’re going to remove ISM soon. So I’m closing it. That being said, both positive and negative feedback are still very welcomed.

Particularly about the parts that move code from ContextualCheckBlock to Consensus::VerifyTx (one of which, by the way, is what’s making qa/rpc-tests/bip68-112-113-p2p.py fail with this branch):

https://github.com/bitcoin/bitcoin/pull/8345/commits/c0e6ca830c54fe6ebb6551d068633b13258062c7 https://github.com/bitcoin/bitcoin/pull/8345/commits/d41d400e9a803372b402a054e7f4508f87994c0a

Note that those movements are not necessary for completing verifyBlock, which will call ContextualCheckBlock anyway, but to complete VerifyTx. One could argue that VerifyTx doesn’t need to do coinbase checks (ie bip34), for example.

jtimon closed this on Jul 21, 2016

MarcoFalke locked this on Sep 8, 2021

Introduce Consensus::GetFlags() and hide IsSuperMajority() #8345