addrman: Make addrman a top-level component #20228

pull jnewbery wants to merge 6 commits into bitcoin:master from jnewbery:2020-10-addrman changing 11 files +74 −107
  1. jnewbery commented at 9:43 am on October 23, 2020: member

    Addrman is currently a member variable of connman. Make it a top-level component with lifetime owned by node.context, and add a reference to addrman in peerman. This allows us to eliminate some functions in connman that are simply forwarding requests to addrman, and simplifies the connman-peerman interface.

    By constructing the addrman in init, we can also add parameters to the ctor, which allows us to test it better. See #20233, where we enable consistency checking for addrman in our functional tests.

  2. jnewbery added the label P2P on Oct 23, 2020
  3. jnewbery added the label Refactoring on Oct 23, 2020
  4. MarcoFalke commented at 10:38 am on October 23, 2020: member

    does it conceptually make sense to have an addrman without connman or connman without addrman? I’d say no.

    This allows us to eliminate some functions in connman that are simply forwarding

    An alternative to achieve that would be to let PeerMan steal a reference to addrman from connman (and assign it to m_addrman)

  5. Saibato commented at 10:57 am on October 23, 2020: contributor

    This allows us to eliminate some functions in connman that are simply forwarding

    In general to have things double or more if ( ,,,) or detrimental decision processes in the same code is a good design in industry where u can kill ppl easy, if there is no double check and just one bit ( that could be just by chance or external forces have flipped ) decides over mayhem.

    In financials we used to do that the same and wrote “bad ugly code”. bcs u get no bonus at the years end, if the code “looks” good but your customers got screwed. Unless that was ur biz.

  6. jnewbery commented at 11:47 am on October 23, 2020: member

    does it conceptually make sense to have an addrman without connman or connman without addrman? I’d say no.

    Why not? If -connect is set, then we don’t use addrman for choosing outbound connections (see m_use_addrman_outgoing inside connman). In general, I think being able to run with different components disabled enforces strict separation between components and means there can’t be unintentional coupling between them.

  7. jnewbery renamed this:
    [addrman] Make addrman a top-level component
    addrman: Make addrman a top-level component
    on Oct 23, 2020
  8. jnewbery commented at 5:52 pm on October 23, 2020: member
    Fixed whitespace linter issue
  9. jnewbery force-pushed on Oct 23, 2020
  10. DrahtBot commented at 9:25 pm on October 23, 2020: 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:

    • #21527 (NOMERGE: net_processing: orphan handling changes by ajtowns)
    • #21244 (Move GetDataDir to ArgsManager by kiminuo)
    • #20966 (banman: save the banlist in a JSON format on disk by vasild)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    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.

  11. MarcoFalke commented at 6:58 am on October 24, 2020: member
    This doesn’t compile on gcc4.8, but we’ll drop support for that soon anyway, so the failure can be ignored/rebased out when 22.0 is branched off.
  12. jnewbery force-pushed on Oct 24, 2020
  13. jnewbery commented at 8:26 am on October 24, 2020: member

    This doesn’t compile on gcc4.8

    Hopefully fixed

  14. jnewbery force-pushed on Oct 27, 2020
  15. DrahtBot added the label Needs rebase on Nov 3, 2020
  16. jnewbery force-pushed on Nov 3, 2020
  17. jnewbery commented at 9:02 am on November 3, 2020: member
    rebased
  18. DrahtBot removed the label Needs rebase on Nov 3, 2020
  19. amitiuttarwar commented at 6:31 pm on November 12, 2020: contributor
    concept ACK
  20. jnewbery commented at 11:28 am on November 19, 2020: member
    rebased
  21. jnewbery force-pushed on Nov 19, 2020
  22. DrahtBot added the label Needs rebase on Nov 19, 2020
  23. jnewbery force-pushed on Nov 19, 2020
  24. jnewbery force-pushed on Nov 19, 2020
  25. jnewbery commented at 5:51 pm on November 19, 2020: member
    Rebased
  26. DrahtBot removed the label Needs rebase on Nov 19, 2020
  27. in src/net.h:248 in c98ddf89f7 outdated 
    251@@ -252,7 +252,7 @@ class CConnman
