Replace Connman and BanMan globals with NodeContext local

ryanofsky commented at 9:32 pm on September 9, 2019: member

This change is mainly a naming / organization change intended to simplify #10102. It:

Renames struct InitInterfaces to struct NodeContext and moves it from src/init.h to src/node/context.h. This is a cosmetic change intended to make the point of the struct more obvious.
Gets rid of BanMan and ConnMan globals making them NodeContext members instead. Getting rid of these globals has been talked about in past as a way to implement testing and simulations. Making them NodeContext members is a way of keeping them accessible without the globals.
Splits g_rpc_interfaces global into g_rpc_node and g_rpc_chain globals. This better separates node and wallet rpc methods. Node RPC methods should have access NodeContext, while wallet RPC methods should only have indirect access to node functionality via interfaces::Chain.
Adds NodeContext& references to interfaces::Chain class and the interfaces::MakeChain() function. This is needed to access ConnMan and BanMan instances without the globals.
Gets rid of redundant Node and Chain instances in Qt tests. This is needed due to the previous MakeChain change, and also makes test setup a little more straightforward. More cleanup could be done in the future, but it will require deduplication of bitcoind, bitcoin-qt, and TestingSetup init code.

ryanofsky commented at 9:36 pm on September 9, 2019: member

I’m looking for concept ACKS before detailed review at the moment. Since this a naming and refactoring change, it’d be great if people could take few minutes and skim the diff, and say whether the new code looks ugly or ok, and offer any suggestions. I’m happy to take suggestions and also split this up more.

DrahtBot added the label Build system on Sep 9, 2019

DrahtBot added the label GUI on Sep 9, 2019

DrahtBot added the label Mining on Sep 9, 2019

DrahtBot added the label P2P on Sep 9, 2019

DrahtBot added the label RPC/REST/ZMQ on Sep 9, 2019

DrahtBot added the label Tests on Sep 9, 2019

DrahtBot added the label Wallet on Sep 9, 2019

fanquake added the label Needs Conceptual Review on Sep 9, 2019

in src/interfaces/chain.cpp:387 in 124bbcabe3 outdated

