Revert "refactor: replace CConnman pointers by references in net

laanwj commented at 9:53 am on July 17, 2020: member

This reverts commit 0c8461a88ed66a1f70559fc96646708949b17e4b.

In discussion in #19174 there was a concept NACK by the original author of the code. It was merged after this without any further discussion.

Revert "refactor: replace CConnman pointers by references in net_processing.cpp"

This reverts commit 0c8461a88ed66a1f70559fc96646708949b17e4b.

In discussion in #19174 there was a concept NACK by the original
author of the code. It was merged after this without any further
discussion.

1aff21b364

laanwj added the label P2P on Jul 17, 2020

DrahtBot commented at 11:27 am on July 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:

#19503 (Add parameter feature to serialization and use it for CAddress by sipa)
#19498 (Tidy up ProcessOrphanTx by jnewbery)
#19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
#19184 (Overhaul transaction request logic by sipa)
#19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)
#18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)
#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.

jnewbery commented at 12:23 pm on July 17, 2020: member

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.

laanwj commented at 12:44 pm on July 17, 2020: member

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

vasild commented at 1:16 pm on July 17, 2020: member

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

MarcoFalke commented at 2:29 pm on July 17, 2020: member

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). https://github.com/bitcoin/bitcoin/blob/c04485850e72e773280e0cc8d88e73ec791bef73/src/node/context.h#L36 Also, places where connman could be nullptr properly check for that. https://github.com/bitcoin/bitcoin/blob/c04485850e72e773280e0cc8d88e73ec791bef73/src/rpc/mining.cpp#L640-L642

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 1aff21b36450644390d6361d496811dda5a6ae31 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.

JeremyRubin commented at 4:00 pm on July 17, 2020: contributor

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.

MarcoFalke commented at 4:06 pm on July 17, 2020: member

Related discussion on IRC for reference:

 0[14:24] <jnewbery> wumpus: can you wait for cfields to review before merging 19542 please?
 1[14:26] <cfields> I was just commenting there... will review in detail today. I suspect I was wrong with the concept nack anyway...
 2[14:26] <cfields> it's not a big deal, though.
 3[14:28] <jnewbery> cfields: thanks!
 4[14:30] <cfields> np, these things happen :)
 5[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
 6[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
 7[14:34] <wumpus> "increases code quality" is kind of subjective
 8[14:38] <wumpus> if it's uncontroversial and doesn't impact other people's work, okay, but not sure about this
 9[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
10[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.

theuni commented at 7:41 pm on July 17, 2020: member

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!

laanwj closed this on Jul 20, 2020

DrahtBot locked this on Feb 15, 2022

Revert “refactor: replace CConnman pointers by references in net_processing.cpp” #19542

Conflicts