252         m_onion_binds = connOptions.onion_binds;
253     }
254 
255-    CConnman(uint64_t seed0, uint64_t seed1, bool network_active = true);
    adamjonas commented at 10:09 pm on December 1, 2020:

    Look like the fuzzer is complaining about this and needs to be updated to take this additional arg type:

    0test/fuzz/connman.cpp:26:14: error: no matching constructor for initialization of 'CConnman'
1    CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeBool()};
2             ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3./net.h:255:5: note: candidate constructor not viable: no known conversion from 'bool' to 'CAddrMan &' for 3rd argument
4    CConnman(uint64_t seed0, uint64_t seed1, CAddrMan& addrman, bool network_active = true);
5    ^
6./net.h:187:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 3 were provided
7class CConnman
8      ^
91 error generated.
    jnewbery commented at 12:59 pm on December 4, 2020:
    Thanks! Should be fixed now.
  28. jnewbery force-pushed on Dec 4, 2020
  29. jnewbery commented at 12:59 pm on December 4, 2020: member
    Rebased and fixed fuzz build errors.
  30. jnewbery commented at 12:15 pm on December 7, 2020: member
    rebased
  31. jnewbery force-pushed on Dec 7, 2020
  32. DrahtBot added the label Needs rebase on Dec 9, 2020
  33. jnewbery commented at 11:51 am on December 9, 2020: member
    rebased
  34. jnewbery force-pushed on Dec 9, 2020
  35. DrahtBot removed the label Needs rebase on Dec 9, 2020
  36. DrahtBot added the label Needs rebase on Dec 10, 2020
  37. jnewbery commented at 10:11 am on December 10, 2020: member
    rebased
  38. jnewbery force-pushed on Dec 10, 2020
  39. DrahtBot removed the label Needs rebase on Dec 10, 2020
  40. DrahtBot added the label Needs rebase on Dec 15, 2020
  41. jnewbery force-pushed on Dec 15, 2020
  42. jnewbery commented at 2:43 pm on December 15, 2020: member
    rebased
  43. jnewbery commented at 7:25 pm on December 18, 2020: member
    rebased
  44. jnewbery force-pushed on Dec 18, 2020
  45. DrahtBot removed the label Needs rebase on Dec 18, 2020
  46. DrahtBot added the label Needs rebase on Jan 2, 2021
  47. jnewbery commented at 2:19 pm on January 2, 2021: member
    Rebased
  48. jnewbery force-pushed on Jan 2, 2021
  49. DrahtBot removed the label Needs rebase on Jan 2, 2021
  50. ajtowns commented at 3:08 am on January 6, 2021: member

    I don’t think having addrman referenced from both peerman and connman makes much sense – peerman already has a reference to connman, so avoiding the layering can be done just by letting peerman access connman’s addrman directly – m_connman.AddrMan().do_something(), and that also avoids the potential for peerman and connman somehow getting their addrmans out of sync.

    As far as better testing of addrman goes, I think that’s easy to do without this PR or the above alternative, see #20233 (comment)

    As far as disabling addrman goes, I’m not convinced that’s a meaningful benefit (most people are going to use it enabled, and I think the memory saving if done at runtime rather than compiletime would be insignificant), and if it was, it can also be done via connman rather than doing it in init/node and then passing that on to both connman and peerman.

  51. jnewbery commented at 9:40 am on January 6, 2021: member

    I’ve left a comment on the linked PR here: #20233 (comment). I’ll copy the parts that are relevant here:

    I don’t think there’s any need for this to be based on #20228 – passing a “check_addrman” from init to CConnman seems to work fine, see ajtowns/bitcoin@202101-addrman-check (commits) eg.

    Sure, we could continue to pass function calls and (now) ctor parameters through CConnman, but I don’t see why that is desirable. Connman uses addrman, but there’s no reason for it to have an addrman. Addrman is also used by peerman and the RPCs. Connman’s resposibility is to open and maintain connections. Storing and maintaining our map of accessible nodes on the network is a separate responsibility.

    There was also some discussion here: http://www.erisian.com.au/bitcoin-core-dev/log-2020-12-16.html#l-585 about being able to manage addrman more directly. Again, in that case it’d be easier if RPC directly held a handle to the addrman instead of passing messages through connman.

    For your specific objections here:

    avoids the potential for peerman and connman somehow getting their addrmans out of sync.

    I don’t see how that could happen. addrman gets constructed and passed to the connman and peerman ctors to be saved as a reference a few lines later. How could those possible get out of sync?

    As far as disabling addrman goes, I’m not convinced that’s a meaningful benefit (most people are going to use it enabled, and I think the memory saving if done at runtime rather than compiletime would be insignificant), and if it was, it can also be done via connman rather than doing it in init/node and then passing that on to both connman and peerman.

    I agree that most people aren’t going run without addrman, or that the memory savings are meaningful. However, I think having it as an option is beneficial from an architectural standpoint – being able to run with different components disabled enforces strict separation between components and means there can’t be unintentional coupling between them.

  52. DrahtBot added the label Needs rebase on Jan 7, 2021
  53. jnewbery commented at 5:36 pm on January 7, 2021: member
    rebased
  54. jnewbery force-pushed on Jan 7, 2021
  55. DrahtBot removed the label Needs rebase on Jan 7, 2021
  56. in src/node/context.h:15 in f36480df77 outdated 
    12@@ -13,6 +13,7 @@