376@@ -375,9 +377,16 @@ class ChainImpl : public Chain
377             notifications.TransactionAddedToMempool(entry.GetSharedTx());
378         }
379     }
380+    std::unique_ptr<Handler> addClient(ChainClient& client) override
381+    {
382+        ChainClients& clients = m_node.chain_clients;
383+        ChainClients::iterator it = clients.emplace(clients.end(), client);
384+        return MakeHandler([&clients, it] { clients.erase(it); });

promag commented at 0:03 am on September 10, 2019:

Is it better to capture member chain_clients than this?

Also, it would be nice to explain that a std::list is used otherwise this .erase would fail.

ryanofsky commented at 3:28 pm on September 10, 2019:

Is it better to capture member chain_clients than this?

I think a good starting point is to just capture what’s needed without unused data or object references. But in this case, capturing this instead of &clients is worse because in addition to capturing unneeded data, it requires the Handler object to be destroyed before the Chain object, and there’s no reason to create that kind of error-prone dependency.

Also, it would be nice to explain that a std::list is used otherwise this .erase would fail.

Sure, added comment.

promag commented at 0:05 am on September 10, 2019: member

Concept ACK.

fanquake added this to the "Chasing Concept ACK" column in a project

fanquake removed the label Build system on Sep 10, 2019

fanquake removed the label Mining on Sep 10, 2019

fanquake added the label Refactoring on Sep 10, 2019

fanquake removed the label RPC/REST/ZMQ on Sep 10, 2019

DrahtBot commented at 2:30 am on September 10, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
#17167 (Allow whitelisting outgoing connections by luke-jr)
#16895 (External signer multisig support by Sjors)
#16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#16442 (Serve BIP 157 compact filters by jimpo)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15454 (Remove the automatic creation and loading of the default wallet by achow101)
#12674 (RPC: Support addnode onetry without making the connection priviliged by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Sep 10, 2019

practicalswift commented at 11:00 am on September 10, 2019: contributor

Concept ACK

MarcoFalke removed the label GUI on Sep 10, 2019

MarcoFalke removed the label P2P on Sep 10, 2019

MarcoFalke removed the label Tests on Sep 10, 2019

MarcoFalke removed the label Wallet on Sep 10, 2019

MarcoFalke commented at 1:43 pm on September 10, 2019: member

Getting rid of these globals has been talked about in past as a way to implement testing and simulations.

Fine with me, but this seems only like a first step, since other globals such as ::mempool are still global. and not members of the Node class.

ryanofsky force-pushed on Sep 10, 2019

ryanofsky commented at 4:19 pm on September 10, 2019: member

Thanks for feedback!

Rebased 124bbcabe314c2e4a48ad61a69c03ee55e633e37 -> 7225ebebdfeac3d365c1490f1b737403488005d7 (pr/noglob.3 -> pr/noglob.4) due to conflict with #16787. Also added requested comment and no-wallet build fix.

re: #16839 (comment)

Fine with me, but this seems only like a first step, since other globals such as ::mempool are still global. and not members of the Node class.

Yes, it’s not all or nothing. It’s perfectly fine for ::mempool to remain a global, but if we want to write multiple-instance tests for code currently using ::mempool, we would have the option of moving it to the Node struct and still accessing it conveniently.

DrahtBot removed the label Needs rebase on Sep 10, 2019

ryanofsky force-pushed on Sep 10, 2019

jamesob commented at 5:12 pm on September 11, 2019: member

Concept ACK. This looks like a step in the right direction in terms of cutting down on globals and moving towards being able to test multi-node behavior without having to rely on the functional tests.

Is your intent to split this into separate commits at some point for detailed review?

ryanofsky commented at 5:34 pm on September 11, 2019: member

Is your intent to split this into separate commits at some point for detailed review?

Could go either way. These are basically boring text replacements that should be about as much effort to review split up or joined, but I’m happy to split it up. I think I could split it up into 4 or 5 commits roughly matching the bullet points in the description. Overall diff would be a little bigger because some lines (chain_clients lines) would change more than once.

jamesob commented at 5:38 pm on September 11, 2019: member

Could go either way.

I’m fine reviewing either, though I guess if it were easy to do some scripted-diffs that would be nice.

in src/interfaces/node.h:260 in a3dc53304a outdated

254@@ -254,6 +255,10 @@ class Node
255     using NotifyHeaderTipFn =
256         std::function<void(bool initial_download, int height, int64_t block_time, double verification_progress)>;
257     virtual std::unique_ptr<Handler> handleNotifyHeaderTip(NotifyHeaderTipFn fn) = 0;
258+
259+    //! Return pointer to internal chain interface, useful for testing.
260+    //! Only works if node interface is being called in same process as implementation.

ariard commented at 2:10 pm on September 14, 2019:

I find comment fuzzy, do you mean only works if node interface is set by the process of the implementation ?

ryanofsky commented at 11:36 pm on September 17, 2019:

re: #16839 (review)

I find comment fuzzy, do you mean only works if node interface is set by the process of the implementation ?

Sorry, this was pretty confusing. Better not to refer to processes in comments before #10102, so I removed this. But to answer the question: this always returns non-null currently and after #10102 will return null if the node is a remote process.

in src/node/node.h:22 in a3dc53304a outdated

18+using ChainClients = std::list<std::reference_wrapper<interfaces::ChainClient>>;
19+
20+//! Node struct containing chain state and connection state.
21+//!
22+//! More state could be moved into this struct to eliminate global variables,
23+//! and allow creating multiple instances of validation and networking objects

ariard commented at 2:18 pm on September 14, 2019:

So you would create multiple Node in one test process, connect them and let them talk maybe with random introduction of interferences in-between ?

ryanofsky commented at 11:36 pm on September 17, 2019:

re: #16839 (review)

So you would create multiple Node in one test process, connect them and let them talk maybe with random introduction of interferences in-between ?

With caveats, yes, since code that isn’t referring to global variables can be instantiated multiple times for testing in a single process.

in src/node/transaction.cpp:17 in a3dc53304a outdated

13@@ -14,12 +14,12 @@
14 
15 #include <future>
16 
17-TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback)
18+TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, CConnman* connman)

