Skip to content

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Mar 27, 2017

GetDifficulty has no business in rpcserver.h. Define it in the interface header of the implementation unit rpcblockchain where it is defined.

Also modernize the signature to:

double GetDifficulty(const CBlockIndex* blockindex = nullptr);

(remove extern, replace NULL with nullptr)

It has no business in `rpcserver.h`. Define it in the interface header
of the implementation unit `rpcblockchain` where it is defined.

Also modernize the signature to:

    double GetDifficulty(const CBlockIndex* blockindex = nullptr);

(remove `extern`, replace `NULL` with `nullptr`)
@jnewbery
Copy link
Contributor

Tested ACK e6dcfee

Can RPCNotifyBlockChange() (https://github.com/bitcoin/bitcoin/pull/10095/files#diff-a0fb7350bf105d107147e0d10f727f47L202) also be declared in rpc/blockchain.h ?

@jonasschnelli
Copy link
Contributor

utACK e6dcfee

@laanwj
Copy link
Member Author

laanwj commented Mar 27, 2017

@jnewbery Done

@laanwj
Copy link
Member Author

laanwj commented Mar 27, 2017

I see rest.cpp also uses a few functions from this file:

extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry);
extern UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool txDetails = false);
extern UniValue mempoolInfoToJSON();
extern UniValue mempoolToJSON(bool fVerbose = false);
extern void ScriptPubKeyToJSON(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex);
extern UniValue blockheaderToJSON(const CBlockIndex* blockindex);

Going to add them to the header as well.

@laanwj laanwj force-pushed the 2017_03_getdifficulty_header branch from de4b7ac to c7e9aa7 Compare March 27, 2017 14:23
@jnewbery
Copy link
Contributor

Looks good to me. Tested ACK c7e9aa7c1a92adba141958e5f86d267f34cc9508

@jtimon
Copy link
Contributor

jtimon commented Mar 28, 2017

ScriptPubKeyToJSON and TxToJSON are defined in https://github.com/bitcoin/bitcoin/blob/master/src/rpc/rawtransaction.cpp#L37 not rpc/blockchain.cpp. Either move the declaration or the definition for both to be in the same .h and .cpp

utACK c7e9aa7c1a92adba141958e5f86d267f34cc9508 modulo nit.

@laanwj
Copy link
Member Author

laanwj commented Mar 28, 2017

@jtimon Good catch. I'll move them to their own header.

@jnewbery
Copy link
Contributor

Or just refactor them out entirely since they're duplicate code: #8824

@laanwj
Copy link
Member Author

laanwj commented Mar 28, 2017 via email

@laanwj laanwj force-pushed the 2017_03_getdifficulty_header branch from c7e9aa7 to f885b67 Compare March 29, 2017 08:01
@laanwj
Copy link
Member Author

laanwj commented Mar 29, 2017

Done, now those two lines remain in rest.cpp, will leave it to another pull to deal with them (I did remove the redundant extern though).

@laanwj laanwj merged commit f885b67 into bitcoin:master Mar 31, 2017
laanwj added a commit that referenced this pull request Mar 31, 2017
f885b67 refactor: Make rest.cpp dependency on `*toJSON` in `blockchain.cpp` explicit (Wladimir J. van der Laan)
8d8f28d refactor: Move RPCNotifyBlockChange out of `rpc/server.h` (Wladimir J. van der Laan)
e6dcfee refactor: Move GetDifficulty out of `rpc/server.h` (Wladimir J. van der Laan)

Tree-SHA512: fc2656611d18442f2fddba5ac1554d958151f6785c2039afdfc36735d7e71592d9686ff6cc7b2ad95180071d7514470e62c52d697c5a1e88f851bddaf5942edb
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request May 20, 2019
f885b67 refactor: Make rest.cpp dependency on `*toJSON` in `blockchain.cpp` explicit (Wladimir J. van der Laan)
8d8f28d refactor: Move RPCNotifyBlockChange out of `rpc/server.h` (Wladimir J. van der Laan)
e6dcfee refactor: Move GetDifficulty out of `rpc/server.h` (Wladimir J. van der Laan)

Tree-SHA512: fc2656611d18442f2fddba5ac1554d958151f6785c2039afdfc36735d7e71592d9686ff6cc7b2ad95180071d7514470e62c52d697c5a1e88f851bddaf5942edb
@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.

5 participants