Replace m_tx_relay/nullptr checks in net_processing.cpp #20676

1. 
   sdaftuar commented at 7:08 pm on December 16, 2020: member

   Semantically, whether m_tx_relay is a nullptr can mean that either the connection is a block-relay-only connection, or that we’re relaying transactions on this connection. While there’s currently a 1-1 mapping between those ideas right now, separate the two concepts more clearly so that in the future it’ll be easier to reason about changes, particularly if we add more connection types which do not relay transactions.

2. Replace m_tx_relay/nullptr checks in net_processing.cpp

   Semantically, whether m_tx_relay is a nullptr can mean that either the
   connection is a block-relay-only connection, or that we're relaying
   transactions on this connection. While there's currently a 1-1 mapping between
   those ideas right now, separate the two concepts more clearly so that in the
   future it'll be easier to reason about changes, particularly if we add more
   connection types which do not relay transactions.

   c80de7834a
3. 
   sdaftuar commented at 7:12 pm on December 16, 2020: member

   This kind of refactor was requested in #19858 (comment). While I think it’s helpful for net_processing code to be using a more meaningful function name rather than check for nullptr in all these places, it’s not entirely clear to me whether it would be best to have the helper function be checking the connection type, or just checking whether m_tx_relay is nullptr or not.

   I opted for the latter approach, because right now (nearly) all those nullptr checks are done before dereferencing m_tx_relay, so I didn’t like the idea of getting rid of the explicit check that the object isn’t null. But if others think that this is not the best way to clean this up, I’m open to suggestions.

4. 
   laanwj commented at 7:47 pm on December 16, 2020: member

   Code review ACK c80de7834ad59fba93548024fc96f541f02b60f7, this does not change behavior
5. DrahtBot added the label P2P on Dec 16, 2020
6. 
   jnewbery commented at 9:31 pm on December 16, 2020: member

   Concept ACK. This seems much clearer to me.
7. 
   sipa commented at 9:35 pm on December 16, 2020: member

   utACK c80de7834ad59fba93548024fc96f541f02b60f7
8. 
   ajtowns commented at 10:11 pm on December 16, 2020: member

   This idea was discussed in the original blocks only PR, https://github.com/bitcoin/bitcoin/pull/15759/files#r310366329 – the reasoning was the explicit == nullptr checks makes it clear that the following dereferencing is safe. That still seems like it makes sense to me, so I’m -0.5 (or however we’re writing weak-nack) on this.

   I tend to think if (m_tx_relay != nullptr && pfrom.ShouldThisPeerGetThisTx(tx)) would be a better way to separate the concepts – makes it clear dereferencing is safe, and the function only needs to deal with the policy decision being made, node code safety. If we know relay is never warranted for this peer, setting m_tx_relay = nullptr at VERACK not just initial connection might be an option (in which case writing if (m_tx_relay != nullptr) { Assume(pfrom.ThisPeerShouldGetTheseTxs()); ... } might make sense)

9. 
   sdaftuar commented at 10:25 pm on December 16, 2020: member

   This idea was discussed in the original blocks only PR, https://github.com/bitcoin/bitcoin/pull/15759/files#r310366329 – the reasoning was the explicit == nullptr checks makes it clear that the following dereferencing is safe. @ajtowns Thanks for the reminder of that discussion, I had forgotten about it! I guess my uneasiness about removing nullptr checks altogether has remained consistent, though I guess I’ve moved a bit on thinking that it’s also hard to reason about what exactly the nullptr check means.

   It occurs to me that the problem may be a bigger issue around the separation between net and net_processing. I think in many ways we want the net_processing layer concerned only about connection types and high level behaviors and not implementation details about the data structures that implement the behaviors. But in fact we dive into the internals of CNode all the time in net_processing (eg grabbing locks and accessing data members directly), which makes attempts to use higher-level abstraction seem incompatible with the rest of the code – if there’s no way around knowing the implementation details, then what is the point of introducing some abstraction?

   I tend to think if (m_tx_relay != nullptr && pfrom.ShouldThisPeerGetThisTx(tx)) would be a better way to separate the concepts – makes it clear dereferencing is safe, and the function only needs to deal with the policy decision being made, node code safety. If we know relay is never warranted for this peer, setting m_tx_relay = nullptr at VERACK not just initial connection might be an option (in which case writing if (m_tx_relay != nullptr) { Assume(pfrom.ThisPeerShouldGetTheseTxs()); ... } might make sense)

   I would also be okay with that an approach that checks for nullptr explicitly and also does the Assume() thing on some other function that is checking the connection type (or vice versa) – do other reviewers have concerns with @ajtowns’ suggestion?

10. 
    ajtowns commented at 10:40 pm on December 16, 2020: member

    But in fact we dive into the internals of CNode all the time in net_processing (eg grabbing locks and accessing data members directly),

    Maybe that will change as @jnewbery’s net/net_processing separation stuff (#19398) progresses? It proposes moving all of m_tx_relay out of net/CNode into net_processing.

11. 
    DrahtBot commented at 1:44 am on December 17, 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:

    • #20083 (p2p: Disconnect, not discourage, misbehaving NODE_BLOOM peers by MarcoFalke)
    • #18819 (net: Replace cs_feeFilter with simple std::atomic by MarcoFalke)

    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.

12. 
    jnewbery commented at 8:35 am on December 17, 2020: member

    Code review ACK c80de7834ad59fba93548024fc96f541f02b60f7

    Maybe that will change as @jnewbery’s net/net_processing separation stuff (#19398) progresses? It proposes moving all of m_tx_relay out of net/CNode into net_processing.

    That’s the hope. Eventually there should be much less reaching across the net/net_processing divide. net_processing eventually will need to access almost nothing from CNode.

    It might make sense to also have a const ConnectionType m_conn_type as a member of Peer (similar to how we have address and m_is_inbound in CNodeState). All of these RelayAddrsWithConn, RelayTxsWithConn, etc functions are net_processing concerns, so it doesn’t make sense that they exist in net.

13. 
    sdaftuar commented at 4:02 pm on December 17, 2020: member

    It occurs to me that moving m_tx_relay to net_processing might change the way we think about the abstraction. I’m okay with closing this PR and deferring how we think about the relationship between the two until @jnewbery’s work to move it lands… It does seem like this PR may be near-sighted in thinking about how net_processing should understand the peer connection behavior, if there are bigger changes in the works?

    I could also just add an assert that m_tx_relay != nullptr after invoking RelayTxsWithConn(), wherever we need it? That would be redundant (as what we’re doing is first checking whether it’s nullptr in that function, then asserting right afterward), but it would document the understanding of how this works inside net_processing, which may be what we’re aiming for. @ajtowns What do you think?

14. 
    jnewbery commented at 5:55 pm on December 17, 2020: member

    I think that this is an improvement, but also agree that the same lines will need to be changed again when the tx inventory data is moved into net_processing, so it might make sense to just leave this for now.

    The next PR in the net/net_processing split series is #19829 if you’d like to help with that work.

15. 
    sdaftuar commented at 5:59 pm on December 17, 2020: member

    @jnewbery Sounds good to me, will close.
16. sdaftuar closed this on Dec 17, 2020

    ---

17. DrahtBot locked this on Feb 15, 2022

Contributors
sdaftuar laanwj jnewbery sipa ajtowns DrahtBot

Labels
P2P