addrman: Make addrman a top-level component #20228

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.

jnewbery added the label P2P on Oct 23, 2020

jnewbery added the label Refactoring on Oct 23, 2020

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)

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.

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.

jnewbery renamed this:
~~[addrman] Make addrman a top-level component~~
addrman: Make addrman a top-level component
on Oct 23, 2020

jnewbery commented at 5:52 PM on October 23, 2020: member

Fixed whitespace linter issue

jnewbery force-pushed on Oct 23, 2020

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.

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.

jnewbery force-pushed on Oct 24, 2020

jnewbery commented at 8:26 AM on October 24, 2020: member

This doesn't compile on gcc4.8

Hopefully fixed

jnewbery force-pushed on Oct 27, 2020

DrahtBot added the label Needs rebase on Nov 3, 2020

jnewbery force-pushed on Nov 3, 2020

jnewbery commented at 9:02 AM on November 3, 2020: member

rebased

DrahtBot removed the label Needs rebase on Nov 3, 2020

amitiuttarwar commented at 6:31 PM on November 12, 2020: contributor

concept ACK

jnewbery commented at 11:28 AM on November 19, 2020: member

rebased

jnewbery force-pushed on Nov 19, 2020

DrahtBot added the label Needs rebase on Nov 19, 2020

jnewbery force-pushed on Nov 19, 2020

jnewbery commented at 5:51 PM on November 19, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Nov 19, 2020

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:

test/fuzz/connman.cpp:26:14: error: no matching constructor for initialization of 'CConnman'
    CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeBool()};
             ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./net.h:255:5: note: candidate constructor not viable: no known conversion from 'bool' to 'CAddrMan &' for 3rd argument
    CConnman(uint64_t seed0, uint64_t seed1, CAddrMan& addrman, bool network_active = true);
    ^
./net.h:187:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 3 were provided
class CConnman
      ^
1 error generated.

jnewbery commented at 12:59 PM on December 4, 2020:

Thanks! Should be fixed now.

jnewbery force-pushed on Dec 4, 2020

jnewbery commented at 12:59 PM on December 4, 2020: member

Rebased and fixed fuzz build errors.

jnewbery commented at 12:15 PM on December 7, 2020: member

rebased

jnewbery force-pushed on Dec 7, 2020

DrahtBot added the label Needs rebase on Dec 9, 2020

jnewbery commented at 11:51 AM on December 9, 2020: member

rebased

jnewbery force-pushed on Dec 9, 2020

DrahtBot removed the label Needs rebase on Dec 9, 2020

DrahtBot added the label Needs rebase on Dec 10, 2020

jnewbery commented at 10:11 AM on December 10, 2020: member

rebased

jnewbery force-pushed on Dec 10, 2020

DrahtBot removed the label Needs rebase on Dec 10, 2020

DrahtBot added the label Needs rebase on Dec 15, 2020

jnewbery force-pushed on Dec 15, 2020

jnewbery commented at 2:43 PM on December 15, 2020: member

rebased

jnewbery commented at 7:25 PM on December 18, 2020: member

rebased

jnewbery force-pushed on Dec 18, 2020

DrahtBot removed the label Needs rebase on Dec 18, 2020

DrahtBot added the label Needs rebase on Jan 2, 2021

jnewbery commented at 2:19 PM on January 2, 2021: member

Rebased

jnewbery force-pushed on Jan 2, 2021

DrahtBot removed the label Needs rebase on Jan 2, 2021

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.

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.

DrahtBot added the label Needs rebase on Jan 7, 2021

jnewbery commented at 5:36 PM on January 7, 2021: member

rebased

jnewbery force-pushed on Jan 7, 2021

DrahtBot removed the label Needs rebase on Jan 7, 2021

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:

class CAddrMan;
class CBlockPolicyEstimator;

jnewbery commented at 12:46 PM on January 14, 2021:

Oops. Fixed.

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.

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:

*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

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.

vasild commented at 2:12 PM on January 12, 2021: member

Concept ACK f36480df7785fb9bb81027b5d59218689a11f0c8

DrahtBot added the label Needs rebase on Jan 13, 2021

jnewbery force-pushed on Jan 14, 2021

jnewbery commented at 12:45 PM on January 14, 2021: member

