CConnman: Remove some global usage in the gui #8888

• React to the BannedListChanged signal and push down the new list from the clientModel, rather than pulling it the other way
• Do the same for NoteStats, pushing them down rather than pulling.

The eventual goal is to contain g_connman in clientModel, at which point we can pass it in and eliminate the global usage.

The wallet changes that were here originally will show up in a new PR.

Concept ACK

concept ACK

Test failure:

start_node: RPC succesfully started
JSONRPC error: Error: Peer-to-peer functionality missing or disabled
Stopping nodes
Not cleaning up dir /tmp/testk3xseb1n/41
Failed
stderr:
  File "/home/travis/build/bitcoin/bitcoin/qa/rpc-tests/test_framework/test_framework.py", line 148, in main
    self.run_test()
  File "/home/travis/build/bitcoin/bitcoin/build/../qa/rpc-tests/wallet.py", line 208, in run_test
    txIdNotBroadcasted  = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 2)
  File "/home/travis/build/bitcoin/bitcoin/qa/rpc-tests/test_framework/coverage.py", line 49, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/travis/build/bitcoin/bitcoin/qa/rpc-tests/test_framework/authproxy.py", line 151, in __call__
    raise JSONRPCException(response['error'])
Pass: False, Duration: 15 s

This change results is two new -Wshadow warnings:

qt/peertablemodel.cpp:62:2445: warning: declaration shadows a local variable [-Wshadow]
qt/peertablemodel.cpp:74:264: warning: declaration shadows a local variable [-Wshadow]

qt/peertablemodel.cpp:54:49: note: previous declaration is here
    void refreshPeers(QList<CNodeCombinedStats> stats)

Before fixing up here, I'd like to get opinions on an alternate approach to the wallet: https://github.com/theuni/bitcoin/commit/349edab789557db615b29a5869118b4b08c50dab . Note that this was done quickly, it needs to be fixed up for correctness in a few places.

There, rather than passing the connman instance into the wallet, the wallet loses the ability to relay. Instead, callers are expected to handle relaying themselves. I really like this approach as it removes the p2p layer dependency from the wallet. @jonasschnelli / @laanwj: Thoughts?

@theuni Personally I like the new approach better (wallet does not require the connman). Even though this would require some "glue logic" in the caller for sending out the inv, it would reduce the complexity of our "modular" design. For example, it would make error handling easier. Right now the wallet can be seen as a proxy between GUI/RPC and Mempool/p2p. And the wallet needs to handle mempool rejections and pass on the error message to the GUI. (Which it does not do right now as you can see in the code and on irc.)

So Concept ACK from me on your new suggestion.

@MarcoFalke Agreed on all points. Notice that with that commit, mempool handling (when recording a new tx) is also pushed out of the wallet, and errors there are handled (albeit awkwardly): https://github.com/theuni/bitcoin/commit/349edab789557db615b29a5869118b4b08c50dab#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR972 . That error handling seemed funky to me as well, but I wasn't sure how problems would manifest. Thanks for the irc link, looks like we need a distinct "this was not accepted to the mempool" message.

I think I'll split this up into 2 PRs, so that they don't drag eachother down. I'll leave this one for the qt changes, and create another to remove p2p from the wallet.

@theuni: I like the https://github.com/theuni/bitcoin/commit/349edab789557db615b29a5869118b4b08c50dab approach. Maybe we could drive the even further (later): What if the wallet only uses signals to access the conman and the mempool? I'm not entirely sure, but AFAIK we would only need two signals, something ike PushInventoryAllNodes() and bool PushToMemoryPool().

Changed labels, as the scope changed.

Also, needs rebase again.

Concept ACK.

Needs rebase.

Still needs rebase.

Closing due to inactivity (feel free to reopen).

Thanks, will do so soon.

@theuni @jonasschnelli @MarcoFalke

I would like to take on rebasing this as it is another step towards eliminating globals. Is there anything else that's within scope here that https://github.com/theuni/bitcoin/commit/349edab789557db615b29a5869118b4b08c50dab didn't cover?

Code here is largely out of date. Pushing network stats to the gui instead of having it query them might be a good idea, though. I don't know enough about the topic to say.