Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce CBlockchain and move CheckBlockHeader #8087

Closed
wants to merge 4 commits into from

Conversation

pstratem
Copy link
Contributor

@pstratem pstratem commented May 23, 2016

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
Copy link
Contributor

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 --

https://github.com/bitcoin/bitcoin/pull/8087.patch
https://github.com/bitcoin/bitcoin/pull/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 pstratem changed the title Introduce CBlockchain and move CheckBlockHeader [WIP]Introduce CBlockchain and move CheckBlockHeader May 23, 2016
@pstratem
Copy link
Contributor Author

Indeed I did

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.
@pstratem pstratem changed the title [WIP]Introduce CBlockchain and move CheckBlockHeader Introduce CBlockchain and move CheckBlockHeader May 23, 2016
@jtimon
Copy link
Contributor

jtimon commented May 24, 2016

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.

@pstratem
Copy link
Contributor Author

@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
Copy link
Member

sipa commented May 25, 2016

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
Copy link
Contributor

dcousens commented May 26, 2016

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
Copy link
Contributor

utACK 942a847

@jtimon
Copy link
Contributor

jtimon commented May 26, 2016

@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
Copy link
Contributor

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
Copy link
Member

sipa commented Apr 9, 2017

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 sipa closed this Apr 9, 2017
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants