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

rpc: Remove the need to include rpc/blockchain.cpp in order to put `GetDifficulty` under test #13288

Merged
merged 1 commit into from Jun 5, 2018

Conversation

Projects
None yet
6 participants
@Empact
Copy link
Member

commented May 20, 2018

By dropping the chain argument to GetDifficulty. GetDifficulty was called in two ways:

  • with a guaranteed non-null blockindex
  • with no argument

Change the latter case to be provided chainActive.Tip() explicitly.

Introduced in: #11748

@Empact

This comment has been minimized.

Copy link
Member Author

commented May 20, 2018

This conflicts with #13230. Will rebase when that goes in.

@Empact Empact changed the title Remove the need to include "rpc/blockchain.cpp' in order to put `GetDifficulty` under test Remove the need to include rpc/blockchain.cpp in order to put `GetDifficulty` under test May 20, 2018

@Empact Empact changed the title Remove the need to include rpc/blockchain.cpp in order to put `GetDifficulty` under test refactoring: Remove the need to include rpc/blockchain.cpp in order to put `GetDifficulty` under test May 20, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented May 20, 2018

/cc #11748

Drop the chain argument to GetDifficulty
This removes the need to include rpc/blockchain.cpp in order to put
GetDifficulty under test. GetDifficulty was called in two ways:
* with a guaranteed non-null blockindex
* with no argument

Change the latter case to be provided chainActive.Tip() explicitly.

@Empact Empact force-pushed the Empact:get-difficulty branch to ebec731 May 21, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented May 21, 2018

Dropped the now-unused chain.h include as well.

@practicalswift

This comment has been minimized.

Copy link
Member

commented May 21, 2018

Concept ACK

What about adding this to lint-includes.sh?

diff --git a/contrib/devtools/lint-includes.sh b/contrib/devtools/lint-includes.sh
index 0f9fec4..97b037f 100755
--- a/contrib/devtools/lint-includes.sh
+++ b/contrib/devtools/lint-includes.sh
@@ -40,4 +40,12 @@ if [[ ${QUOTE_SYNTAX_INCLUDES} != "" ]]; then
     EXIT_CODE=1
 fi

+INCLUDED_CPP_FILES=$(git grep -E "^#include <[^>]*\.cpp>" -- "**/*.cpp" "**/*.h")
+if [[ ${INCLUDED_CPP_FILES} != "" ]]; then
+    echo "The following files #include .cpp files:"
+    echo "${INCLUDED_CPP_FILES}"
+    echo
+    EXIT_CODE=1
+fi
+
 exit ${EXIT_CODE}
@Empact

This comment has been minimized.

Copy link
Member Author

commented May 21, 2018

@practicalswift I'm in favor - we'd need to merge this and #13291 to get something like that to pass, so how about as a follow-up?

@MarcoFalke
Copy link
Member

left a comment

utACK ebec731

@@ -352,7 +342,7 @@ static UniValue getdifficulty(const JSONRPCRequest& request)
);

LOCK(cs_main);
return GetDifficulty();
return GetDifficulty(chainActive.Tip());

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 21, 2018

Member

style nit: Personally I prefer ::chainActive for globals.

This comment has been minimized.

Copy link
@Empact

Empact May 22, 2018

Author Member

I like this too but I'm going to leave this PR be. I have a follow-up I think will accommodate it better.

@kallewoof
Copy link
Member

left a comment

utACK ebec731

@practicalswift

This comment has been minimized.

Copy link
Member

commented May 24, 2018

utACK ebec731

@MarcoFalke MarcoFalke changed the title refactoring: Remove the need to include rpc/blockchain.cpp in order to put `GetDifficulty` under test rpc: Remove the need to include rpc/blockchain.cpp in order to put `GetDifficulty` under test May 24, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

utACK ebec731

@laanwj laanwj merged commit ebec731 into bitcoin:master Jun 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 5, 2018

Merge #13288: rpc: Remove the need to include rpc/blockchain.cpp in o…
…rder to put `GetDifficulty` under test

ebec731 Drop the chain argument to GetDifficulty (Ben Woosley)

Pull request description:

  By dropping the chain argument to `GetDifficulty`. `GetDifficulty` was called in two ways:
  * with a guaranteed non-null blockindex
  * with no argument

  Change the latter case to be provided `chainActive.Tip()` explicitly.

  Introduced in: #11748

Tree-SHA512: f2c97014be185f3e3de92db15848548650e4a67fab20a41bcfa851c5c63c245915cbe9380f84d9da2081e8756d31a41de417db1d35cfecf41ddb4f25070eb525

@Empact Empact deleted the Empact:get-difficulty branch Jun 6, 2018

fanquake added a commit to fanquake/bitcoin that referenced this pull request Sep 3, 2018

laanwj added a commit that referenced this pull request Sep 4, 2018

Merge #14135: doc: correct GetDifficulty doc after #13288
68bfc0b doc: correct GetDifficulty doc after #13288 (fanquake)

Pull request description:

  `chain` is no longer passed to GetDifficulty, and we just return `1.0` if no `blockindex`.

Tree-SHA512: 701375d732f343200c4abfaf9039d5c12b10abff97b022e84564f81b26b5ba552f1eb0c0d0fd5370b29b53319eafcf39773a36e1c2dd04ee77e61c18c7b183fa

HashUnlimited added a commit to ToDoThings/bitcoin that referenced this pull request Sep 10, 2018

jfhk added a commit to jfhk/bitcoin that referenced this pull request Nov 14, 2018

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 26, 2018

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.