refactor: move CConnman::RelayInv{Filtered} to PeerManager, remove CConnman::RelayTransaction #5977

kwvg · 2024-04-09T13:27:45Z

Additional information

Dependency for backport: merge bitcoin#20769, #19499, #23575, #23695, #21160, #24692, partial bitcoin#20196, #25176, merge bitcoin-core/gui#526 (networking backports: part 4) #5982
Unfortunately, we are stuck with using the unique_ptr const-ref of PeerManager for initializing {CJ, LLMQ}Context due to some unit tests depending on Dash-specific entities but based on a testing setup that doesn't initialize PeerManager (specifically validation_chainstatemanager_tests, which uses ChainTestingSetup, as opposed to TestingSetup, which initializes PeerManager).

Attempting to dereference it earlier will result in crashing (see CI pipeline here)

Breaking Changes

None observed.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation (note: N/A)
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

src/coinjoin/context.cpp

github-actions · 2024-04-12T15:17:30Z

This pull request has conflicts, please rebase.

knst

see comment about quorumInstantSendManager

knst · 2024-04-19T10:13:55Z

src/llmq/blockprocessor.h

    CDeterministicMNManager& m_dmnman;
    CEvoDB& m_evoDb;
+    const std::unique_ptr<PeerManager>& m_peerman;


I don't like const std::unique_ptr<PeerManager>& spreading all over code base, but overall PR looks fine except quorumInstantSendManager case

knst · 2024-04-19T10:18:23Z

src/llmq/context.cpp

