net processing: swap out signals for an interface class #10756

theuni · 2017-07-06T20:26:28Z

See individual commits.
Benefits:

Allows us to begin moving stuff out of CNode and into CNodeState (after Small step towards demangling cs_main from CNodeState #10652 and follow-ups)
Drops boost dependency and overhead
Drops global signal registration
Friendlier backtraces

gmaxwell · 2017-07-09T11:35:43Z

Super mega concept ACK. FWIW, I tested something functionally like this before (eliminating the signals with a direct function call) and saw a measurable performance improvement, I assume because there is synchronization inside the signals stuff (plus a super deep call stack).

TheBlueMatt · 2017-07-10T00:22:21Z

This is awesome. Lots of future work to do, but good first step. Will circle back around post-15.

jnewbery · 2017-07-10T13:59:26Z

@gmaxwell

tested something functionally like this before (eliminating the signals with a direct function call) and saw a measurable performance improvement

How did you test that? It'd be good to benchmark whether this gives us a performance improvement.

jnewbery

Tested ACK f785e8479a8ffca6c89d71f9dd6f1c1b455c2671. A few nitty comments inline, plus one more: https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L126 seems to be a comment that's got stranded from its code, and is now nonsensical after this PR. Consider removing it entirely.

I've run this over the function test suite and it seemed to run slightly faster, though within expected variance. Do you have any ways of more focused performance testing?

jnewbery · 2017-07-10T14:07:10Z

src/net.cpp

@@ -1996,15 +1996,16 @@ void CConnman::ThreadMessageHandler()
                continue;

            // Receive messages


Nit: move Receive messages comment into the if block (the block covers both receiving and sending messages. Having the comment outside the block suggests it's just for receiving messages)

jnewbery · 2017-07-10T14:11:03Z

src/net.h

@@ -422,18 +423,18 @@ struct CombinerAll
 };

 // Signals for message handling


Change comment to Interface for message handling

jnewbery · 2017-07-10T14:18:43Z

src/net_processing.h

+    * Send queued protocol messages to be sent to a give node.
+    *
+    * @param[in]   pto             The node which we are sending messages to.
+    * @param[in]   connman         The connection manager for that node.


remove this line. connman is no longer a param for this function.

theuni · 2017-07-11T00:19:32Z

Remembering @gmaxwell's mention of it before, I was hoping to see a slight speedup here, but I haven't been able to observe anything substantial either. No test varies more than 1% from any other.

@jnewbery my test was to do a sync to block 150000 between two localhost nodes running from ramdisk datadirs. I believe that focuses on the hot-spot well enough.

theuni · 2017-07-11T16:37:58Z

Needed rebase after #10179, so I went ahead and squashed it down too.

gmaxwell · 2017-07-11T19:17:42Z

@jnewbery GBE connected hosts running a patch like this, out of ram, may have also required turning up our networking buffers. maybe that other locking changes we made mooted the improvement. I'll go see if I can find the relevant chat logs.

gmaxwell

ACK.

TheBlueMatt

This is awesome. As mentioned, after this, PeerLogicValidation should probably get a raname, as the "Validation" referred to ValidationInterface.

TheBlueMatt · 2017-08-02T18:37:31Z

src/net.cpp

+                    return;
+                // Send messages
+                {
+                    LOCK(pnode->cs_sendProcessing);


Can you drop cs_sendProcessing while you're here? Its literally entirely unused except for this lock here.

Sure. It's safe to drop it now, but we'd need to put it back for parallel processing.

TheBlueMatt · 2017-08-02T18:58:36Z

src/net.cpp

@@ -2212,6 +2213,7 @@ CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In) : nSeed0(nSeed0In), nSe
    nMaxAddnode = 0;
    nBestHeight = 0;
    clientInterface = NULL;
+    messageInterface = NULL;


I believe we're nullptr'ing now?

TheBlueMatt · 2017-08-02T19:00:39Z

src/net.cpp

@@ -1966,7 +1964,9 @@ bool CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
    if (fAddnode)
        pnode->fAddnode = true;

-    GetNodeSignals().InitializeNode(pnode, *this);
+    if (messageInterface) {
+        messageInterface->InitializeNode(pnode);


I feel like we should just go for the direct deref here. A CConnman without a messageInterface feels like a client-of-the-API failure. Maybe just assert(messageInterface) in ::Start?

Heh, I went back and forth on this. Was waiting for someone to complain. Agreed.

TheBlueMatt · 2017-08-02T19:01:39Z

src/net.h

-    boost::signals2::signal<void (CNode*, CConnman&)> InitializeNode;
-    boost::signals2::signal<void (NodeId, bool&)> FinalizeNode;
+protected:
+    NetEventsInterface() = default;


Why did you define the constructor here?

Just a habit of mine to declare the ctor as protected for classes that should be inherited from. Since there are pure virtuals here, it's not really necessary.

TheBlueMatt · 2017-08-02T19:02:11Z

src/net.h

+protected:
+    NetEventsInterface() = default;
+public:
+    virtual ~NetEventsInterface() = default;


Why do we need to make the destructor virtual? CConnman doesn't care about construction/destruction, it just takes a ptr.

To make sure that the base gets cleaned up if, for example, it's stored as an abstract std::unique_ptr

Well my point was the CConnman (and related net stuff), which "owns" this interface explicitly does not do so (and does not have any desire to ever own the object), so adding anything additional to the interface feels strange to me.

I must be missing your point. This is just a safety precaution in case anything ever wants to hold an abstract pointer. This may be the case if, for example, we create a vector of message processors in order to run them against each-other.

I can remove it if you'd like, but I'm not understanding the objection to being cautious.

Well my objection was just that it forces you into a virtual destructor unless you remember to call derived classes final, and since I dont think anything should ever have anything other than a reference to (ie will not destruct directly) a NetEventsInterface, no need to make anything virtual. I don't care too strongly, however.

TheBlueMatt · 2017-08-02T23:22:53Z

Note that some of the Process functions are likely to move again into CNodeState or so, I believe, but this is an obviously good step.

theuni · 2017-08-16T18:22:42Z

Rebased, updated for recent changes, and addressed @TheBlueMatt's feedback

TheBlueMatt

utACK 6e6f06c8d971f21262c6f2272b70170d19670c2a. Feel like renaming PeerLogicValidation now that its not strictly about "Validation" anymore?

TheBlueMatt · 2017-08-18T00:44:34Z

src/net_processing.cpp

@@ -417,20 +369,20 @@ void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connman) {
                return;
            }
        }
