p2p: improve onion detection in AttemptToEvictConnection #20193

Now that #19991 is merged, we may have a more robust way to determine inbound onion peers using CNode::m_inbound_onion rather than IsLocal().

Use it for the new AttemptToEvictConnection logic added in #19670 to address #19500.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19972 (tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable. by practicalswift)

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.

Concept ACK, but please add a AttemptToEvictConnection unit test for this scenario.

See #19966 on ideas on how to make AttemptToEvictConnection testable, or feel free to cherry-pick in 818dd2a04da1ae7329c58a79a4fade5ec62c549d and df1328fcecc3f359574fa53979402894b4a73a12 from #19972 ("tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.") .

I agree, but ISTM (a) the code needs refactoring to be unit-testable, (b) refactoring of critical code to make it testable is low priority for reviewers, high risk, and unlikely to be merged (unless someone of v high stature drives it), and (c) ATEC improvements and fixes without tests do see review and have been merged over the years, so I didn't plan to gate this (essentially one-line improvement) on critical refactoring and technical debt reduction.

@jonatack

unlikely to be merged (unless someone of v high stature drives it)

I find it very concerning that you're worried that you're not "high stature"(!) enough to suggest such a change.

Either it is in the best interest of our users to have your PR merged (based on pure code and concept review), or it is not.

Who wrote the code simply doesn't enter that equation.

FWIW I pledge to never let the probability of a ACK/NACK for a given PR to be conditional on who submitted the PR. That would be ridiculous and obviously not in the best interest of our users.

ISTM the bottlenecks to refactoring critical code for testability are risk, expert review, and low review interest. I could refactor ATEC but that doesn't mean people will be eager to review it, and I don't think it's realistic to gate a one-line improvement on a larger technical debt reduction bottleneck like that. I don't believe the previous changes to ATEC have been gated on it either. There are clearly good reasons for that? Perhaps raise this larger topic in an issue or core dev meeting rather than here?

I could refactor ATEC but that doesn't mean people will be eager to review it, …

FWIW I would be very eager to review a change which made AttemptToEvictConnection unit testable. As demonstrated it can be made unit testable with only few lines of code.

I've always been somewhat amazed about how little testing AttemptToEvictConnection has considering the importance of that function for the network health. See #16660 (August 2019) and #19966 (September 2020) for context.

As the only person until now who reviewed your ATEC refactoring, I'm aware. I do think global refactoring and writing tests for ATEC is orthogonal and out of scope to this one-line change, and far harder to have reviewed and merged, which is why it was not proposed in the parent #19670. Nor, for that matter, in the other parent PR #19991 that had no tests. Both of these were larger changes. 🤷‍♂

Nor, for that matter, in the other parent PR #19991 that had no tests.

Btw, https://github.com/bitcoin/bitcoin/pull/20172/commits/da8b1092fc2d5fa5ee9da3310dae16d4c919bc1b in #20172 adds a functional test that checks alternative port for Tor onion connections.

Good!