13 class ArgsManager;
14 class BanMan;
15 class CBlockPolicyEstimator;
16+class CAddrMan;
    vasild commented at 11:18 am on January 12, 2021:

    nit: to follow the alphabetical order:

    0class CAddrMan;
1class CBlockPolicyEstimator;
    jnewbery commented at 12:46 pm on January 14, 2021:
    Oops. Fixed.
  57. in src/net.h:1130 in f36480df77 outdated 
    1092@@ -1096,7 +1093,7 @@ class CConnman
1093     std::vector<ListenSocket> vhListenSocket;
1094     std::atomic<bool> fNetworkActive{true};
1095     bool fAddressesInitialized{false};
1096-    CAddrMan addrman;
1097+    CAddrMan& addrman;
    vasild commented at 11:34 am on January 12, 2021:

    A member variable of type reference is actually a bare pointer. By assigning it to an object that lives outside of the class we loose the benefits of smart pointers and have to manage lifetime ourselves.

    What about defining addrman as std::shared_ptr in NodeContext, CConnman and PeerManager?

    vasild commented at 11:37 am on January 12, 2021:
    nit: m_addrman?
    jnewbery commented at 12:48 pm on January 14, 2021:

    shared_ptr implies shared ownership of a resource. In this case, CConnman and PeerManager don’t own the addrman, they just hold a reference to it.

    We already use this same model for holding references to mempool, chainman, conman, etc, where the resource is owned by NodeContext.

    jnewbery commented at 12:50 pm on January 14, 2021:
    I’d prefer not to rename this. addrman is used in >50 places in net.cpp/net.h, so updating the name would add a lot of noise to the diff for this PR.
    vasild commented at 1:20 pm on January 14, 2021:
    ok
    vasild commented at 1:40 pm on January 14, 2021:

    shared_ptr implies shared ownership of a resource. In this case, CConnman and PeerManager don’t own the addrman, they just hold a reference to it.

    shared_ptr would just destroy the object when its last instance is destroyed. The fact that we know that this is going to be the instance in NodeContext does not make it less useful.

    The problem with the bare pointer (or bare reference) is that it can become dangling. shared_ptr aims to fix exactly that problem.

    We already use this same model for holding references to mempool, chainman, conman, etc, where the resource is owned by NodeContext.

    Right, that can be improved with shared_ptr too.

    jnewbery commented at 2:51 pm on January 14, 2021:

    Another good reason to leave this as a reference is that it explicitly communicates that there must be a reference to an addrman, and that we can ‘dereference’ addrman throughout peerman without checking for nullness first.

    In any case, I agree that this discussion can be deferred to another issue/PR.

  58. in src/init.cpp:1405 in f36480df77 outdated 
    1400@@ -1398,7 +1401,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1401     ChainstateManager& chainman = *Assert(node.chainman);