ariard commented at 2:21 pm on September 14, 2019:

Couldn’t BroadcastTransaction be directly a method of Node instead of passing CConnman ? Maybe sendrawtransaction should be given g_rpc_chain instead of g_rpc_node, overall it’s seems to use the chain as a server too

ryanofsky commented at 11:37 pm on September 17, 2019:

re: #16839 (review)

Couldn’t BroadcastTransaction be directly a method of Node instead of passing CConnman ?

Yes, but I don’t think that would be a good idea. I think interface classes should be a thin layer on top of wallet and node code and not a place where real functionality is implemented. The model is:

Maybe sendrawtransaction should be given g_rpc_chain instead of g_rpc_node, overall it’s seems to use the chain as a server too

The Chain interface is meant to be used by non-node code to get limited access to chain state. It’s not really meant to be used by code inside the node to access its own state. Using direct access to node state some places and indirect access other places I think would just make the code more confusing and inconsistent.

Also using g_rpc_chain inside the node would lead to a link error with --disable-wallet since g_rpc_chain is a wallet variable defined and used in rpcwallet.cpp.

MarcoFalke commented at 6:19 pm on September 18, 2019:

Yes, but I don’t think that would be a good idea. I think interface classes should be a thin layer on top of wallet and node code and not a place where real functionality is implemented. The model is:

I don’t understand this answer. src/node/transaction.cpp is “node code” as I understand it and not merely some interface. All the other global state accessed in this function (chainstate, mempool) should eventually be a node member. So the suggestion by @ariard to make this a node member function makes sense to me.

ryanofsky commented at 6:30 pm on September 18, 2019:

re: #16839 (review)

I don’t understand this answer. src/node/transaction.cpp is “node code” as I understand it and not merely some interface. All the other global state accessed in this function (chainstate, mempool) should eventually be a node member. So the suggestion by @ariard to make this a node member function makes sense to me.

I don’t think any code that isn’t broadcasting transactions should need to see or #include the BroadcastTransaction function declaration. But I’m not sure in general what benefits you and Antoine see in the suggestion, so maybe you could clarify. To me it just looks like making code more monolithic, less separable, and harder to use and test, like asking for main.cpp again.

MarcoFalke commented at 7:11 pm on September 18, 2019:

I guess I’d prefer to pass in the node here, so that in the future when mempool and chainstate are members of the node struct, this doesn’t need to change again.

0TransactionError BroadcastTransaction(const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback, Node& node)

ryanofsky commented at 8:02 pm on September 18, 2019:

re: #16839 (review)

I guess I’d prefer to pass in the node here, so that in the future when mempool and chainstate are members of the node struct, this doesn’t need to change again.

Sure, I made the suggested change. I do think in the future it would be better if a function like BroadcastTransaction took separate CChainState& and CConnman* arguments, (especially since the CConnman* argument could be optional and replace the bool relay argument). Passing separate arguments would make the function easier to call without needing to fill a struct, and without giving it access to unneed state. But using Node& does have a temporary advantage of avoiding the need to update every caller when the implementation changes, so it’s a reasonable suggestion.

ariard commented at 0:42 am on September 19, 2019:

Thanks got your point. Was doing far out-of-scope thinking, sendrawtransaction to your local node isn’t great for privacy, it could be a binary to connect to any node to broadcast instead of a RPC call. But yes, in that case it wouldn’t even use any interface..

in src/rpc/blockchain.h:51 in a3dc53304a outdated

46@@ -46,4 +47,9 @@ UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex
47 /** Used by getblockstats to get feerates at different percentiles by weight  */
48 void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES], std::vector<std::pair<CAmount, int64_t>>& scores, int64_t total_weight);
49 
50+//! Pointer to node state that needs to be declared as a global to be accessible
51+//! RPC methods. Due to limitations of the RPC framework, there's currently no