-        connman.ForNode(nodeid, [&connman](CNode* pfrom){
+        connman->ForNode(nodeid, [&connman](CNode* pfrom){


Can drop the & now for connman, can just copy (and in a few other places).

theuni · 2017-09-05T23:12:44Z

Rebased. Same as before, plus I took the suggestion here: #10756 (comment)

laanwj · 2017-09-06T21:10:35Z

src/net.cpp

@@ -89,10 +89,6 @@ std::string strSubVersion;

 limitedmap<uint256, int64_t> mapAlreadyAskedFor(MAX_INV_SZ);

-// Signals for message handling


Another global block gone, nice!

laanwj · 2017-09-06T21:12:25Z

src/net.h

        nSendBufferMaxSize = connOptions.nSendBufferMaxSize;
        nReceiveFloodSize = connOptions.nReceiveFloodSize;
        nMaxOutboundTimeframe = connOptions.nMaxOutboundTimeframe;
        nMaxOutboundLimit = connOptions.nMaxOutboundLimit;
        vWhitelistedRange = connOptions.vWhitelistedRange;
+


Empty line?

laanwj · 2017-09-06T21:13:47Z

src/net.h

@@ -436,19 +439,16 @@ struct CombinerAll
    }
 };

-// Signals for message handling
-struct CNodeSignals
+// Interface for message handling


Ideally we'd use a doxygen-compliant comment here, so that it shows the class description

/** * Interface for message handling */

I had to rebase anyway, so I went ahead and fixed these two nits as well.

There are a few too many edge-cases here to make this a scripted diff. The following commits will move a few functions into PeerLogicValidation, where the local connman instance can be used. This change prepares for that usage.

Drop boost signals in favor of a stateful class. This will allow the message processing loop to actually move to net_processing in a future step.

The copy in PeerLogicValidation can be used instead.

JeremyRubin

utack 80e2e9d

JeremyRubin · 2017-09-07T00:50:26Z

src/net_processing.h

-void UnregisterNodeSignals(CNodeSignals& nodeSignals);
-
-class PeerLogicValidation : public CValidationInterface {
+class PeerLogicValidation : public CValidationInterface, public NetEventsInterface {


Maybe it makes sense to mark stuff final/not be overriding an abc here? NetEventsInterface feels overkill unless we anticipate per-connection different handlers.

I don't anticipate per-connection, but definitely per-connman. I have immediate plans to add different handlers and run them against each-other for testing.

Making this final is a good idea, but I'd rather not keep squashing more changes in. Let's do that in a follow up that also renames PeerLogicValidation to something that better reflects what it's doing.

TheBlueMatt · 2017-09-07T17:52:50Z

src/init.cpp

    g_connman.reset();
+    peerLogic.reset();


As discussed, peerLogic should probably get destroyed before g_connman, though g_connman needs to be stopped first (and all references to m_msgproc in CConnman need to be in threads that will be stopped or should be gated on running - OpenNetworkConnection I believe is the only current violation of this).

This should avoid either attempting to use an invalid reference/pointer to the other.

There are a few too many edge-cases here to make this a scripted diff. The following commits will move a few functions into PeerLogicValidation, where the local connman instance can be used. This change prepares for that usage. Github-Pull: bitcoin#10756 Rebased-From: 28f11e9

Drop boost signals in favor of a stateful class. This will allow the message processing loop to actually move to net_processing in a future step. Github-Pull: bitcoin#10756 Rebased-From: 8ad663c

The copy in PeerLogicValidation can be used instead. Github-Pull: bitcoin#10756 Rebased-From: 80e2e9d

This should avoid either attempting to use an invalid reference/pointer to the other. Github-Pull: bitcoin#10756 Rebased-From: 2525b97

…e class 2525b97 net: stop both net/net_processing before destroying them (Cory Fields) 80e2e9d net: drop unused connman param (Cory Fields) 8ad663c net: use an interface class rather than signals for message processing (Cory Fields) 28f11e9 net: pass CConnman via pointer rather than reference (Cory Fields) Pull request description: See individual commits. Benefits: - Allows us to begin moving stuff out of CNode and into CNodeState (after bitcoin#10652 and follow-ups) - Drops boost dependency and overhead - Drops global signal registration - Friendlier backtraces Tree-SHA512: af2038c959dbec25f0c90c74c88dc6a630e6b9e984adf52aceadd6954aa463b6aadfccf979c2459a9f3354326b5077ee02048128eda2a649236fadb595b66ee3

PeerLogicValidation was originally net_processing's implementation to the validation interface. It has since grown to contain much of net_processing's logic. Therefore rename it to reflect its responsibilities. Suggested in bitcoin#10756 (review).

…ager -BEGIN VERIFY SCRIPT- sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test) -END VERIFY SCRIPT- PeerLogicValidation was originally net_processing's implementation to the validation interface. It has since grown to contain much of net_processing's logic. Therefore rename it to reflect its responsibilities. Suggested in bitcoin#10756 (review).

…ager -BEGIN VERIFY SCRIPT- sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test) sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test) -END VERIFY SCRIPT- PeerLogicValidation was originally net_processing's implementation to the validation interface. It has since grown to contain much of net_processing's logic. Therefore rename it to reflect its responsibilities. Suggested in bitcoin#10756 (review).

bb6a32c [net processing] Move Misbehaving() to PeerManager (John Newbery) aa114b1 [net_processing] Move SendBlockTransactions into PeerManager (John Newbery) 3115e00 [net processing] Move MaybePunishPeerForTx to PeerManager (John Newbery) e662e2d [net processing] Move ProcessOrphanTx to PeerManager (John Newbery) b70cd89 [net processing] Move MaybePunishNodeForBlock into PeerManager (John Newbery) d777835 [net processing] Move ProcessHeadersMessage to PeerManager (John Newbery) 64f6162 [whitespace] tidy up indentation after scripted diff (John Newbery) 58bd369 scripted-diff: [net processing] Rename PeerLogicValidation to PeerManager (John Newbery) 2297b26 [net_processing] Pass chainparams to PeerLogicValidation constructor (John Newbery) 824bbd1 [move only] Collect all private members of PeerLogicValidation together (John Newbery) Pull request description: Continues the work of moving net_processing logic into PeerLogicValidation. See bitcoin/bitcoin#19704 and bitcoin/bitcoin#19607 (comment) for motivation. This PR also renames `PeerLogicValidation` to `PeerManager` as suggested in bitcoin/bitcoin#10756 (review). ACKs for top commit: MarcoFalke: re-ACK bb6a32c only change is rebase due to conflict in struct NodeContext and variable rename 🤸 hebasto: re-ACK bb6a32c, only rebased, and added renaming `s/peer_logic/peerman/` into scripted-diff since my [previous](bitcoin/bitcoin#19791 (review)) review (verified with `git range-diff`). Tree-SHA512: a2de4a521688fd25125b401e5575402c52b328a0fa27b3010567008d4f596b960aabbd02b2d81f42658f88f4365443fadb1008150a62fbcea123fb42d85a2c21

…ager -BEGIN VERIFY SCRIPT- sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test) sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test) -END VERIFY SCRIPT- PeerLogicValidation was originally net_processing's implementation to the validation interface. It has since grown to contain much of net_processing's logic. Therefore rename it to reflect its responsibilities. Suggested in bitcoin#10756 (review).

