addrman: Make addrman a top-level component by jnewbery · Pull Request #20228 · bitcoin/bitcoin

jnewbery · 2020-10-23T09:43:07Z

Addrman is currently a member variable of connman. Make it a top-level component with lifetime owned by node.context, and add a reference to addrman in peerman. This allows us to eliminate some functions in connman that are simply forwarding requests to addrman, and simplifies the connman-peerman interface.

By constructing the addrman in init, we can also add parameters to the ctor, which allows us to test it better. See #20233, where we enable consistency checking for addrman in our functional tests.

maflcko · 2020-10-23T10:38:47Z

does it conceptually make sense to have an addrman without connman or connman without addrman? I'd say no.

This allows us to eliminate some functions in connman that are simply forwarding

An alternative to achieve that would be to let PeerMan steal a reference to addrman from connman (and assign it to m_addrman)

Saibato · 2020-10-23T10:57:36Z

This allows us to eliminate some functions in connman that are simply forwarding

In general to have things double or more if ( ,,,) or detrimental decision processes in the same code is a good design in industry where u can kill ppl easy, if there is no double check and just one bit ( that could be just by chance or external forces have flipped ) decides over mayhem.

In financials we used to do that the same and wrote "bad ugly code". bcs u get no bonus at the years end, if the code "looks" good but your customers got screwed. Unless that was ur biz.

jnewbery · 2020-10-23T11:47:00Z

does it conceptually make sense to have an addrman without connman or connman without addrman? I'd say no.

Why not? If -connect is set, then we don't use addrman for choosing outbound connections (see m_use_addrman_outgoing inside connman). In general, I think being able to run with different components disabled enforces strict separation between components and means there can't be unintentional coupling between them.

jnewbery · 2020-10-23T17:52:58Z

Fixed whitespace linter issue

DrahtBot · 2020-10-23T21:25:08Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net_processing: lock clean up #21527 (NOMERGE: net_processing: orphan handling changes by ajtowns)
Move GetDataDir to ArgsManager #21244 (Move GetDataDir to ArgsManager by kiminuo)
banman: save the banlist in a JSON format on disk #20966 (banman: save the banlist in a JSON format on disk by vasild)
multiprocess: Add bitcoin-wallet -ipcconnect option #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
multiprocess: Add basic spawn and IPC support #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

maflcko · 2020-10-24T06:58:23Z

This doesn't compile on gcc4.8, but we'll drop support for that soon anyway, so the failure can be ignored/rebased out when 22.0 is branched off.

jnewbery · 2020-10-24T08:26:38Z

This doesn't compile on gcc4.8

Hopefully fixed

jnewbery · 2020-11-03T09:02:07Z

rebased

amitiuttarwar · 2020-11-12T18:31:17Z

concept ACK

jnewbery · 2020-11-19T11:28:29Z

rebased

node.context owns the CAddrMan. CConnman holds a reference to the CAddrMan.

It just forwards calls to CAddrMan::SetServices.

It just forwards calls to CAddrMan::Good.

It just forwards calls to CAddrMan::Add.

jnewbery · 2021-03-20T10:26:13Z

Thanks for the review @MarcoFalke. I've addressed all of your comments.

jnewbery · 2021-03-20T12:24:45Z

@MarcoFalke - I've taken your suggested diff.

maflcko · 2021-03-22T09:59:30Z

re-ACK 06653e4aaccc84dc7773d3888f687cf44e20abcd 🕸

Only changes:

Add missing m_node.addrman.reset() in tests
Remove unused symbols random_service and random_address in tests
Add commit 06653e4aac

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 06653e4aaccc84dc7773d3888f687cf44e20abcd 🕸