ariard commented at 2:25 pm on September 14, 2019:

How to override RPC framework limitations ? Encapsulating RPC functions in its own structure like it has been done for the P2P stack with CConnman ?

ryanofsky commented at 11:37 pm on September 17, 2019:

re: #16839 (review)

How to override RPC framework limitations ? Encapsulating RPC functions in its own structure like it has been done for the P2P stack with CConnman ?

Yes, that’s one way. Another option would be to add a std::any member to the JSONRPCRequest struct, so arbitrary context could be passed into RPC methods in a type-safe way.

in src/rpc/mining.cpp:428 in a3dc53304a outdated

424@@ -424,10 +425,10 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
425     if (strMode != "template")
426         throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid mode");
427 
428-    if(!g_connman)
429+    if(!g_rpc_node->connman)

ariard commented at 2:32 pm on September 14, 2019:

IMO not sure why getblocktemplate need to access state of P2P stack ? I would guess the functionality would be useless without a mempool to populate the template and being connected increase the likeliness of a fresh mempool but I could also provide mempool.dat to my node… If you remove it, you can also move g_rpc_node declaration in src/rpc/net.h.

ryanofsky commented at 11:37 pm on September 17, 2019:

re: #16839 (review)

IMO not sure why getblocktemplate need to access state of P2P stack ? I would guess the functionality would be useless without a mempool to populate the template and being connected increase the likeliness of a fresh mempool but I could also provide mempool.dat to my node… If you remove it, you can also move g_rpc_node declaration in src/rpc/net.h.

I think you’re right, and these checks probably don’t accomplish much, though maybe they are useful as precautions. But I think if we were going to change this behavior, it’d make more sense in a standalone PR.

ariard commented at 2:42 pm on September 14, 2019: member

Great concept ACK! See comments on minor points, also I think maybe the Handler/Cleanup handler logic could be simplified in future work, didn’t spend time on the how yet..

ryanofsky force-pushed on Sep 18, 2019

ryanofsky commented at 0:37 am on September 18, 2019: member

I’m fine reviewing either, though I guess if it were easy to do some scripted-diffs that would be nice.

Thanks, I split this up into smaller commits now (two of them scripted diffs)

Updated a3dc53304a4ccd0996e51322bee40572b5af7d25 -> 7b65a5356ea107af345b980a53e7e2d669711461 (pr/noglob.5 -> pr/noglob.6, compare)

DrahtBot added the label Needs rebase on Sep 18, 2019

ryanofsky force-pushed on Sep 18, 2019

ryanofsky commented at 3:17 pm on September 18, 2019: member

Could remove the “Needs conceptual review” tag, I think, since this has had a few concept ACKS.

I updated this to drop the last commit 7b65a5356ea107af345b980a53e7e2d669711461, which turned out not to be very related to the other changes, and just complicated the PR

Updated 7b65a5356ea107af345b980a53e7e2d669711461 -> 437a5e439fff50567503c0d76baaba50ebbfbd8f (pr/noglob.6 -> pr/noglob.7, compare) dropping the last commit Rebased 437a5e439fff50567503c0d76baaba50ebbfbd8f -> 396544eb9e7bfbcf41ef7f541b10ffd004603b5b (pr/noglob.7 -> pr/noglob.8) due to minor #include line conflict with #16512

MarcoFalke removed the label Needs Conceptual Review on Sep 18, 2019

DrahtBot removed the label Needs rebase on Sep 18, 2019

in src/node/node.h:30 in 2181a55c62 outdated

