net: Address outstanding review comments from PR20721 #21198

pull jnewbery wants to merge 1 commits into bitcoin:master from jnewbery:2021-02-20721-followups changing 3 files +9 −6
  2. jnewbery commented at 3:57 pm on February 16, 2021: member

    Thanks for reviewing #20721 @ajtowns @amitiuttarwar @MarcoFalke. Here are your suggested changes.

    I’ve marked as draft for now and will rebase on main once #21015 is merged so I can take @MarcoFalke’s suggestion here: #20721 (review).

    EDIT: That suggestion was taken in #21015, which has now been merged.

  3. DrahtBot added the label P2P on Feb 16, 2021
  4. in src/net.h:1028 in cb849ced9b outdated 
    1021@@ -1022,8 +1022,12 @@ class CConnman
1022 
1023     void SetAsmap(std::vector<bool> asmap) { addrman.m_asmap = std::move(asmap); }
1024 
1025-    /** Return true if the peer has been connected for long enough to do inactivity checks. */
1026-    bool RunInactivityChecks(const CNode& node) const;
1027+    /** Return true if we should disconnect the peer for failing an inactivity check. */
1028+    bool ShouldRunInactivityChecks(const CNode& node, const uint64_t const* pnow=nullptr) const
1029+    {
1030+        const uint64_t now = (pnow ? *pnow : GetSystemTimeInSeconds());
    MarcoFalke commented at 4:33 pm on February 16, 2021:
    I think you can use std::optional<uint64_t>::value_or(uint64_t) instead of passing pointers, no?
    ajtowns commented at 6:43 am on February 17, 2021:

    Was there any reason to move this from net.cpp to net.h? (Coding guidelines say stuff should be in the .cpp unless inlining is critical [EDIT: I assumed the coding guidelines were why you didn’t make it inline in the first place])

    I don’t think optnow.value_or(GetSystemTimeInseconds()) would be a good idea – that would mean always invoking GetSystemTimeInSeconds and throwing away the result if you’d passed in a time.

    MarcoFalke commented at 7:01 am on February 17, 2021:
    Ah good point on the arg to value_or always being evaluated. Though, keeping the ternary as is and changing the type to optional would work?
    jnewbery commented at 9:43 am on February 17, 2021:

    Was there any reason to move this from net.cpp to net.h? (Coding guidelines say stuff should be in the .cpp unless inlining is critical [EDIT: I assumed the coding guidelines were why you didn’t make it inline in the first place])

    Only because you and amiti seemed to want it inlined. I didn’t realise that this guidance was in the developer notes, but I completely agree. net.h is included in just about everything so any lines of code in this file are compiled hundreds of times.

    I’ll move it back to the cpp file.

    ajtowns commented at 9:46 am on February 17, 2021:
    @jnewbery Having it inline means the ?: can be resolved at compile time for every use case, and I couldn’t resist the temptation. @MarcoFalke Yes, keeping the ?: should work fine with optional instead of pointer.
    jnewbery commented at 10:04 am on February 17, 2021:

    I don’t have a strong opinion about any of this. My default is to implement things in the cpp file unless the function is in a tight loop where performance really matters and the function call overhead is meaningful. Your suggestion in the previous PR was to inline it, and amiti mentioned the same thing to me, so I made that change here.

    I’ve now moved it back to the cpp and made the optional now arg a std::optional.

    ajtowns commented at 3:09 am on February 18, 2021:
    Yeah, it wouldn’t surprise me if it gets optimised during linking these days anyway. My bad habits don’t need to be yours!
    jnewbery commented at 8:18 am on February 18, 2021:
    I don’t think we have link time optimization enabled yet, but hopefully in the future!
  5. jnewbery force-pushed on Feb 17, 2021
  6. jnewbery force-pushed on Feb 17, 2021
  7. in src/net.cpp:1223 in ae12c657fa outdated 
    1219@@ -1219,6 +1220,8 @@ bool CConnman::InactivityCheck(const CNode& node) const
1220     // use setmocktime in the tests).
1221     int64_t now = GetSystemTimeInSeconds();
1222 
1223+    if (ShouldRunInactivityChecks(node, now)) return false;
    ajtowns commented at 3:10 am on February 18, 2021:
    if ( ! ShouldRunInactivityChecks())
    jnewbery commented at 8:17 am on February 18, 2021:

    :flushed:

    fixed

  8. jnewbery force-pushed on Feb 18, 2021
  9. jnewbery force-pushed on Feb 22, 2021
  10. jnewbery commented at 8:46 am on February 22, 2021: member
    Rebased on master to pick up fix for interface_zmq.py in #21008.
  11. jnewbery force-pushed on Feb 26, 2021
  12. DrahtBot commented at 4:33 pm on March 4, 2021: 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:

    • #21563 (net: Drop cs_sendProcessing mutex that guards nothing by hebasto)
    • #21236 (net processing: Extract addr send functionality into MaybeSendAddr() by jnewbery)

    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.

  13. jnewbery force-pushed on Mar 30, 2021
  14. jnewbery commented at 9:52 am on March 30, 2021: member
    Rebased and moved out of draft.
  15. jnewbery marked this as ready for review on Mar 30, 2021
  16. jnewbery renamed this:
    [net] Address outstanding review comments from PR20721
    net: Address outstanding review comments from PR20721
    on Mar 30, 2021
  17. DrahtBot commented at 9:05 am on April 1, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  18. DrahtBot added the label Needs rebase on Apr 1, 2021
  19. [net] Changes to RunInactivityChecks 
    - rename to ShouldRunInactivityChecks (https://github.com/bitcoin/bitcoin/pull/20721#discussion_r576394790)
- take optional time now (https://github.com/bitcoin/bitcoin/pull/20721#discussion_r575895661)
- call from within InactivityChecks (https://github.com/bitcoin/bitcoin/pull/20721#discussion_r575894665)
- update comment (https://github.com/bitcoin/bitcoin/pull/20721#discussion_r575894343)
- change ordering of inequality (https://github.com/bitcoin/bitcoin/pull/20721#discussion_r574925129)
    5ed535a02f
  20. jnewbery commented at 10:35 am on April 1, 2021: member
    Rebased
  21. jnewbery force-pushed on Apr 1, 2021
  22. jnewbery removed the label Needs rebase on Apr 1, 2021
  23. laanwj commented at 2:36 pm on April 1, 2021: member
    Code review ACK 5ed535a02f8f0a6f65bbe19f48a8c81f43298393
  24. laanwj merged this on Apr 1, 2021
  25. laanwj closed this on Apr 1, 2021
  26. jnewbery deleted the branch on Apr 1, 2021
  27. sidhujag referenced this in commit 6733623b83 on Apr 1, 2021
  28. Fabcien referenced this in commit a3f088f61e on Feb 2, 2022
  29. DrahtBot locked this on Aug 16, 2022


jnewbery MarcoFalke ajtowns DrahtBot laanwj

Labels
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-19 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me