…ager -BEGIN VERIFY SCRIPT- sed -i 's/PeerLogicValidation/PeerManager/g' $(git grep -l PeerLogicValidation ./src ./test) sed -i 's/peer_logic/peerman/g' $(git grep -l peer_logic ./src ./test) -END VERIFY SCRIPT- PeerLogicValidation was originally net_processing's implementation to the validation interface. It has since grown to contain much of net_processing's logic. Therefore rename it to reflect its responsibilities. Suggested in bitcoin/bitcoin#10756 (review).

21f05c1 net: drop unused connman param (Cory Fields) 50853a2 net: use an interface class rather than signals for message processing (furszy) 67757cd net: pass CConnman via pointer rather than reference (furszy) Pull request description: Another decouple from #2411. Coming from bitcoin#10756. > See individual commits. > Benefits: > * Allows us to begin moving stuff out of CNode and into CNodeState. > * Drops boost dependency and overhead > * Drops global signal registration > * Friendlier backtraces ACKs for top commit: random-zebra: ACK 21f05c1 Fuzzbawls: ACK 21f05c1 Tree-SHA512: 8a1ace6d9b8dd2a36d0e1b1465b1f71e7c0a5fcd9a33462210a724cc6eafc119166bd0ee9e9db7862ac41dc62bfa3373c51f575d138b42ec0e140b2524e831d4