22@@ -20,6 +23,9 @@ class ChainClient;
23 //! and linking outside of bitcoind for simulation or testing.
24 struct Node
25 {
26+    std::unique_ptr<CConnman> connman;

MarcoFalke commented at 6:22 pm on September 18, 2019:

I think we should be using struct only for the dumbest data structures and not complex classes (such as the node). So this should be changed to m_connman

ryanofsky commented at 6:30 pm on September 18, 2019:

re: #16839 (review)

I think we should be using struct only for the dumbest data structures and not complex classes (such as the node). So this should be changed to m_connman

This is definitely supposed to be a dumb structure that doesn’t have any methods or contain any functionality. The only code that should need to access multiple members of this struct are init code, rpc code, and test code, and this struct is supposed to be a way of passing references around without needing to use globals or declare the same variables and parameters over and over. It’s not intended to be a wrapper class, or a new layer of code, or a grab-bag of random functionality.

MarcoFalke commented at 7:08 pm on September 18, 2019:

Thanks for clarifying. Could add that sentence to the doxygen comment of Node for people like me?

ryanofsky commented at 8:01 pm on September 18, 2019:

re: #16839 (review)

Thanks for clarifying. Could add that sentence to the doxygen comment of Node for people like me?

Sorry, existing comment was kind of a placeholder that didn’t explain things well. I rewrote it to hopefully be more helpful.

MarcoFalke commented at 6:25 pm on September 18, 2019: member

ACK (looked at the GitHub diff)

ryanofsky force-pushed on Sep 18, 2019

ryanofsky commented at 9:01 pm on September 18, 2019: member

Thanks for the review!

Updated 396544eb9e7bfbcf41ef7f541b10ffd004603b5b -> dab8d3e2c9cd9bfc92a4520edf6bf3374a9fe4f5 (pr/noglob.8 -> pr/noglob.9, compare) with suggested changes: better Node comment, BroadcastTransaction argument change.

ryanofsky commented at 3:57 pm on September 20, 2019: member

FYI maintainers, “Needs conceptual review” label was removed but this is still showing up in the “Chasing Concept ACK” column from https://github.com/bitcoin/bitcoin/projects/8.

MarcoFalke removed this from the "Chasing Concept ACK" column in a project

DrahtBot added the label Needs rebase on Sep 24, 2019

ryanofsky force-pushed on Sep 24, 2019

ryanofsky commented at 6:44 pm on September 24, 2019: member

Rebased dab8d3e2c9cd9bfc92a4520edf6bf3374a9fe4f5 -> ae5d88f8e08def49c7afdda040d6bb9fe73a0e3d (pr/noglob.9 -> pr/noglob.10) due to conflict with #16912

DrahtBot removed the label Needs rebase on Sep 24, 2019

jamesob commented at 8:55 pm on September 24, 2019: member

ACK https://github.com/bitcoin/bitcoin/pull/16839/commits/ae5d88f8e08def49c7afdda040d6bb9fe73a0e3d

Read through the code partially locally (to verify move-onlys) and on Github. Seems pretty uncontroversial and nicely split into scripted-diffs. I can do some manual testing tomorrow if need be. This Node object has a lot of promise for testing instead of having to push around globals.

DrahtBot added the label Needs rebase on Oct 2, 2019

ryanofsky force-pushed on Oct 2, 2019

ryanofsky commented at 2:15 pm on October 2, 2019: member

Rebased ae5d88f8e08def49c7afdda040d6bb9fe73a0e3d -> 50e0bb2df33cc47be10cf8e26ea7d9ce1d2e9704 (pr/noglob.10 -> pr/noglob.11) due to minor conflict (in qt test comment) with #17015

DrahtBot removed the label Needs rebase on Oct 2, 2019

in src/interfaces/node.cpp:267 in 21185e296a outdated

264     }
265     WalletCreationStatus createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, std::unique_ptr<Wallet>& result) override
266     {
267         std::shared_ptr<CWallet> wallet;
268-        WalletCreationStatus status = CreateWallet(*m_interfaces.chain, passphrase, wallet_creation_flags, name, error, warning, wallet);
269+        WalletCreationStatus status = CreateWallet(*m_node.chain, passphrase, wallet_creation_flags, name, error, warning, wallet);

ariard commented at 7:07 pm on October 8, 2019:

Do Node really need a reference to Chain, ChainImpl has already a reference to Node that’s a bit confusing ? I mean createWallet and loadWallet couldn’t be method of Wallet interface instead ? Would it work for the GUI ?

More generally, if we refactor RPC code to encapsulate in a RPCServer class, I see a WalletRPC subclass with a Chain member and a NodeRPC subclass with a Node member. And Node maybe initialize differently to avoid access to uneeded state which would address some concerns of #16839 (review)

ryanofsky commented at 10:07 pm on October 8, 2019:

#16839 (review)

Do Node really need a reference to Chain, ChainImpl has already a reference to Node that’s a bit confusing ?

I’m not sure what the problem might be here. One object owns the other and the owned object contains a back reference. The ChainImpl back reference to the Node struct can go away with some cleanups to initialization order (creating objects after the objects they depend on rather than before), but I didn’t want to try to change initialization order in this PR (it’s especially messy in the GUI and tests, see #16294).

I mean createWallet and loadWallet couldn’t be method of Wallet interface instead ? Would it work for the GUI ?

Sort of. interfaces::Wallet is the interface used to control an individual wallet, so it isn’t where I would put methods to load and create unrelated wallets. But #10102 adds a factory interface interfaces::Init which could be a logical place to move these methods.

More generally, if we refactor RPC code to encapsulate in a RPCServer class, I see a WalletRPC subclass with a Chain member and a NodeRPC subclass with a Node member.

Sure, I wouldn’t push to change the RPC framework in this way without knowing what the goals were, but I think this would be a reasonable design.

ariard commented at 3:21 am on November 2, 2019:

Thanks for answers (sorry for delay), wasn’t implying to do changes in this PR. Apart of RPC framework which need design goals, I think other one would make code nicer in future PRs, bookmarked.

in src/rpc/blockchain.h:53 in 50e0bb2df3 outdated

46@@ -46,4 +47,9 @@ UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex
47 /** Used by getblockstats to get feerates at different percentiles by weight  */
48 void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES], std::vector<std::pair<CAmount, int64_t>>& scores, int64_t total_weight);
49 
50+//! Pointer to node state that needs to be declared as a global to be accessible
51+//! RPC methods. Due to limitations of the RPC framework, there's currently no
52+//! direct way to pass in state to RPC methods without globals.
53+extern Node* g_rpc_node;