Rebased now that the fuzz conflicts should no longer be a problem (#20828)

jnewbery force-pushed on Jan 14, 2021

jnewbery commented at 12:55 PM on January 14, 2021: member

Thanks for the review @vasild - I've addressed all your comments.

vasild approved

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.

DrahtBot removed the label Needs rebase on Jan 14, 2021

DrahtBot added the label Needs rebase on Mar 12, 2021

jnewbery commented at 9:33 AM on March 19, 2021: member

Rebased

jnewbery force-pushed on Mar 19, 2021

vasild approved

vasild commented at 9:57 AM on March 19, 2021: member

ACK cfbf35efb6acf182a31b57220b9001a1459c937c

DrahtBot removed the label Needs rebase on Mar 19, 2021

sipa commented at 3:07 AM on March 20, 2021: member

utACK cfbf35efb6acf182a31b57220b9001a1459c937c

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.

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).

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)

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?

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0fa201a047..619693536b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -972,6 +972,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler)
 void PeerManagerImpl::FinalizeNode(const CNode& node)
 {
     NodeId nodeid = node.GetId();
+    {
     LOCK(cs_main);
     int misbehavior{0};
     {
@@ -990,14 +991,6 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
     if (state->fSyncStarted)
         nSyncStarted--;
 
-    if (node.fSuccessfullyConnected && misbehavior == 0 &&
-        !node.IsBlockOnlyConn() && !node.IsInboundConn()) {
-        // Only change visible addrman state for full outbound peers.  We don't
-        // call Connected() for feeler connections since they don't have
-        // fSuccessfullyConnected set.
-        m_addrman.Connected(node.addr);
-    }
-
     for (const QueuedBlock& entry : state->vBlocksInFlight) {
         mapBlocksInFlight.erase(entry.hash);
     }
@@ -1022,6 +1015,14 @@ void PeerManagerImpl::FinalizeNode(const CNode& node)
         assert(m_wtxid_relay_peers == 0);
         assert(m_txrequest.Size() == 0);
     }
+    } // cs_main
+    if (node.fSuccessfullyConnected && misbehavior == 0 &&
+        !node.IsBlockOnlyConn() && !node.IsInboundConn()) {
+        // Only change visible addrman state for full outbound peers.  We don't
+        // call Connected() for feeler connections since they don't have
+        // fSuccessfullyConnected set.
+        m_addrman.Connected(node.addr);
+    }
     LogPrint(BCLog::NET, "Cleared nodestate for peer=%d\n", nodeid);
 }

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.

MarcoFalke commented at 8:38 AM on March 20, 2021: member

I looked at cfbf35efb6acf182a31b57220b9001a1459c937c 🐣

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash f3498d80e5b4a7afbc97ad18b42947699359862c306a91e93f1dbb3d43a2cb07 -

</details>

jnewbery force-pushed on Mar 20, 2021

[net] Construct addrman outside connman

node.context owns the CAddrMan. CConnman holds a reference to
the CAddrMan.

1c25adf6d2

[net_processing] Keep addrman reference in PeerManager 392a95d393

[net] remove CConnman::SetServices

It just forwards calls to CAddrMan::SetServices.

8073673dbc

[net] remove CConnman::MarkAddressGood

It just forwards calls to CAddrMan::Good.

bcd7f30b79

[net] remove CConnman::AddNewAddresses

It just forwards calls to CAddrMan::Add.

7c4cc67c0c

jnewbery force-pushed on Mar 20, 2021

jnewbery commented at 10:26 AM on March 20, 2021: member

Thanks for the review @MarcoFalke. I've addressed all of your comments.

jnewbery commented at 12:24 PM on March 20, 2021: member

@MarcoFalke - I've taken your suggested diff.

jnewbery force-pushed on Mar 20, 2021

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

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh2TQwAuta5eICUDWp5ZOK6mqTbXME6W4oZ2tq4JrradpQgatb0gd7Cvl9voHBS
95imHMkW8gS8qyTS5lCJfmaGZs6GsANQ4WY/SSMDAYPZHR7UUnicrlP5hBE1zUP0
olJXiIu/cNeTibA2gImd1bwxW11h3UII6R8fZ91lLHBTlsakqgX84n1fsRmOHdn7
UG4S6VDK9qceJ/dNTbww56vEn7hnrE9t0FUmFNqb6Yb5LgkG11Bw9XA71urWcFjc
2nvl4CBmZI73SzCyPtcyQ8iYuL6INtg6Rt6BtV6ff8Xm9cYB28cvkP6PFwjp4SHw
Zns+ZiBshcMWgODAvaetrCQNVHo597lW94zfCrSRrhsIVWw9TK6sTJRpkJRLMNxv
6ASeEetu7cME/nFHzabA+PV6nhQIcHoaczCZfdkiY+n1u83KJGvyEl3c12LW/rLT
yjNU8VV/34GM7Fr+HmjK1TJx3aBdcurK+4sNorAr+7JiAovtqTTjEavKYnoeftmm
N1OnbouT
=MOU8
-----END PGP SIGNATURE-----

Timestamp of file with hash 04be068a42318cb4f710b8b8d5521a5bda7090c95bd0fa710e8d6dbc7a70894e -

</details>

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?

[net] remove fUpdateConnectionTime from FinalizeNode

PeerManager can just call directly into CAddrMan::Connected() now.

3fc06d3d7b

jnewbery force-pushed on Mar 22, 2021

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.

MarcoFalke commented at 10:33 AM on March 22, 2021: member

re-ACK 3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682 only change is squash 🏀

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash a02bd2eb33e158baac0fe590960dabd93a73914fa44637c90c6b2566e605a7f5 -

</details>

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.

MarcoFalke referenced this in commit d400e672a0 on Mar 23, 2021

sidhujag referenced this in commit 63198f2302 on Mar 23, 2021

vasild approved

vasild commented at 9:21 AM on March 26, 2021: member

ACK 3fc06d3d7b43dc1143fe0850db23c4e7ffbfe682

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.

MarcoFalke merged this on Mar 30, 2021

MarcoFalke closed this on Mar 30, 2021

jnewbery deleted the branch on Mar 30, 2021

sidhujag referenced this in commit c9799f13a0 on Mar 30, 2021

DrahtBot locked this on Aug 16, 2022