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

Bitcoin Core refactoring and cleanups 1 #2912

Merged
merged 22 commits into from May 28, 2019

Conversation

@str4d
Copy link
Contributor

commented Jan 27, 2018

Cherry-picked from the following upstream PRs:

Additionally, the Equihash parameters are moved into the consensus parameters, and
various other functions in main.cpp have const CChainParams& arguments added.

Part of #2074.

@str4d str4d added this to the 1.1.0 milestone Jan 27, 2018

@str4d str4d self-assigned this Jan 27, 2018

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2018

Came across these as I was working on #2905, but it turned out that the merge conflict this resolved was very minor relative to the size of this PR, so I'm keeping it separate.

@str4d str4d force-pushed the str4d:2074-main-refactor-1 branch 2 times, most recently from c4a99bf to 4f3d624 Jan 27, 2018

@str4d str4d added this to 1.1.0: Upstream Improvements in Release planning Feb 3, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 7, 2018

☔️ The latest upstream changes (presumably #2898) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d str4d force-pushed the str4d:2074-main-refactor-1 branch from 4f3d624 to 40ed794 Mar 9, 2018

@str4d str4d requested review from charlieok, bitcartel and mdr0id Mar 9, 2018

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

Rebased on master to fix merge conflicts. I also pulled in bitcoin/bitcoin#6986 which I'd previously missed, which required pulling in bitcoin/bitcoin#5994 (taking the commits I'd already cherry-picked in #2225).

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2018

⌛️ Trying commit 40ed794 with merge 6dcc7dd...

zkbot added a commit that referenced this pull request Mar 9, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2018

💔 Test failed - pr-try

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

Most of the Zcash-specific RPC tests failed because they assume that the first getnewaddress call will return the address the miner is using, but after bitcoin/bitcoin#5994 that no longer appears to be the case. I'm trying to figure out what exactly caused the change in behaviour, but the end result appears to be intentional, so I'll adjust the RPC tests to grab the addresses from the mined coinbase instead.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 10, 2018

⌛️ Trying commit d67af6d with merge b094b02...

zkbot added a commit that referenced this pull request Mar 10, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 10, 2018

💔 Test failed - pr-try

@str4d str4d force-pushed the str4d:2074-main-refactor-1 branch from d67af6d to 5d7dbf1 Mar 12, 2018

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

Missed a more complex coinbase case in wallet_shieldcoinbase, and refactored my commit to handle it better.

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2018

⌛️ Trying commit 5d7dbf1 with merge 59ef0e4...

zkbot added a commit that referenced this pull request Mar 12, 2018

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5994
- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters.

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 12, 2018

💔 Test failed - pr-try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

⌛️ Trying commit e12d018 with merge fcf551e...

zkbot added a commit that referenced this pull request May 24, 2019

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters, and
various other functions in `main.cpp` have `const CChainParams&` arguments added.

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

💔 Test failed - pr-try

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Kitchen sink builder failed with --enable-proton, but that works for me locally, so I'm hoping this was a transient failure.

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

⌛️ Trying commit e12d018 with merge e25cb39...

zkbot added a commit that referenced this pull request May 24, 2019

Auto merge of #2912 - str4d:2074-main-refactor-1, r=<try>
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters, and
various other functions in `main.cpp` have `const CChainParams&` arguments added.

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

☀️ Test successful - pr-try
State: approved= try=True

@LarryRuane
Copy link
Contributor

left a comment

ACK (the one suggestion is nonblocking). I ran qa/zcash/full_test_suite.py and all tests pass. Many if not most of the changes involved adding an argument to functions, so I didn't review all the changes to the callers carefully, since the compiler would tell us if anything had been missed. I compared the more complicated commits with the corresponding bitcoin commits, and they all look good (the same except where zcash requires them to be different). Overall, a nice cleanup.

@@ -1868,7 +1868,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)

#ifdef ENABLE_MINING
// Generate coins in the background
GenerateBitcoins(GetBoolArg("-gen", false), GetArg("-genproclimit", 1), Params());
GenerateBitcoins(GetBoolArg("-gen", false), GetArg("-genproclimit", 1), chainparams);

This comment has been minimized.

Copy link
@LarryRuane

LarryRuane May 24, 2019

Contributor

There are two other places within this function where you could make a similar change, lines 1123 and 1865.

This comment has been minimized.

Copy link
@str4d

str4d May 28, 2019

Author Contributor

Mmm. There are still a bunch more places that need refactoring in this way, so in the interest of PR size, let's leave that to 2074-main-refactor-2 😄

UpdateTime(pblock, Params().GetConsensus(), pindexPrev);
pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, Params().GetConsensus());
UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);
pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus());

This comment has been minimized.

Copy link
@LarryRuane

LarryRuane May 24, 2019

Contributor

I did verify that you replaced all the calls to Params() in this function with chainparams -- looks good.

@str4d str4d requested review from Eirik0 and removed request for bitcartel and mdr0id May 28, 2019

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

I see above that @Eirik0 thumbed-up the changes I made after his review, and since then the only changes to this PR have been fixing merge conflicts. Any further comments can be addressed in subsequent PRs.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

📌 Commit e12d018 has been approved by str4d

@str4d str4d added this to the v2.0.6 milestone May 28, 2019

@str4d str4d moved this from In Review to Merge Queue in Arborist Team May 28, 2019

zkbot added a commit that referenced this pull request May 28, 2019

Auto merge of #2912 - str4d:2074-main-refactor-1, r=str4d
Bitcoin Core refactoring and cleanups 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6538
- bitcoin/bitcoin#6163
- bitcoin/bitcoin#6982
- bitcoin/bitcoin#6986
- bitcoin/bitcoin#7053
- bitcoin/bitcoin#7444
- bitcoin/bitcoin#7793
  - Excluding some comments in `txmempool.h` on code we haven't yet pulled in.
- bitcoin/bitcoin#7916
- bitcoin/bitcoin#7815

Additionally, the Equihash parameters are moved into the consensus parameters, and
various other functions in `main.cpp` have `const CChainParams&` arguments added.

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

⌛️ Testing commit e12d018 with merge 3d37ebe...

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 3d37ebe to master...

@Eirik0
Copy link
Contributor

left a comment

Late reACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.