ariard commented at 7:13 pm on October 8, 2019:

nit: Wouldn’t shock me to move it to a new src/rpc/node.h libbitcoin_server source file but maybe too early

ryanofsky commented at 10:07 pm on October 8, 2019:

re: #16839 (review)

nit: Wouldn’t shock me to move it to a new src/rpc/node.h libbitcoin_server source file but maybe too early

Yes, though more ideally instead of more node files in the rpc directory, I’d like rpc files in the node directory, so generic rpc code would be distinct and easier to reuse from the wallet. Along the lines of #15732, I could see:

src/rpc/server.h, etc - rpc server implementation in libbitcoin_common.a src/node/rpc.h, rpc.cpp - node rpc methods in libbitcoin_node.a src/wallet/rpc.h, rpc.cpp - wallet rpc methods in libbitcoin_wallet.a

in src/node/node.h:28 in 18d8ee1bdc outdated

21+//! to use globals. More variables could be added to this struct (particularly
22+//! references to validation and mempool objects) to eliminate use of globals
23+//! and make code more modular and testable. The struct isn't intended to have
24+//! any member functions. It should just be a collection of references that can
25+//! be used without pulling in unwanted dependencies or functionality.
26 struct Node

ariard commented at 7:20 pm on October 8, 2019:

nit: maybe it should be name NodeState to avoid confusion with Node the already-there interface class?

ryanofsky commented at 10:07 pm on October 8, 2019:

re: #16839 (review)

nit: maybe it should be name NodeState to avoid confusion with Node the already-there interface class?

I think it’d be better to rename interfaces::Node to NodeInterface if the point is to distinguish between the internal struct and the external interface. I guess I have some aversion to adding suffixes like Info Data State to type names because you could add those suffixes to pretty much any type name just making it more verbose without changing the meaning.

Also, in practice, I think there shouldn’t be ambiguity in the code because interfaces::Node is only used by GUI code to control the node from the outside, not something used inside the node.