ecde04a [Consensus] Bump Active Protocol version to 70923 for v5.3 (random-zebra) b63e4f5 Consensus: Add v5.3 enforcement height for testnet. (furszy) f44be94 Only relay IPv4, IPv6, Tor addresses (Pieter Wuille) 015298c fix: tor: Call event_base_loopbreak from the event's callback (furszy) 34ff7a8 Consensus: Add mnb ADDRv2 guard. (furszy) b4515dc GUI: Present v3 onion addresses properly in MNs list. (furszy) 337d43d tests: don't export in6addr_loopback (Vasil Dimov) 2cde8e0 GUI: Do not show the tor v3 onion address in the topbar. (furszy) 0b5f406 Doc: update tor.md with latest upstream information. (furszy) 89df7f2 addrman: ensure old versions don't parse peers.dat (Vasil Dimov) bb90c5c test: add getnetworkinfo network name regression tests (Jon Atack) d8e01b5 rpc: update GetNetworksInfo() to not return unsupported networks (Jon Atack) 57fc7b0 net: update GetNetworkName() with all enum Network cases (Jon Atack) 647d60b tests: Modify rpc_bind to conform to bitcoin#14532 behaviour. (Carl Dong) d4d6729 Allow running rpc_bind.py --nonloopback test without IPv6 (Kristaps Kaupe) 4a034d8 test: Add rpc_bind test to default-run tests (Wladimir J. van der Laan) 61a08af [tests] bind functional test nodes to 127.0.0.1 Prevents OSX firewall (Sjors Provoost) 6a4f1e0 test: Add basic addr relay test (furszy) 78aa61c net: Make addr relay mockable (furszy) ba954ca Send and require SENDADDRV2 before VERACK (Pieter Wuille) 61c2ed4 Bump net protocol version + don't send 'sendaddrv2' to pre-70923 software (furszy) ccd508a tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov) 6da9a14 net: advertise support for ADDRv2 via new message (furszy) e58d5d0 Migrate to test_large_inv() to Misbehaving logging. (furszy) d496b64 [QA] fix mininode CAddress ser/deser (Jonas Schnelli) cec9567 net: CAddress & CAddrMan: (un)serialize as ADDRv2 Change the serialization of `CAddrMan` to serialize its addresses in ADDRv2/BIP155 format by default. Introduce a new `CAddrMan` format version (3). (furszy) b8c1dda streams update: get rid of nType and nVersion. (furszy) 3eaa273 Support bypassing range check in ReadCompactSize (Pieter Wuille) a237ba4 net: recognize TORv3/I2P/CJDNS networks (Vasil Dimov) 8e50853 util: make EncodeBase32 consume Spans (Sebastian Falbesoner) 1f67e30 net: CNetAddr: add support to (un)serialize as ADDRv2 (Vasil Dimov) 2455420 test: move HasReason so it can be reused (furszy) d41adb4 util: move HasPrefix() so it can be reused (Vasil Dimov) f6f86af Unroll Keccak-f implementation (Pieter Wuille) 45222e6 Implement keccak-f[1600] and SHA3-256 (Pieter Wuille) 08ad06d net: change CNetAddr::ip to have flexible size (furszy) 3337219 net: improve encapsulation of CNetAddr. (furszy) 910d5c4 test: Do not instantiate CAddrDB for static call (Hennadii Stepanov) 6b607ef Drop IsLimited in favor of IsReachable (Ben Woosley) a40711b IsReachable is the inverse of IsLimited (DRY). Includes unit tests (marcaiaf) 8839828 net: don't accept non-left-contiguous netmasks (Vasil Dimov) 5d7f864 rpcbind: Warn about exposing RPC to untrusted networks (Luke Dashjr) 2a6abd8 CNetAddr: Add IsBindAny method to check for INADDR_ANY (Luke Dashjr) 4fdfa45 net: Always default rpcbind to localhost, never "all interfaces" (Luke Dashjr) 31064a8 net: Minor accumulated cleanups (furszy) 9f9c871 tests: Avoid using C-style NUL-terminated strings as arguments (practicalswift) f6c52a3 tests: Add tests to make sure lookup methods fail on std::string parameters with embedded NUL characters (practicalswift) a751b9b net: Avoid using C-style NUL-terminated strings as arguments in the netbase interface (furszy) f30869d test: add IsRFC2544 tests (Mark Tyneway) ed5abe1 Net: Proper CService deserialization + GetIn6Addr return false if addr isn't an IPv6 addr (furszy) 86d73fb net: save the network type explicitly in CNetAddr (Vasil Dimov) ad57dfc net: document `enum Network` (Vasil Dimov) cb160de netaddress: Update CNetAddr for ORCHIDv2 (Carl Dong) c3c04e4 net: Better misbehaving logging (furszy) 3660487 net: Use C++11 member initialization in protocol (Marco) 082baa3 refactor: Drop unused CBufferedFile::Seek() (Hennadii Stepanov) e2d776a util: CBufferedFile fixes (Larry Ruane) 6921f42 streams: backport OverrideStream class (furszy) Pull request description: Conjunction of a large number of back ports, updates and refactorings that made with the final goal of implementing v3 Onion addresses support (BIP155 https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki) before the tor v2 addresses EOL, scheduled, by the Tor project, for (1) July 15th: v2 addr support removal from the code base, and (2) October 15th: v2 addr network disable, where **every peer in our network running under Tor will loose the connection and drop the network**. As BIP155 describes, this is introducing a new P2P message to gossip longer node addresses over the P2P network. This is required to support new-generation Onion addresses, I2P, and potentially other networks that have longer endpoint addresses than fit in the 128 bits of the current addr message. In order to achieve the end goal, had to: 1. Create Span class and push it up to latest Bitcoin implementation. 2. Update the whole serialization framework and every object using it up to latest Bitcoin implementation (3-4 years ahead of what we currently are in master). 3. Update the address manager implementing ASN-based bucketing of the network nodes. 4. Update and refactor the netAddress and address manager tests to latest Bitcoin implementation (4 years ahead of what we currently are in master). 5. Several util string, vector, encodings, parsing, hashing backports and more.. Important note: This PR it is not meant to be merged as a standalone PR, will decouple smaller ones moving on. Adding on each sub-PR its own description isolated from this big monster. Second note: This is still a **work-in-progress**, not ready for testing yet. I'm probably missing to mention few PRs that have already adapted to our sources. Just making it public so can decouple the changes, we can start merging them and i can continue working a bit more confortable (rebase a +170 commits separate branch is not fun..). ### List of back ported and adapted PRs: Span and Serialization: ---------------- * bitcoin#12886. * bitcoin#12916. * bitcoin#13558. * bitcoin#13697. (Only Span's commit 29943a9) * bitcoin#17850. * bitcoin#17896. * bitcoin#12752. * bitcoin#16577. * bitcoin#16670. (without faebf62) * bitcoin#17957. * bitcoin#18021. * bitcoin#18087. * bitcoin#18112 (only from 353f376 that we don't support). * bitcoin#18167. * bitcoin#18317. * bitcoin#18591 (only Span's commit 0fbde48) * bitcoin#18468. * bitcoin#19020. * bitcoin#19032. * bitcoin#19367. * bitcoin#19387. Net, NetAddress and AddrMan: ---------------- * bitcoin#7932. * bitcoin#10756. * bitcoin#10765. * bitcoin#12218. * bitcoin#12855. * bitcoin#13532. * bitcoin#13575. * bitcoin#13815. * bitcoin#14532. * bitcoin#15051. * bitcoin#15138. * bitcoin#15689. * bitcoin#16702. * bitcoin#17243. * bitcoin#17345. * bitcoin#17754. * bitcoin#17758. * bitcoin#17812. * bitcoin#18023. * bitcoin#18454. * bitcoin#18512. * bitcoin#19314. * bitcoin#19687 Keys and Addresses encoding: ---------------- * bitcoin#11372. * bitcoin#17511. * bitcoin#17721. Util: ---------------- * bitcoin#9140. * bitcoin#16577. * bitcoin#16889. * bitcoin#19593. Bench: ---------------- * bitcoin#16299. BIP155: ---------------- * bitcoin#19351. * bitcoin#19360. * bitcoin#19534. * bitcoin#19628. * bitcoin#19841. * bitcoin#19845. * bitcoin#19954. * bitcoin#19991 (pending). * bitcoin#19845. * bitcoin#20000 (pending). * bitcoin#20120. * bitcoin#20284. * bitcoin#20564. * bitcoin#21157 (pending). * bitcoin#21564 (pending). * Fully removed v2 onion addr support. * Add hardcoded seeds. * Add release-notes, changes to files.md and every needed documentation. I'm currently working on the PRs marked as "pending", this isn't over, but I'm pretty pretty close :). What a long road.. ACKs for top commit: random-zebra: utACK ecde04a Fuzzbawls: ACK ecde04a Tree-SHA512: 82c95fbda76fce63f96d8a9af7fa9a89cb1e1b302b7891e27118a6103af0be23606bf202c7332fa61908205e6b6351764e2ec23d753f1e2484028f57c2e8b51a

