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.

Code review ACK c80de78, this does not change behavior

Concept ACK. This seems much clearer to me.

utACK c80de78

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)

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?

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.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• p2p: Disconnect, not discourage, misbehaving NODE_BLOOM peers #20083 (p2p: Disconnect, not discourage, misbehaving NODE_BLOOM peers by MarcoFalke)
• net: Replace cs_feeFilter with simple std::atomic #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.

Code review ACK c80de78

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.

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?

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.

@jnewbery Sounds good to me, will close.