ariard commented at 3:26 am on November 2, 2019:

I guess I have some aversion to adding suffixes like Info Data State to type names because you could add those suffixes to pretty much any type name just making it more verbose without changing the meaning.

Ah, you’re right but most of the times it’s the cheapest way to avoid name collisions!

+1 for NodeInterface if we have opportunity later

ariard approved

ariard commented at 7:27 pm on October 8, 2019: member

ACK 50e0bb2, run tests and thoroughly read code.

I find the interwining of Node and Chain a bit confusing but good enough for now.

DrahtBot added the label Needs rebase on Oct 9, 2019

ryanofsky force-pushed on Oct 9, 2019

DrahtBot removed the label Needs rebase on Oct 9, 2019

ryanofsky commented at 4:56 pm on October 10, 2019: member

Thanks for the review!

Rebased 50e0bb2df33cc47be10cf8e26ea7d9ce1d2e9704 -> 9c4a2526615e6db54b49cf84ac77e165c3c9cbf3 (pr/noglob.11 -> pr/noglob.12) due to conflict with #15437

re: #16839#pullrequestreview-298995605

I find the interwining of Node and Chain a bit confusing but good enough for now.

In your comments you refer to Chain but in the code it is always interfaces::Chain. I wonder if renaming interfaces::Chain to ChainInterface would make it clearer that this class is an external interface used outside the node (by the wallet) to access chain state and not something that has to do with implementing node functionality or storing node data.

DrahtBot added the label Needs rebase on Oct 16, 2019

ryanofsky force-pushed on Oct 17, 2019

DrahtBot removed the label Needs rebase on Oct 17, 2019

DrahtBot added the label Needs rebase on Oct 21, 2019

ryanofsky force-pushed on Oct 21, 2019

DrahtBot removed the label Needs rebase on Oct 21, 2019

DrahtBot added the label Needs rebase on Oct 26, 2019

ryanofsky force-pushed on Oct 26, 2019

DrahtBot removed the label Needs rebase on Oct 26, 2019

scripted-diff: Rename InitInterfaces to NodeContext

-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }

s 'struct InitInterfaces'              'struct NodeContext'
s 'InitInterfaces interfaces'          'NodeContext node'
s 'InitInterfaces& interfaces'         'NodeContext\& node'
s 'InitInterfaces m_interfaces'        'NodeContext m_context'
s 'InitInterfaces\* g_rpc_interfaces'  'NodeContext* g_rpc_node'
s 'g_rpc_interfaces = &interfaces'     'g_rpc_node = \&node'
s 'g_rpc_interfaces'                   'g_rpc_node'
s 'm_interfaces'                       'm_context'
s 'interfaces\.chain'                  'node.chain'
s '\(AppInitMain\|Shutdown\|Construct\)(interfaces)' '\1(node)'
s 'init interfaces' 'chain clients'
-END VERIFY SCRIPT-

301bd41a2e

MOVEONLY: Move NodeContext struct to node/context.h 4d5448c76b

Pass NodeContext, ConnMan, BanMan references more places

So g_connman and g_banman globals can be removed next commit.

e6f4f895d5

scripted-diff: Remove g_connman, g_banman globals