fanquake added the P2P label Jul 7, 2017

jnewbery mentioned this pull request Jul 7, 2017

[wallet] Remove Wallet dependencies from init.cpp #10762

Merged

jnewbery reviewed Jul 10, 2017

View reviewed changes

theuni force-pushed the no-net-signals2 branch from 20f05dc to 1f166fd Compare July 11, 2017 16:36

gmaxwell approved these changes Jul 11, 2017

View reviewed changes

TheBlueMatt reviewed Aug 2, 2017

View reviewed changes

theuni mentioned this pull request Aug 3, 2017

Refactor fRelayTxes global to CConnman member #10915

Closed

theuni force-pushed the no-net-signals2 branch from 1f166fd to 6e6f06c Compare August 16, 2017 18:21

TheBlueMatt reviewed Aug 18, 2017

View reviewed changes

theuni force-pushed the no-net-signals2 branch from 6e6f06c to 927036f Compare September 5, 2017 23:11

laanwj self-assigned this Sep 6, 2017

laanwj reviewed Sep 6, 2017

View reviewed changes

theuni added 3 commits September 6, 2017 19:03

net: pass CConnman via pointer rather than reference

28f11e9

There are a few too many edge-cases here to make this a scripted diff. The following commits will move a few functions into PeerLogicValidation, where the local connman instance can be used. This change prepares for that usage.