@@ -43,7 +43,7 @@ LLMQContext::LLMQContext(CChainState& chainstate, CConnman& connman, CDeterminis
    }()},
    isman{[&]() -> llmq::CInstantSendManager* const {
        assert(llmq::quorumInstantSendManager == nullptr);
-        llmq::quorumInstantSendManager = std::make_unique<llmq::CInstantSendManager>(*llmq::chainLocksHandler, chainstate, connman, *llmq::quorumManager, *sigman, *shareman, sporkman, mempool, mn_sync, unit_tests, wipe);
+        llmq::quorumInstantSendManager = std::make_unique<llmq::CInstantSendManager>(*llmq::chainLocksHandler, chainstate, connman, *llmq::quorumManager, *sigman, *shareman, sporkman, mempool, mn_sync, peerman, unit_tests, wipe);


can you describe a situation when exactly does it happen?

ProcessPendingInstantSendLocks > ProcessPendingInstantSendLocks[1] >
ProcessInstantSendLock > RelayInvFiltered

The separate threads won't travel like that until no messages received -> so, peerman won't be nullptr

Crashing was observed in feature_dip3_v19 and feature_llmq_evo without those changes (tested on d02dcb0)

2024-04-23T14:47:29.534746Z (mocktime: 2014-12-04T17:29:03Z) [ msghand] [net_processing.cpp:5470] [operator()] SendMessages -- queued inv: qsigrec 7239135df98b46c8ab03e684ef15b90c4b6bd5fe8aa757b11105c9c880f347e0 index=0 peer=1 2024-04-23T14:47:29.534770Z (mocktime: 2014-12-04T17:29:03Z) [ msghand] [net.cpp:4033] [PushMessage] sending inv (37 bytes) peer=1 2024-04-23T14:47:29.534845Z (mocktime: 2014-12-04T17:29:03Z) [ msghand] [net_processing.cpp:5470] [operator()] SendMessages -- queued inv: qsigrec 7239135df98b46c8ab03e684ef15b90c4b6bd5fe8aa757b11105c9c880f347e0 index=0 peer=2 2024-04-23T14:47:29.534861Z (mocktime: 2014-12-04T17:29:03Z) [ msghand] [net.cpp:4033] [PushMessage] sending inv (37 bytes) peer=2 2024-04-23T14:47:29.534956Z (mocktime: 2014-12-04T17:29:03Z) [ msghand] [net_processing.cpp:5470] [operator()] SendMessages -- queued inv: qsigrec 7239135df98b46c8ab03e684ef15b90c4b6bd5fe8aa757b11105c9c880f347e0 index=0 peer=4 2024-04-23T14:47:29.534971Z (mocktime: 2014-12-04T17:29:03Z) [ msghand] [net.cpp:4033] [PushMessage] sending inv (37 bytes) peer=4 2024-04-23T14:47:29.535221Z (mocktime: 2014-12-04T17:29:03Z) [ msghand] [net_processing.cpp:3146] [ProcessMessage] received: inv (37 bytes) peer=2 2024-04-23T14:47:29.535239Z (mocktime: 2014-12-04T17:29:03Z) [ msghand] [net_processing.cpp:3670] [ProcessMessage] got inv: qsigrec 7239135df98b46c8ab03e684ef15b90c4b6bd5fe8aa757b11105c9c880f347e0 have peer=2 2024-04-23T14:47:29.566421Z (mocktime: 2014-12-04T17:29:03Z) [ isman] [llmq/instantsend.cpp:951] [ProcessPendingInstantSendLocks] CInstantSendManager::ProcessPendingInstantSendLocks -- verified locks. count=0, alreadyVerified=1, vt=0, nodes=0 2024-04-23T14:47:29.566439Z (mocktime: 2014-12-04T17:29:03Z) [ isman] [llmq/instantsend.cpp:996] [ProcessInstantSendLock] CInstantSendManager::ProcessInstantSendLock -- txid=62fd9a36bf4b11c4e7c0bfa9ffe80f6dc66fef54136c2d241a478daae1f880e6, islock=c2e83196cf1af12277b225ff043b4fd[...] 2024-04-23T14:47:29.566499Z (mocktime: 2014-12-04T17:29:03Z) [ isman] [llmq/instantsend.cpp:1226] [RemoveNonLockedTx] CInstantSendManager::RemoveNonLockedTx -- txid=62fd9a36bf4b11c4e7c0bfa9ffe80f6dc66fef54136c2d241a478daae1f880e6, retryChildren=1, retryChildrenCount=0 2024-04-23T14:47:29.566591Z (mocktime: 2014-12-04T17:29:03Z) [ isman] [stacktraces.cpp:528] [PrintCrashInfo] Posix Signal: Segmentation fault No debug information available for stacktrace. You should add debug information and then run: dashd -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaacwiyltnbscaudponuxqictnftw4ylmhiqfgzlhnvsw45dboruw63ramzqxk3duaaaa====

knst

LGTM cbdc34d

PastaPastaPasta · 2024-04-23T14:54:23Z

Range diff here

 -:  ---------- >  1:  10a006e626 test: disable ipv6 tests for now
 -:  ---------- >  2:  6423f6bc5d Merge #21395: Net processing: Remove unused CNode.address member
 -:  ---------- >  3:  1a07e04a0e Merge #21370: Use C++11 member initializer in CNodeState
 -:  ---------- >  4:  666e6e2015 Merge #20406: util: Avoid invalid integer negation in FormatMoney and ValueFromAmount
 -:  ---------- >  5:  6ca0eb189e Merge #21007: bitcoind: Add -daemonwait option to wait for initialization
 -:  ---------- >  6:  99f09a0be6 Merge #20040: wallet: Refactor OutputGroups to handle fees and spending eligibility on grouping
 -:  ---------- >  7:  3325620ce1 Merge #21418: contrib: Make systemd invoke dependencies only when ready
 -:  ---------- >  8:  dd92322962 Merge #21447: Always add -daemonwait to known command line arguments
 -:  ---------- >  9:  b8aec5cb86 Merge #20197: p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage
 -:  ---------- > 10:  daacafb539 Merge #21426: rpc: remove scantxoutset EXPERIMENTAL warning
 -:  ---------- > 11:  16ccb90bbb Merge bitcoin/bitcoin#21902: refactor: Remove useless extern keyword
 -:  ---------- > 12:  f3bc9535da Merge bitcoin/bitcoin#22187: test: Add sync_blocks in wallet_orphanedreward.py
 -:  ---------- > 13:  cc70886b4d Merge bitcoin/bitcoin#21795: fuzz: Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders)
 -:  ---------- > 14:  b6dbd8bd7d Merge bitcoin/bitcoin#22092: test: convert documentation into type annotations
 -:  ---------- > 15:  2996daad4c (followup) bitcoin#19940
 -:  ---------- > 16:  348f4a8b9e Merge bitcoin/bitcoin#22121: doc: Various validation doc fixups
 -:  ---------- > 17:  26ff28a028 Merge bitcoin/bitcoin#21353: interfaces: Stop exposing wallet destdata to gui
 -:  ---------- > 18:  6c242da723 Merge bitcoin/bitcoin#21936: fuzz: Terminate immediately if a fuzzing harness tries to create a TCP socket (belt and suspenders)
 1:  818cca0780 = 19:  569efa0dd2 net: use ForEachNode instead of manually iterating through vNodes
 2:  a25b5df120 = 20:  fbb2748d58 net: move Relay{Inv, InvFiltered, Transaction} out of CConnman
 3:  82be8c308b = 21:  ff68ce3c47 net: retire CConnman::RelayTransaction, use PeerManager::RelayTransaction
 4:  7b5adc451b = 22:  7877aa05e6 trivial: cleanup unnecessary headers in context files
 5:  d02dcb0701 = 23:  b234a84358 net: move CConnman::RelayInv{Filtered} into PeerManager
 6:  6a86ebf425 = 24:  33c8f236a0 llmq: pass PeerManager to llmq::CInstantSendManager constructor
 7:  cbdc34d055 = 25:  11df052792 trivial: add `Asserts` for `m_peerman` pointer container uses

knst

utACK 11df052

github-actions · 2024-04-23T15:00:41Z

This pull request has conflicts, please rebase.

PastaPastaPasta

utACK 11df052

PastaPastaPasta · 2024-04-23T15:24:57Z

range-diff

 -:  ---------- >  1:  c47e9e78ed bench: drop leftover `boost::lexical_cast` benches, drop header from list
 -:  ---------- >  2:  9ae39f9ba7 qt: drop leftover `boost::function` usage in Qt, drop header from list
 -:  ---------- >  3:  fbc6bc88cf init: use local ArgsManager variable instead of global when possible
 -:  ---------- >  4:  65585e68e6 merge bitcoin#25550: remove note on arm cross-compilation from build-unix.md
 -:  ---------- >  5:  3265b54a2f merge bitcoin#24624: Avoid potential -Wdeprecated-enum-enum-conversion warnings
 -:  ---------- >  6:  740d25c062 merge bitcoin#24406: Fix Wambiguous-reversed-operator compiler warnings
 -:  ---------- >  7:  7bcc56c9b6 Merge bitcoin/bitcoin#22138: script: fix spelling linter raising spuriously on "invokable"
 -:  ---------- >  8:  00b828c0f5 Merge bitcoin/bitcoin#21939: refactor: Replace memset calls with array initialization
 -:  ---------- >  9:  b8b3c4c289 Merge bitcoin-core/gui#163: Peer details: replace Direction with Connection Type
 -:  ---------- > 10:  4d20cb7173 Merge #20373: refactor, net: Increase CNode data member encapsulation
 -:  ---------- > 11:  366ca98b39 Merge bitcoin/bitcoin#22144: Randomize message processing peer order
 -:  ---------- > 12:  74b20eb309 Merge bitcoin/bitcoin#22013: net: ignore block-relay-only peers when skipping DNS seed
 -:  ---------- > 13:  751c9e66c4 Merge bitcoin/bitcoin#21719: refactor: Add and use EnsureConnman in rpc code
 -:  ---------- > 14:  18169f4957 Merge #20786: net: [refactor] Prefer integral types in CNodeStats
 1:  569efa0dd2 = 15:  d54ba447a7 net: use ForEachNode instead of manually iterating through vNodes
 2:  fbb2748d58 = 16:  0323c6ca17 net: move Relay{Inv, InvFiltered, Transaction} out of CConnman
 3:  ff68ce3c47 = 17:  c3f1ac2291 net: retire CConnman::RelayTransaction, use PeerManager::RelayTransaction
 4:  7877aa05e6 = 18:  313a7e9a50 trivial: cleanup unnecessary headers in context files
 5:  b234a84358 ! 19:  bfd33cd2b4 net: move CConnman::RelayInv{Filtered} into PeerManager
    @@ src/net.h: private:
     
      ## src/net_processing.cpp ##
     @@ src/net_processing.cpp: public:
    -     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) override;
    +     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override;
          bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; }
          void SendPings() override;
     +    void RelayInv(CInv &inv, const int minProtoVersion) override;
    @@ src/rpc/misc.cpp: static RPCHelpMan sporkupdate()
          }
      
          const NodeContext& node = EnsureAnyNodeContext(request.context);
    --    if (!node.connman) {
    -+    if (!node.peerman) {
    -         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
    -     }
    +-    CConnman& connman = EnsureConnman(node);
    ++    PeerManager& peerman = EnsurePeerman(node);
      
    -@@ src/rpc/misc.cpp: static RPCHelpMan sporkupdate()
    +     // SPORK VALUE
          int64_t nValue = request.params[1].get_int64();
      
          // broadcast new spork
    --    if (node.sporkman->UpdateSpork(nSporkID, nValue, *node.connman)) {
    -+    if (node.sporkman->UpdateSpork(*node.peerman, nSporkID, nValue)) {
    +-    if (node.sporkman->UpdateSpork(nSporkID, nValue, connman)) {
    ++    if (node.sporkman->UpdateSpork(peerman, nSporkID, nValue)) {
              return "success";
          }
      
 6:  33c8f236a0 = 20:  35be4e2ebe llmq: pass PeerManager to llmq::CInstantSendManager constructor
 7:  11df052792 = 21:  f2fe39fadc trivial: add `Asserts` for `m_peerman` pointer container uses

PastaPastaPasta

re-utACK f2fe39f

UdjinM6

LGTM, utACK (assuming CI is happy too)

knst · 2024-04-23T15:44:06Z

Ranged diff:

$ gfd cbdc34d0553ed0a05c1d1456305f37806a30c3e3 f2fe39fadc96327cf0a93d018860a26a46ff57c1 
fp1=2dacfb08bdf02fdaf0740edac19435bfaa0e52d3
fp2=6194e454d7a9ed15b34bb75f9865005748b17b13
--- /dev/fd/63  2024-04-23 22:43:11.078103491 +0700
+++ /dev/fd/62  2024-04-23 22:43:11.078103491 +0700
@@ -1261,24 +1261,22 @@
      } else {
          throw JSONRPCError(RPC_INTERNAL_ERROR, "Error voting : " + exception.GetMessage());
 diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
-index dfb463eb3a..da6771c17b 100644
+index bbc2e4eec2..d011bb60f1 100644
 --- a/src/rpc/misc.cpp
 +++ b/src/rpc/misc.cpp
-@@ -213,7 +213,7 @@ static RPCHelpMan sporkupdate()
+@@ -214,13 +214,13 @@ static RPCHelpMan sporkupdate()
      }
  
      const NodeContext& node = EnsureAnyNodeContext(request.context);
--    if (!node.connman) {
-+    if (!node.peerman) {
-         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
-     }
+-    CConnman& connman = EnsureConnman(node);
++    PeerManager& peerman = EnsurePeerman(node);
  
-@@ -221,7 +221,7 @@ static RPCHelpMan sporkupdate()
+     // SPORK VALUE
      int64_t nValue = request.params[1].get_int64();
  
      // broadcast new spork
--    if (node.sporkman->UpdateSpork(nSporkID, nValue, *node.connman)) {
-+    if (node.sporkman->UpdateSpork(*node.peerman, nSporkID, nValue)) {
+-    if (node.sporkman->UpdateSpork(nSporkID, nValue, connman)) {
++    if (node.sporkman->UpdateSpork(peerman, nSporkID, nValue)) {
          return "success";
      }

knst

utACK f2fe39f

ForEachNode is publicly available, which will help us extract the functions from CConnman in a subsequent commit

…tion

Required to avoid crashes when calling RelayInvFiltered in situations where the PeerManager* atomic hasn't been set (possible when ProcessMessage isn't called, leaving the value unset, while a separate thread traverses the ProcessPendingInstantSendLocks > ProcessPendingInstantSendLocks[1] > ProcessInstantSendLock > RelayInvFiltered call chain). [1] - There is a function with the exact same name but with multiple arguments

, bitcoin#23695, bitcoin#21160, bitcoin#24692, partial bitcoin#20196, bitcoin#25176, merge bitcoin-core/gui#526 (networking backports: part 4) ab7ac1b partial bitcoin#25176: Fix frequent -netinfo JSON errors from null getpeerinfo#relaytxes (Kittywhiskers Van Gogh) c89799d merge bitcoin#24692: Follow-ups to bitcoin#21160 (Kittywhiskers Van Gogh) 33098ae merge bitcoin#21160: Move tx inventory into net_processing (Kittywhiskers Van Gogh) 24205d9 partial bitcoin#20196: fix GetListenPort() to derive the proper port (Kittywhiskers Van Gogh) 7f72009 merge bitcoin-core/gui#526: Add address relay/processed/rate-limited fields to peer details (Kittywhiskers Van Gogh) a9114f1 merge bitcoin#23695: Always serialize local timestamp for version msg (Kittywhiskers Van Gogh) d936c28 merge bitcoin#23575: Rework FillNode (Kittywhiskers Van Gogh) 6f8c730 merge bitcoin#19499: Make timeout mockable and type safe, speed up test (Kittywhiskers Van Gogh) 43a82bd merge bitcoin#20769: fixes "advertised address where nobody is listening" (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #5978 * Dependent on #5977 ## Breaking Changes None observed. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK ab7ac1b Tree-SHA512: 87bf5108bb80576c5bff8cd577add7800044da252fd18590e06a727f0bf70de94e2e9294b4412cdd9f1f6676472b0359902af361aaffc4c9ee299ad07d6af009

kwvg mentioned this pull request Apr 9, 2024

backport: merge bitcoin#21594, #21843, #22306, #22211, #22387, #21528, #22616, #22604, #22960, #23218 (networking backports: part 3) #5978

Merged

5 tasks

kwvg force-pushed the inv_refac branch from 3ddce9a to e9624e2 Compare April 9, 2024 19:58

kwvg mentioned this pull request Apr 10, 2024

backport: merge bitcoin#20769, #19499, #23575, #23695, #21160, #24692, partial bitcoin#20196, #25176, merge bitcoin-core/gui#526 (networking backports: part 4) #5982

Merged

5 tasks

kwvg added this to the 21 milestone Apr 10, 2024

knst reviewed Apr 10, 2024

View reviewed changes

src/coinjoin/context.cpp Outdated Show resolved Hide resolved

github-actions bot added the needs rebase label Apr 12, 2024

kwvg force-pushed the inv_refac branch from e9624e2 to c281216 Compare April 17, 2024 14:50

github-actions bot removed the needs rebase label Apr 17, 2024

kwvg force-pushed the inv_refac branch from c281216 to cbdc34d Compare April 18, 2024 10:02

kwvg marked this pull request as ready for review April 18, 2024 10:04

kwvg requested review from knst, UdjinM6 and PastaPastaPasta April 18, 2024 10:05

knst reviewed Apr 19, 2024

View reviewed changes

kwvg force-pushed the inv_refac branch from cbdc34d to 11df052 Compare April 23, 2024 14:37

knst approved these changes Apr 23, 2024

View reviewed changes

knst previously approved these changes Apr 23, 2024

View reviewed changes

github-actions bot added the needs rebase label Apr 23, 2024

PastaPastaPasta previously approved these changes Apr 23, 2024

View reviewed changes

kwvg dismissed stale reviews from PastaPastaPasta and knst via f2fe39f April 23, 2024 15:14

kwvg force-pushed the inv_refac branch from 11df052 to f2fe39f Compare April 23, 2024 15:14

github-actions bot removed the needs rebase label Apr 23, 2024

PastaPastaPasta approved these changes Apr 23, 2024

View reviewed changes

UdjinM6 approved these changes Apr 23, 2024

View reviewed changes

knst approved these changes Apr 23, 2024

View reviewed changes

kwvg added 7 commits April 23, 2024 16:06

net: use ForEachNode instead of manually iterating through vNodes

d54ba44

ForEachNode is publicly available, which will help us extract the functions from CConnman in a subsequent commit

net: move Relay{Inv, InvFiltered, Transaction} out of CConnman

0323c6c

net: retire CConnman::RelayTransaction, use PeerManager::RelayTransac…

c3f1ac2

…tion

trivial: cleanup unnecessary headers in context files

313a7e9

net: move CConnman::RelayInv{Filtered} into PeerManager

bfd33cd

trivial: add Asserts for m_peerman pointer container uses

f2fe39f

PastaPastaPasta merged commit 2b1c165 into dashpay:develop Apr 24, 2024
4 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move CConnman::RelayInv{Filtered} to PeerManager, remove CConnman::RelayTransaction #5977

refactor: move CConnman::RelayInv{Filtered} to PeerManager, remove CConnman::RelayTransaction #5977

kwvg commented Apr 9, 2024 •

edited

github-actions bot commented Apr 12, 2024

knst left a comment

knst Apr 19, 2024 •

edited

knst Apr 19, 2024

kwvg Apr 23, 2024 •

edited

kwvg Apr 23, 2024 •

edited

knst left a comment

PastaPastaPasta commented Apr 23, 2024

knst left a comment

github-actions bot commented Apr 23, 2024

PastaPastaPasta left a comment

PastaPastaPasta commented Apr 23, 2024

PastaPastaPasta left a comment •

edited

UdjinM6 left a comment

knst commented Apr 23, 2024 •

edited

knst left a comment

refactor: move CConnman::RelayInv{Filtered} to PeerManager, remove CConnman::RelayTransaction #5977

refactor: move CConnman::RelayInv{Filtered} to PeerManager, remove CConnman::RelayTransaction #5977

Conversation

kwvg commented Apr 9, 2024 • edited

Additional information

Breaking Changes

Checklist:

github-actions bot commented Apr 12, 2024

knst left a comment

Choose a reason for hiding this comment

knst Apr 19, 2024 • edited

Choose a reason for hiding this comment

knst Apr 19, 2024

Choose a reason for hiding this comment

kwvg Apr 23, 2024 • edited

Choose a reason for hiding this comment

kwvg Apr 23, 2024 • edited

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Apr 23, 2024

knst left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2024

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Apr 23, 2024

PastaPastaPasta left a comment • edited

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

knst commented Apr 23, 2024 • edited

knst left a comment

Choose a reason for hiding this comment

kwvg commented Apr 9, 2024 •

edited

knst Apr 19, 2024 •

edited

kwvg Apr 23, 2024 •

edited

kwvg Apr 23, 2024 •

edited

PastaPastaPasta left a comment •

edited

knst commented Apr 23, 2024 •

edited