-BEGIN VERIFY SCRIPT-
sed -i 's:#include <interfaces/chain.h>:#include <banman.h>\n#include <interfaces/chain.h>\n#include <net.h>\n#include <net_processing.h>:' src/node/context.cpp
sed -i 's/namespace interfaces {/class BanMan;\nclass CConnman;\nclass PeerLogicValidation;\n&/' src/node/context.h
sed -i 's/std::unique_ptr<interfaces::Chain> chain/std::unique_ptr<CConnman> connman;\n    std::unique_ptr<PeerLogicValidation> peer_logic;\n    std::unique_ptr<BanMan> banman;\n    &/' src/node/context.h
sed -i '/std::unique_ptr<[^>]\+> \(g_connman\|g_banman\|peerLogic\);/d' src/banman.h src/net.h src/init.cpp
sed -i 's/g_connman/m_context.connman/g' src/interfaces/node.cpp
sed -i 's/g_banman/m_context.banman/g' src/interfaces/node.cpp
sed -i 's/g_connman/m_node.connman/g' src/interfaces/chain.cpp src/test/setup_common.cpp
sed -i 's/g_banman/m_node.banman/g' src/test/setup_common.cpp
sed -i 's/g_connman/node.connman/g' src/init.cpp src/node/transaction.cpp
sed -i 's/g_banman/node.banman/g' src/init.cpp
sed -i 's/peerLogic/node.peer_logic/g' src/init.cpp
sed -i 's/g_connman/g_rpc_node->connman/g' src/rpc/mining.cpp src/rpc/net.cpp src/rpc/rawtransaction.cpp
sed -i 's/g_banman/g_rpc_node->banman/g' src/rpc/net.cpp
sed -i 's/std::shared_ptr<CWallet> wallet =/node.context()->connman = std::move(test.m_node.connman);\n    &/' src/qt/test/wallettests.cpp
-END VERIFY SCRIPT-

8922d7f6b7

Avoid using g_rpc_node global in wallet code

Wallet code should use interfaces::Chain and not directly access to node state.

Add a g_rpc_chain replacement global for wallet code to use, and move
g_rpc_node definition to a libbitcoin_server source file so there are link
errors if wallet code tries to access it.

362ded410b

DrahtBot added the label Needs rebase on Oct 28, 2019

MarcoFalke commented at 3:32 pm on October 28, 2019: member

Looks ready for merge after rebase

ryanofsky force-pushed on Oct 28, 2019

ryanofsky renamed this:
~~Replace Connman and BanMan globals with Node local~~
Replace Connman and BanMan globals with NodeContext local
on Oct 28, 2019

DrahtBot removed the label Needs rebase on Oct 28, 2019

ryanofsky commented at 11:58 pm on October 28, 2019: member

Rebased 34a7a50dbeec1ee36b18fa0d1a0ffbf7a1907f98 -> 362ded410b8cb1104b7ef31ff8488fec4824a7d5 (pr/noglob.15 -> pr/noglob.16) due to conflict with #16202

Also renamed struct Node to struct NodeContext, since it should also be useful to have a WalletContext later to put things like cs_wallets, vpwallets, and the interfaces::Chain pointer shared between multiple wallets.

laanwj commented at 11:29 am on October 30, 2019: member

ACK 362ded410b8cb1104b7ef31ff8488fec4824a7d5

laanwj referenced this in commit 471e5f8829 on Oct 30, 2019

laanwj merged this on Oct 30, 2019

laanwj closed this on Oct 30, 2019

ariard commented at 3:30 am on November 2, 2019: member

Post-humous ACK, diff from 50e0bb2 is renaming struct Node, integration of bip61 removal and some comments changed.

I wonder if renaming interfaces::Chain to ChainInterface would make it clearer that this class is an external interface used outside the node (by the wallet) to access chain state and not something that has to do with implementing node functionality or storing node data.

Agree may have avoid us confusion there.

deadalnix referenced this in commit 6b9c6d6c31 on May 17, 2020

deadalnix referenced this in commit 2f0efa5f4f on May 17, 2020

deadalnix referenced this in commit 2b8b4bb8e9 on Jun 6, 2020

deadalnix referenced this in commit 5c8f7c532c on Jun 7, 2020

deadalnix referenced this in commit 7a4fc02d8d on Jun 7, 2020

ftrader referenced this in commit a0b24b7586 on Apr 14, 2021

ftrader referenced this in commit bb78078695 on Apr 14, 2021

kittywhiskers referenced this in commit 6589af75b1 on Jan 9, 2022

kittywhiskers referenced this in commit 70afa78415 on Jan 11, 2022

kittywhiskers referenced this in commit 5bcd907b0b on Jan 25, 2022

DrahtBot locked this on Feb 15, 2022

Replace Connman and BanMan globals with NodeContext local #16839

Conflicts