Introduce CBlockchain and move CheckBlockHeader #8087

pstratem commented at 7:23 AM on May 23, 2016: contributor

The ultimate goal here is to move all of the consensus functions defined in main.cpp to CBlockchain.

Initially they will all simply be static functions to ensure that the changes are obviously correct.

TheBlueMatt commented at 7:34 AM on May 23, 2016: member

I think you forgot to include the files in git.

On May 23, 2016 12:23:38 AM PDT, Patrick Strateman notifications@github.com wrote:

You can view, comment on, or merge this pull request online at:

#8087

-- Commit Summary --

Introduce CBlockchain and move CheckBlockHeader

-- File Changes --

M src/Makefile.am (5) M src/main.cpp (18) M src/main.h (1)

-- Patch Links --

#8087.patch #8087.diff

You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #8087

pstratem renamed this:
~~Introduce CBlockchain and move CheckBlockHeader~~
[WIP]Introduce CBlockchain and move CheckBlockHeader
on May 23, 2016

pstratem commented at 7:38 AM on May 23, 2016: contributor

Indeed I did

pstratem force-pushed on May 23, 2016

Introduce new class CBlockchain

High level consensus logic is currently in  main.cpp
In time these functions should be moved to CBlockchain
Initially as static methods and then as normal methods.

201e4837b7

Move CheckBlockHeader to CBlockchain::CheckBlockHeader 88d3cf54df

pstratem force-pushed on May 23, 2016

move CheckTransaction to CBlockchain::CheckTransaction 942a84727b

pstratem renamed this:
~~[WIP]Introduce CBlockchain and move CheckBlockHeader~~
Introduce CBlockchain and move CheckBlockHeader
on May 23, 2016

jonasschnelli added the label Refactoring on May 23, 2016

jtimon commented at 1:27 PM on May 24, 2016: contributor

This is a very different approach to libconsensus. I already see chainparams.h, checkpoints and other dependencies that shouldn't be in the consensus module (and libconsensus). This seems more disruptive to my work on libconsensus than segwit (for which I'm waiting to continue proposing more consensus related refactors). Concept NACK.

Introduce pure CBlockchain::CheckBlockHeader method. 6687fdd8fb

pstratem commented at 3:09 PM on May 24, 2016: contributor

@jtimon I think there was a misunderstanding in the intention here.

My goal is simply to more clearly define the interface functions in an effort to separate networking and consensus logic.

The implementing functions should be pure while the interface used by bitcoind/bitcoin-qt would have some state, such as the block index and utxo states as well as which chain we're dealing with (ie ConsensusParams)

sipa commented at 1:53 PM on May 25, 2016: member

I think we need to first have a plan about refactorings in this space. There are too many people with conflicting goals pulling in different directions. Without coordination, I fear we'll end up with partially-completed refactors that are worse than what we've started off with.

dcousens commented at 2:58 AM on May 26, 2016: contributor

Without coordination, I fear we'll end up with partially-completed refactors that are worse than what we've started off with.

Agreed, perhaps this is an article for in-depth technical discussion after the next meeting? Might need a mediator...

dcousens commented at 2:59 AM on May 26, 2016: contributor

utACK 942a847

jtimon commented at 7:18 AM on May 26, 2016: contributor

@pstratem Encapsulating consensus logic has been a long term goal of mine. I believe you are underestimating how disruptive this PR would be to my previous (but still not reviewed or merged) work in this subject. Let's please take little steps trying to find common ground. I never created the promised "libconsensus plan" document with pictures, but people really really interested have always had the chance to look at my longest branches (thus I assumed nobody was really as interested as they claimed...). Please, feel free to comment on any of my multiple "consensus" closed PRs.

TheBlueMatt commented at 5:15 PM on October 28, 2016: member

Needs rebase. Also see related work such as #8969 which attempts to move towards this in a different way - pull everything that isnt CBlockchain out of main.cpp first, then just put everything left in a class.

sipa commented at 2:43 PM on April 9, 2017: member

Closing. I think this needs significant rework after #8969 and #9260. Feel free to ask to reopen if you want to pick this up.

sipa closed this on Apr 9, 2017

MarcoFalke locked this on Sep 8, 2021