1402 
1403     assert(!node.peerman);
1404-    node.peerman = std::make_unique<PeerManager>(chainparams, *node.connman, node.banman.get(),
1405+    node.peerman = std::make_unique<PeerManager>(chainparams, *node.connman, *node.addrman, node.banman.get(),
1406                                                  *node.scheduler, chainman, *node.mempool, ignores_incoming_txs);
    vasild commented at 11:47 am on January 12, 2021:

    Here we pass 5 members of node:

    0*node.connman, *node.addrman, node.banman.get(), *node.scheduler, *node.mempool

    What about just passing a pointer to node itself, i.e. store a pointer to the “parent” node class in PeerManager? And have node->ConnMan(), node->AddrMan(), etc getters.

    jnewbery commented at 12:52 pm on January 14, 2021:
    Because net_processing doesn’t depend on node/context (and adding that would be a circular dependency between net_processing and node/context). In fact, a NodeContext object isn’t even needed to construct a PeerManager object - see denialofservice_tests.cpp.
    vasild commented at 1:28 pm on January 14, 2021:
    ok
  59. in src/test/denialofservice_tests.cpp:120 in f36480df77 outdated 
    129@@ -129,8 +130,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
130     BOOST_CHECK(dummyNode1.fDisconnect == true);
131     SetMockTime(0);
132 
133-    bool dummy;
134-    peerLogic->FinalizeNode(dummyNode1, dummy);
135+    peerLogic->FinalizeNode(dummyNode1);
    vasild commented at 2:11 pm on January 12, 2021:
    Just an observation: before this patch AddrMan::Connected() would not have been called here and it will be called after this patch. Also PeerManager::FinalizeNode() would try to dereference PeerManager::m_addrman. It is fine, as far as I can see.
    jnewbery commented at 12:54 pm on January 14, 2021:
    Yes, this is a slightly different ordering, but I agree that it’s fine. Everything between here and the end of Finalize is just internal net_processing book-keeping.
  60. vasild commented at 2:12 pm on January 12, 2021: member
    Concept ACK f36480df7785fb9bb81027b5d59218689a11f0c8
  61. DrahtBot added the label Needs rebase on Jan 13, 2021
  62. jnewbery force-pushed on Jan 14, 2021
  63. jnewbery commented at 12:45 pm on January 14, 2021: member
    Rebased now that the fuzz conflicts should no longer be a problem (#20828)
  64. jnewbery force-pushed on Jan 14, 2021
  65. jnewbery commented at 12:55 pm on January 14, 2021: member
    Thanks for the review @vasild - I’ve addressed all your comments.
  66. vasild approved
  67. vasild commented at 1:34 pm on January 14, 2021: member

    ACK a4a0bdab0bf4b47ccb049982197cbea59fad1e61

    I think the usage of shared_ptr would benefit this code, but I ACK it nevertheless because the pattern of using one unique_ptr and a few bare pointers to its contents is already used elsewhere. Don’t consider that discussion as a blocker for this PR.

  68. DrahtBot removed the label Needs rebase on Jan 14, 2021
  69. DrahtBot added the label Needs rebase on Mar 12, 2021
  70. jnewbery commented at 9:33 am on March 19, 2021: member
    Rebased
  71. jnewbery force-pushed on Mar 19, 2021
  72. vasild approved
  73. vasild commented at 9:57 am on March 19, 2021: member
    ACK cfbf35efb6acf182a31b57220b9001a1459c937c
  74. DrahtBot removed the label Needs rebase on Mar 19, 2021
  75. sipa commented at 3:07 am on March 20, 2021: member
    utACK cfbf35efb6acf182a31b57220b9001a1459c937c
  76. in src/test/util/setup_common.cpp:192 in 8f3d35a83a outdated 
    187@@ -187,8 +188,9 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
188         throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
189     }
190 
191+    m_node.addrman = std::make_unique<CAddrMan>();
    MarcoFalke commented at 8:16 am on March 20, 2021:

    8f3d35a83affc7061e6bf9b9e6be6e9861c84632:

    Any reason to implicitly deconstruct this last, as opposed to in the same order that init deconstructs it? This will leave a “dangling” addrman in memory after the datadir (and addrman.dat) is cleared.

    jnewbery commented at 9:42 am on March 20, 2021:
    Just to be clear, are you suggesting a ~TestingSetup() dtor that destructs the components in reverse order, like in Shutdown()? That seems reasonable to me.
    MarcoFalke commented at 9:51 am on March 20, 2021:
    No, just add m_node.addrman.reset() to where the other node members are reset.
    jnewbery commented at 10:25 am on March 20, 2021:
    Done. Seems odd to me that the components are constructed in the TestingSetup() ctor, but destructed in the ChainTestingSetup() destructor, but I’ve followed the existing pattern.
  77. in src/test/fuzz/connman.cpp:131 in 48f96385c8 outdated 
    127@@ -128,9 +128,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
128             [&] {
129                 connman.SetNetworkActive(fuzzed_data_provider.ConsumeBool());
130             },
131-            [&] {
132-                connman.SetServices(random_service, ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS));
    MarcoFalke commented at 8:24 am on March 20, 2021:

    48f96385c809a3e1d957538db8864ac068aca7eb

    When removing this here, it would be nice to update the addrman fuzz target to use ConsumeWeakEnum

    jnewbery commented at 10:20 am on March 20, 2021:
    Improving the addrman fuzzer seems out of scope of this PR (since it doesn’t change the addrman interface).
  78. in src/test/fuzz/connman.cpp:101 in 61aecae0da outdated 
     97@@ -98,9 +98,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman)
 98             [&] {
 99                 (void)connman.GetNodeCount(fuzzed_data_provider.PickValueInArray({ConnectionDirection::None, ConnectionDirection::In, ConnectionDirection::Out, ConnectionDirection::Both}));
100             },
101-            [&] {
102-                connman.MarkAddressGood(random_address);
    MarcoFalke commented at 8:26 am on March 20, 2021:

    61aecae0dafcc4c050343ef9c3e0242a11614cd9

    what is the point of keeping the unused variable?

    jnewbery commented at 10:20 am on March 20, 2021:
    Done (also removed the unused random_service variable)
  79. in src/net_processing.cpp:998 in cfbf35efb6 outdated 
     991@@ -993,8 +992,10 @@ void PeerManagerImpl::FinalizeNode(const CNode& node, bool& fUpdateConnectionTim
 992 
 993     if (node.fSuccessfullyConnected && misbehavior == 0 &&
 994         !node.IsBlockOnlyConn() && !node.IsInboundConn()) {
 995-        // Only change visible addrman state for outbound, full-relay peers
 996-        fUpdateConnectionTime = true;
 997+        // Only change visible addrman state for full outbound peers.  We don't
 998+        // call Connected() for feeler connections since they don't have
 999+        // fSuccessfullyConnected set.
1000+        m_addrman.Connected(node.addr);
    MarcoFalke commented at 8:36 am on March 20, 2021:

    cfbf35efb6acf182a31b57220b9001a1459c937c

    I am not a fan of moving this into the cs_main scope. The goal should be to limit cs_main to validation, not to accidentally extend it to cover addrman and other components.

    jnewbery commented at 10:21 am on March 20, 2021:
    In general I agree with reducing cs_main scope, but I think doing that in Finalize can happen in a follow-up PR.
    MarcoFalke commented at 10:56 am on March 20, 2021:

    Agree, but does it need to be made worse?

     0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 0fa201a047..619693536b 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -972,6 +972,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
 5 void PeerManagerImpl::FinalizeNode(const CNode& node)
 6 {
 7     NodeId nodeid = node.GetId();
 8+    {
 9     LOCK(cs_main);
10     int misbehavior{0};
11     {
12@@ -990,14 +991,6 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
13     if (state->fSyncStarted)
14         nSyncStarted--;
15 
16-    if (node.fSuccessfullyConnected && misbehavior == 0 &&
17-        !node.IsBlockOnlyConn() && !node.IsInboundConn()) {
18-        // Only change visible addrman state for full outbound peers.  We don't
19-        // call Connected() for feeler connections since they don't have
20-        // fSuccessfullyConnected set.
21-        m_addrman.Connected(node.addr);
22-    }
23-
24     for (const QueuedBlock& entry : state->vBlocksInFlight) {
25         mapBlocksInFlight.erase(entry.hash);
26     }
27@@ -1022,6 +1015,14 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
28         assert(m_wtxid_relay_peers == 0);
29         assert(m_txrequest.Size() == 0);
30     }
31+    } // cs_main
32+    if (node.fSuccessfullyConnected && misbehavior == 0 &&
33+        !node.IsBlockOnlyConn() && !node.IsInboundConn()) {
34+        // Only change visible addrman state for full outbound peers.  We don't
35+        // call Connected() for feeler connections since they don't have
36+        // fSuccessfullyConnected set.
37+        m_addrman.Connected(node.addr);
38+    }
39     LogPrint(BCLog::NET, "Cleared nodestate for peer=%d\n", nodeid);
40 }
41
    jnewbery commented at 12:24 pm on March 20, 2021:
    I’m really not convinced that it matters, but I’ve taken your diff.
    MarcoFalke commented at 3:09 pm on March 20, 2021:
    Sorry, I didn’t type make after writing the diff. Looks like ci doesn’t like me.
    jnewbery commented at 4:40 pm on March 20, 2021:
    ah indeed. Fails compilation because misbehavior isn’t in the outer scope. I’ve now fixed.
  80. MarcoFalke commented at 8:38 am on March 20, 2021: member

    I looked at cfbf35efb6acf182a31b57220b9001a1459c937c 🐣

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3I looked at cfbf35efb6acf182a31b57220b9001a1459c937c 🐣
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjQXgwAifz7QTdA8HDHM9fKlC8qGtT0ayvExXLvJoVxixHPY+IecXmTOr3rcQws
 8/84tejW8Oylc1SCcKcw8Pdr2VF4cYZbCK+oo3Xj1NCzJlraNJ5i2nsnq0mzZJdUI
 96OUyCOPa2c/a9lb7cscSHm8SIymYYMi6YwMW461BcO20xdEC3N3a4Eb0p7ECvJZU
109i5RSnLntHrEON5x1NSkzMPq1rL5iXzb7oLSFDgjlJD03t6VuKRF8CcTURGUQo1y
11TV+zlETUgzvMC7/uM0M4OoJPkUChleH1Sc48SMYGWpB1FZnkbMwt49Tpyvd4vOzc
12wPZwnDCwnT7xbPhkq4uJfaUZ9sCi7JAb637gKtseOIYrH6NbxfXzMsMv7vzOiT0r
13CKsOOv6xonhgKre8UC4KIR/PtKP2zqzN9AV1/9Voh6sMqPNHvYUZK8I7W4UYFoEk
14aAR6Z+Az2ADYUhkxI11Vk0ou2/m7iwSURenbOWikPMu5QkS+MG0OJYaoOhJQ6kG+
15gfAqzFs0
16=nQs9
17-----END PGP SIGNATURE-----

    Timestamp of file with hash f3498d80e5b4a7afbc97ad18b42947699359862c306a91e93f1dbb3d43a2cb07 -

  81. jnewbery force-pushed on Mar 20, 2021
  82. jnewbery force-pushed on Mar 20, 2021
  83. jnewbery force-pushed on Mar 20, 2021
  84. [net] Construct addrman outside connman 
    node.context owns the CAddrMan. CConnman holds a reference to
the CAddrMan.
    1c25adf6d2
  85. [net_processing] Keep addrman reference in PeerManager 392a95d393
  86. [net] remove CConnman::SetServices 
    It just forwards calls to CAddrMan::SetServices.
    8073673dbc
  87. [net] remove CConnman::MarkAddressGood 
    It just forwards calls to CAddrMan::Good.
    bcd7f30b79
  88. [net] remove CConnman::AddNewAddresses 
    It just forwards calls to CAddrMan::Add.
    7c4cc67c0c
  89. jnewbery force-pushed on Mar 20, 2021
  90. jnewbery commented at 10:26 am on March 20, 2021: member
    Thanks for the review @MarcoFalke. I’ve addressed all of your comments.
  91. jnewbery commented at 12:24 pm on March 20, 2021: member
    @MarcoFalke - I’ve taken your suggested diff.
  92. jnewbery force-pushed on Mar 20, 2021
  93. MarcoFalke commented at 9:59 am on March 22, 2021: member

    re-ACK 06653e4aaccc84dc7773d3888f687cf44e20abcd 🕸

    Only changes:

    • Add missing m_node.addrman.reset() in tests
    • Remove unused symbols random_service and random_address in tests
    • Add commit 06653e4aac

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 06653e4aaccc84dc7773d3888f687cf44e20abcd 🕸
 4
 5Only changes:
 6* Add missing m_node.addrman.reset() in tests
 7* Remove unused symbols random_service and random_address in tests
 8* Add commit 06653e4aac
 9-----BEGIN PGP SIGNATURE-----
10
11iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
12pUh2TQwAuta5eICUDWp5ZOK6mqTbXME6W4oZ2tq4JrradpQgatb0gd7Cvl9voHBS
1395imHMkW8gS8qyTS5lCJfmaGZs6GsANQ4WY/SSMDAYPZHR7UUnicrlP5hBE1zUP0
14olJXiIu/cNeTibA2gImd1bwxW11h3UII6R8fZ91lLHBTlsakqgX84n1fsRmOHdn7
15UG4S6VDK9qceJ/dNTbww56vEn7hnrE9t0FUmFNqb6Yb5LgkG11Bw9XA71urWcFjc
162nvl4CBmZI73SzCyPtcyQ8iYuL6INtg6Rt6BtV6ff8Xm9cYB28cvkP6PFwjp4SHw
17Zns+ZiBshcMWgODAvaetrCQNVHo597lW94zfCrSRrhsIVWw9TK6sTJRpkJRLMNxv
186ASeEetu7cME/nFHzabA+PV6nhQIcHoaczCZfdkiY+n1u83KJGvyEl3c12LW/rLT
19yjNU8VV/34GM7Fr+HmjK1TJx3aBdcurK+4sNorAr+7JiAovtqTTjEavKYnoeftmm
20N1OnbouT
21=MOU8
22-----END PGP SIGNATURE-----

    Timestamp of file with hash 04be068a42318cb4f710b8b8d5521a5bda7090c95bd0fa710e8d6dbc7a70894e -

  94. MarcoFalke commented at 10:01 am on March 22, 2021: member
    Could squash the last two commits and mention that --color-moved=dimmed-zebra can be used on them for review?
  95. [net] remove fUpdateConnectionTime from FinalizeNode 
    PeerManager can just call directly into CAddrMan::Connected() now.
    3fc06d3d7b
  96. jnewbery force-pushed on Mar 22, 2021
  97. jnewbery commented at 10:26 am on March 22, 2021: member

    Could squash the last two commits

    Done

    and mention that –color-moved=dimmed-zebra can be used on them for review?

    I tried this and it wasn’t very useful - there are only a couple of lines that are moved without changing.

  98. MarcoFalke commented at 10:33 am on March 22, 2021: member

    re-ACK 3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682 only change is squash 🏀

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682 only change is squash 🏀
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhYRgv+NLihAeug6hwKodKc6lvG+BPtQdQqV3afHzM2iCE9UzX8UPEWDomfRpNr
 8/TCwhhMSaT1yKoWh3S9ePwDiVqEx0g9UPfJIfB0bZ20N94tDZY/L/9NXk4jxNlV7
 90uqA5jtS93p3QggaVRhGOuOYUBXlgFC+kROYSLgAK9wuQmDI1X9NGlgx1h1tC3yt
10qrGvDAuL+x1ALFYH6fOkAZHEwntte+lHPwymA9wktdPCntsmQaM7WimftLMK+z/H
1147EYxadADEKJ84aLzeLzU2NJjb+Ow5OwEvsH6DdwcAazCrN5TnSWvpEso+EXP+qY
12/CN7Iyy/beRbZsA0inj81zKs2P9k5sKvJuKvzdZLY+HjHcOBZnvB4TqgrSrlQ/sf
139smWO+ndxPc1vjakxxkB7ZFTVL+ju/sc9S14eD6zx9WAldtTeafz6/3YePo7IQlB
14gq6AIW8HVsTDV0BHgjG3P2H+TclKtdT5pykhN781QhMuHVvfvkKs7p+dfCv7Lwxi
15ugjJlKIA
16=o4vy
17-----END PGP SIGNATURE-----

    Timestamp of file with hash a02bd2eb33e158baac0fe590960dabd93a73914fa44637c90c6b2566e605a7f5 -

  99. jnewbery commented at 11:02 am on March 22, 2021: member
    @vasild @sipa: Do you mind reACKing? There have only been minor changes since your last reviews.
  100. MarcoFalke referenced this in commit d400e672a0 on Mar 23, 2021
  101. sidhujag referenced this in commit 63198f2302 on Mar 23, 2021
  102. vasild approved
  103. vasild commented at 9:21 am on March 26, 2021: member
    ACK 3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682
  104. jnewbery commented at 9:53 am on March 30, 2021: member
    @sipa do you mind rereviewing this? It has two ACKs on the current branch.
  105. MarcoFalke merged this on Mar 30, 2021
  106. MarcoFalke closed this on Mar 30, 2021
  107. jnewbery deleted the branch on Mar 30, 2021
  108. sidhujag referenced this in commit c9799f13a0 on Mar 30, 2021
  109. DrahtBot locked this on Aug 16, 2022


jnewbery MarcoFalke Saibato DrahtBot amitiuttarwar adamjonas ajtowns vasild sipa

Labels
Refactoring P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me