Skip to content

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Apr 14, 2016

Part of #7829.

@laanwj
Copy link
Member

laanwj commented Apr 14, 2016

utACK 145ae6e

If there are other functions where you can easily pass in CChainParams& with low impact to the rest of the code (e.g. like #7828), I'd prefer if you combine them into this as multiple commits instead of open separate pulls.

@instagibbs
Copy link
Member

utACK 145ae6e

@jtimon
Copy link
Contributor Author

jtimon commented Apr 14, 2016

@laanwj well, the idea of #7829 is helping new people doing them. I did #7828 as a "canonical example" (in fact, I wanted to pass CPolicy at the same time, but ProcessMessage() has waited enough for CPolicy to exist...) and this one because it seems slightly more confusing (or could confuse someone trying to do it for DisconnectTip()). I wouldn't mind doing DisconnectTip() in this PR for that reason.

Probably the less disruptive remaining ones are ConnectBlock(), CheckBlockHeader() and ContextualCheckBlockHeader() [I didn't put ContextualCheckBlock() because @morcos and I want to just destroy the function]. But these seem good exercises for #7829.

Besides that, I must have tried to do CheckBlockHeader() like in 10 different PRs already (probably 5 of them did it before and 5 of them after the cursed consensus moveonly), but I've never tried to get someone else to do it. Perhaps this breaks the curse or something.

@laanwj
Copy link
Member

laanwj commented Apr 15, 2016

@jtimon Well sure it helps reviewing to have small pull requests, but if it's just adding an argument to a function and passing it on that's hardly a risky refactor, nor controversial in any way.

I wouldn't mind doing DisconnectTip() in this PR for that reason

ok Let's do that.

@jtimon
Copy link
Contributor Author

jtimon commented Apr 20, 2016

Closing in favor of #7916 , which contains this same commit plus another one doing it for DisconnectTip() and InvalidateBlock() too.

@jtimon jtimon closed this Apr 20, 2016
@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.

3 participants