Only changes:
* Add missing m_node.addrman.reset() in tests
* Remove unused symbols random_service and random_address in tests
* Add commit 06653e4aac
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh2TQwAuta5eICUDWp5ZOK6mqTbXME6W4oZ2tq4JrradpQgatb0gd7Cvl9voHBS
95imHMkW8gS8qyTS5lCJfmaGZs6GsANQ4WY/SSMDAYPZHR7UUnicrlP5hBE1zUP0
olJXiIu/cNeTibA2gImd1bwxW11h3UII6R8fZ91lLHBTlsakqgX84n1fsRmOHdn7
UG4S6VDK9qceJ/dNTbww56vEn7hnrE9t0FUmFNqb6Yb5LgkG11Bw9XA71urWcFjc
2nvl4CBmZI73SzCyPtcyQ8iYuL6INtg6Rt6BtV6ff8Xm9cYB28cvkP6PFwjp4SHw
Zns+ZiBshcMWgODAvaetrCQNVHo597lW94zfCrSRrhsIVWw9TK6sTJRpkJRLMNxv
6ASeEetu7cME/nFHzabA+PV6nhQIcHoaczCZfdkiY+n1u83KJGvyEl3c12LW/rLT
yjNU8VV/34GM7Fr+HmjK1TJx3aBdcurK+4sNorAr+7JiAovtqTTjEavKYnoeftmm
N1OnbouT
=MOU8
-----END PGP SIGNATURE-----

Timestamp of file with hash 04be068a42318cb4f710b8b8d5521a5bda7090c95bd0fa710e8d6dbc7a70894e -

maflcko · 2021-03-22T10:01:27Z

Could squash the last two commits and mention that --color-moved=dimmed-zebra can be used on them for review?

PeerManager can just call directly into CAddrMan::Connected() now.

jnewbery · 2021-03-22T10:26:28Z

Could squash the last two commits

Done

and mention that --color-moved=dimmed-zebra can be used on them for review?

I tried this and it wasn't very useful - there are only a couple of lines that are moved without changing.

maflcko · 2021-03-22T10:33:59Z

re-ACK 3fc06d3 only change is squash 🏀

Show signature and timestamp

Signature:

Timestamp of file with hash a02bd2eb33e158baac0fe590960dabd93a73914fa44637c90c6b2566e605a7f5 -

jnewbery · 2021-03-22T11:02:57Z

@vasild @sipa: Do you mind reACKing? There have only been minor changes since your last reviews.

5555446 fuzz: Use ConsumeWeakEnum in addrman for service flags (MarcoFalke) Pull request description: This has minimally better performance. Reported by me in bitcoin/bitcoin#20228 (comment) ACKs for top commit: practicalswift: Tested ACK 5555446 vasild: ACK 5555446 Tree-SHA512: 4e5f51fe4f2bd7b2f37d0690e41203341ba45c0c9bc9247449cd26cfb5f77dc2ec61df3e4963276f68694e4b3ca3d0a76367a51c4d775501edeb3224d305a261

… flags 5555446 fuzz: Use ConsumeWeakEnum in addrman for service flags (MarcoFalke) Pull request description: This has minimally better performance. Reported by me in bitcoin#20228 (comment) ACKs for top commit: practicalswift: Tested ACK 5555446 vasild: ACK 5555446 Tree-SHA512: 4e5f51fe4f2bd7b2f37d0690e41203341ba45c0c9bc9247449cd26cfb5f77dc2ec61df3e4963276f68694e4b3ca3d0a76367a51c4d775501edeb3224d305a261

vasild

ACK 3fc06d3

jnewbery · 2021-03-30T09:53:16Z

@sipa do you mind rereviewing this? It has two ACKs on the current branch.

jnewbery added P2P Refactoring labels Oct 23, 2020

jnewbery changed the title ~~[addrman] Make addrman a top-level component~~ addrman: Make addrman a top-level component Oct 23, 2020

jnewbery force-pushed the 2020-10-addrman branch from 1b6dbae to e160909 Compare October 23, 2020 17:53

DrahtBot mentioned this pull request Oct 23, 2020

net: Remove g_relay_txes #20217

Merged

jnewbery mentioned this pull request Oct 23, 2020

addrman: Make consistency checks a runtime option #20233

Closed

jnewbery force-pushed the 2020-10-addrman branch from e160909 to 1755f08 Compare October 24, 2020 08:23