net: use an interface class rather than signals for message processing

8ad663c

Drop boost signals in favor of a stateful class. This will allow the message processing loop to actually move to net_processing in a future step.

net: drop unused connman param

80e2e9d

The copy in PeerLogicValidation can be used instead.

theuni force-pushed the no-net-signals2 branch from 927036f to 80e2e9d Compare September 6, 2017 23:33

JeremyRubin approved these changes Sep 7, 2017

View reviewed changes

TheBlueMatt reviewed Sep 7, 2017

View reviewed changes

net: stop both net/net_processing before destroying them

2525b97

This should avoid either attempting to use an invalid reference/pointer to the other.

maflcko mentioned this pull request Oct 3, 2017

Fix crash on shutdown with invalid wallet #11326

Merged

morcos mentioned this pull request Nov 2, 2017

0.15: Backports #11592

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017

net: drop unused connman param

b4136f2

The copy in PeerLogicValidation can be used instead. Github-Pull: bitcoin#10756 Rebased-From: 80e2e9d

UdjinM6 mentioned this pull request Oct 23, 2018

Fix ' warning: delete called on non-final 'PeerLogicValidation' that has virtual functions but non-virtual destructor' dashpay/dash#2363

Closed

jnewbery mentioned this pull request Aug 24, 2020

