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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Add parameter feature to serialization and use it for CAddress #19503 (Add parameter feature to serialization and use it for CAddress by sipa)
• Tidy up ProcessOrphanTx #19498 (Tidy up ProcessOrphanTx by jnewbery)
• refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
• Overhaul transaction request logic #19184 (Overhaul transaction request logic by sipa)
• p2p: Move all header verification into the network layer, extend logging #19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)
• Cache responses to GETADDR to prevent topology leaks #18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)
• Use wtxid for transaction relay #18044 (Use wtxid for transaction relay by sdaftuar)

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.

Can we wait for a comment from @theuni before merging this? I think that we all agree that #19174 was merged too quickly, but it caused a bunch of PRs to have to be rebased and reverting it would mean rebasing everything yet again.

We must also hold up those other PRs as long as this should maybe be reverted. Right now this is still a clean git revert

Not sure if there is a deterministic way to find all such PRs, but at least #19031 and #19503 are among them.

reverting it would mean rebasing everything yet again

They could also be reset to the previous version, as this was the only conflict. (Assuming no other changes to the pulls in question happened in the meantime).

As to the conceptual discussion, I think everyone agrees with @theuni that connman should be treated as an optional pointer. In current master, connman is a pointer (and it is not trivially possible to make it a non-pointer).

However, the lines changed by the patch are called only where connman is already confirmed to be not null. If we go ahead to merge this, it would be good to document in the code or somewhere else why those cases should be treated as pointers and, at the same time, why nullptr checks are not needed for them.

ACK 1aff21b for now. I only checked that it cleanly reverts, but ideally we'd have clear documentation on how to pass connman around and why. Sorry for causing this disagreement in the first place. I wish the time spent on this was used on user-facing changes, but properly documenting this will hopefully avoid misunderstandings in the future.

FWIW I did review the original #19174 and it does appear safe/not a bug, so happy to re-ACK it if @theuni weighs in affirmatively.

+1 Marco you can just revert the rebases, no need to write new code. But may as well wait till we get word from Cory on if there's a reason not to take the patch.

Related discussion on IRC for reference:

[14:24] <jnewbery> wumpus: can you wait for cfields to review before merging 19542 please?
[14:26] <cfields> I was just commenting there... will review in detail today. I suspect I was wrong with the concept nack anyway...
[14:26] <cfields> it's not a big deal, though.
[14:28] <jnewbery> cfields: thanks!
[14:30] <cfields> np, these things happen :)
[14:33] <wumpus> yes, though, "this causes a few PRs to need to be rebased" was another reason to be cautious about merging it in the first place
[14:34] <wumpus> I think we should avoid doing these kind of mass data type changes unless there is a really good rationale and people agree about doing it
[14:34] <wumpus> "increases code quality" is kind of subjective
[14:38] <wumpus> if it's uncontroversial and doesn't impact other people's work, okay, but not sure about this
[14:48] <jnewbery> wumpus: yes, agree. When I wrote my ACK I originally had a comment about not merging immediately because of conflicts, but I deleted that because I didn't want to tell the maintainers how to do their job
[16:46] <MarcoFalke> when merging I do check all conflicts for reviews, and in this case, all of the conflicts had either, no review, didn't compile, or didn't pass the test suite. So in all cases a rebase or force push wouldn't have been harmful or was needed anyway.

Concept NACK :p

Looking in more detail now, it's clear that I jumped to an incorrect conclusion in #19174. My apologies to @theStack for a review so shallow it proved harmful. I'll make sure that future NACKs come with a more thorough review.

I agree with @jnewbery's assessment here: #19174 (comment).

As a note on process, I rather appreciate the way this worked out. Bad reviews should be called out as such (as @MarcoFalke and @jnewbery did), and worrisome merges should be pointed out for discussion (as @JeremyRubin did), even if ultimately no further action is taken. Though one might argue that I was so off-base on my argument that @MarcoFalke was right not to sit around and wait for me to figure it out :)

Thanks to everyone for being reasonable. Sorry for the noise!