jnewbery force-pushed the 2020-10-addrman branch from 1755f08 to 98551d3 Compare October 27, 2020 12:19

DrahtBot mentioned this pull request Oct 28, 2020

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

DrahtBot added the Needs rebase label Nov 3, 2020

jnewbery force-pushed the 2020-10-addrman branch from 98551d3 to d71274d Compare November 3, 2020 09:01

DrahtBot removed the Needs rebase label Nov 3, 2020

DrahtBot mentioned this pull request Nov 3, 2020

[net] Consolidate logic around calling CAddrMan::Connected() #20291

Merged

jnewbery force-pushed the 2020-10-addrman branch from d71274d to d280f67 Compare November 19, 2020 11:28

DrahtBot added the Needs rebase label Nov 19, 2020

jnewbery force-pushed the 2020-10-addrman branch from d280f67 to ef7c0c3 Compare November 19, 2020 17:50

jnewbery force-pushed the 2020-10-addrman branch 3 times, most recently from a765e9b to 8715f8d Compare March 20, 2021 10:20

jnewbery added 5 commits March 20, 2021 10:24

[net] Construct addrman outside connman

1c25adf

node.context owns the CAddrMan. CConnman holds a reference to the CAddrMan.

[net_processing] Keep addrman reference in PeerManager

392a95d

[net] remove CConnman::SetServices

8073673

It just forwards calls to CAddrMan::SetServices.

[net] remove CConnman::MarkAddressGood

bcd7f30

It just forwards calls to CAddrMan::Good.

[net] remove CConnman::AddNewAddresses

7c4cc67

It just forwards calls to CAddrMan::Add.

jnewbery force-pushed the 2020-10-addrman branch from 8715f8d to 880cea7 Compare March 20, 2021 10:24

maflcko mentioned this pull request Mar 20, 2021

fuzz: Use ConsumeWeakEnum in addrman for service flags #21487

Merged

jnewbery force-pushed the 2020-10-addrman branch from 2d7fa2d to 06653e4 Compare March 20, 2021 16:38

[net] remove fUpdateConnectionTime from FinalizeNode

3fc06d3

PeerManager can just call directly into CAddrMan::Connected() now.

jnewbery force-pushed the 2020-10-addrman branch from 06653e4 to 3fc06d3 Compare March 22, 2021 10:26

DrahtBot mentioned this pull request Mar 25, 2021

net_processing: lock clean up #21527

Closed

vasild approved these changes Mar 26, 2021

View reviewed changes

maflcko merged commit 1999baa into bitcoin:master Mar 30, 2021

jnewbery deleted the 2020-10-addrman branch March 30, 2021 10:38

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 30, 2021

Merge bitcoin#20228: addrman: Make addrman a top-level component

c9799f1

jnewbery mentioned this pull request Aug 18, 2021

addrman: Remove CAddrMan::Clear() function #22697

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

jnewbery commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 23, 2020

Uh oh!

Saibato commented Oct 23, 2020

Uh oh!

jnewbery commented Oct 23, 2020

Uh oh!

jnewbery commented Oct 23, 2020

Uh oh!

DrahtBot commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Oct 24, 2020

Uh oh!

jnewbery commented Oct 24, 2020

Uh oh!

jnewbery commented Nov 3, 2020

Uh oh!

amitiuttarwar commented Nov 12, 2020

Uh oh!

jnewbery commented Nov 19, 2020

Uh oh!

jnewbery commented Mar 20, 2021

Uh oh!

jnewbery commented Mar 20, 2021

Uh oh!

maflcko commented Mar 22, 2021

Uh oh!

maflcko commented Mar 22, 2021

Uh oh!

jnewbery commented Mar 22, 2021

Uh oh!

maflcko commented Mar 22, 2021

Uh oh!

jnewbery commented Mar 22, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jnewbery commented Oct 23, 2020 •

edited

Loading

DrahtBot commented Oct 23, 2020 •

edited

Loading