[net processing] Move Misbehaving() to PeerManager #19791

Merged

This was referenced Jun 12, 2021

[net processing] Swap out signals for an interface class PIVX-Project/PIVX#2417

Merged

Road to Tor v3 support (BIP155) PIVX-Project/PIVX#2411

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -1996,15 +1996,16 @@ void CConnman::ThreadMessageHandler()
		continue;

		// Receive messages

		@@ -422,18 +423,18 @@ struct CombinerAll
		};

		// Signals for message handling

		@@ -89,10 +89,6 @@ std::string strSubVersion;

		limitedmap<uint256, int64_t> mapAlreadyAskedFor(MAX_INV_SZ);

		// Signals for message handling

net processing: swap out signals for an interface class #10756

net processing: swap out signals for an interface class #10756

Conversation

theuni commented Jul 6, 2017 • edited Loading

gmaxwell commented Jul 9, 2017 • edited Loading

TheBlueMatt commented Jul 10, 2017

jnewbery commented Jul 10, 2017

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Jul 11, 2017

theuni commented Jul 11, 2017

gmaxwell commented Jul 11, 2017

gmaxwell left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 2, 2017

theuni commented Aug 16, 2017

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRubin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Jul 6, 2017 •

edited

Loading

gmaxwell commented Jul 9, 2017 •

edited

Loading