Add vConnect to CConnman::Options by benma · Pull Request #10596 · bitcoin/bitcoin

benma · 2017-06-15T08:18:08Z

Split the "-connect" argument parsing out of CConnman and put it into
AppInitMain().

benma · 2017-06-15T08:59:05Z

@theuni please take a look if you can.

sipa · 2017-06-16T23:28:32Z

src/net.h

Pass by const reference?

std::thread takes arguments by value/rvalue only. Taking a reference would be dangerous as the arg could go out of scope before it's used in the thread.

Edit: Thinking more about this, std::thread's constructor turns references into copies anyway, so I suppose a const reference here would just extend the lifetime of the copy as usual.

I also read that the reference is turned into a copy, but I figured it's better to be explicit about it being a copy. I also didn't use a pointer because dealing with its lifetime is not worth it.

I also prefer it this way, explicitly.

theuni · 2017-06-16T23:43:28Z

@benma Thanks, will review asap.

theuni

Concept ACK. Looks good, just a few nits.

theuni · 2017-06-20T20:48:18Z

src/init.cpp

I didn't mention it on your first PRs because I didn't want to scare you away, but we've just agreed to change our code style from hungarian to snake_case. Mind updating? See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md.

Don't worry about changing everything around this code, just what you're touching.

Done (I originally thought it was better to keep the local style and fix them all in batches)

theuni · 2017-06-20T21:00:23Z

src/init.cpp

Please rename this so that they're more clear, since this is exposed outside of CConnman now.
connOptions.vConnect -> connOptions.m_specified_outgoing
connOptions.bNoConnect -> (!) connOptions.m_use_addrman_outgoing

theuni · 2017-06-20T21:08:17Z

src/net.cpp

All outbound connections are moving to one thread soon, but for now, we should return an error if connOptions.m_use_addrman_outgoing && !connOptions.m_specified_outgoing.empty().

Also, If addrman isn't being used for outgoing and there are no specified connections (incoming connections only), we can just skip creating the thread completely.

Done, but should the error maybe be an assert? That code path cannot be triggered and is not meaningful to the user. If other things want to use Connman and mess it up, it's a coding error and they'll see the assert.

It will be possible (and reasonable) when vAddedNodes is added as an connman option like the others. It might be a good next target :)

What does it have to do with vAddedNodes? The specifc new error that is introduced doesn't depend on it.

benma · 2017-06-22T09:58:47Z

@theuni just to let you know that I'll address everything in about two weeks (will be pretty much offline until then). Cheers.

theuni · 2017-07-18T00:46:13Z

src/init.cpp

connOptions.m_use_addrman_outgoing should be false if !connect.size() == 1 && connect[0] == "0"

Right! Fixed.

theuni · 2017-07-19T17:52:09Z

src/net.h

I'd rather not do this, as everything else here is set to null, false, or uninitialized. A value of true is unexpected.

Done. Note that now, the default options are to not make any outbound connections (I put it to true to have it make outbound connections by default, like bitcoind does).

Split the "-connect" argument parsing out of CConnman and put it into AppInitMain().

theuni · 2017-07-19T22:10:00Z

src/init.cpp

+    if (!connOptions.m_use_addrman_outgoing) {
+        const auto connect = gArgs.GetArgs("-connect");
+        if (connect.size() != 1 || connect[0] != "0") {
+            connOptions.m_specified_outgoing = connect;


this is broken again if connect.size() == 1 && connect[0] == "0" :)

How? If connect.size() == 1 && connect[0] == "0", then m_use_addrman_outgoing = false and m_specified_outgoing is empty, and the thread is not started.

Yes sorry, nevermind.

benma · 2017-07-26T21:35:12Z

@theuni does the PR look good to you?

theuni · 2017-07-31T14:38:50Z

utACK 352d582. This will need to go in after the 0.15 split though, due to the new strings.

benma · 2017-07-31T21:32:58Z

@theuni thanks. Did you mean this string? If so, maybe we can just remove it (resp. replace it with an assertion)?

See #10596 (comment) (sorry for the late response, somehow I missed it).

352d582 Add vConnect to CConnman::Options (Marko Bencun) Pull request description: Split the "-connect" argument parsing out of CConnman and put it into AppInitMain(). Tree-SHA512: f2d3efc4e2c5808ff98696ea20dd96df599bc472ed5afc9c3eea305d94c36a6ab50c632aa05396c7c34d1917d91b1e7ccd725656ff2631e2a36d9eac477455dc

chefsaroar · 2019-03-24T17:56:57Z

Add BTC my wallet

352d582 Add vConnect to CConnman::Options (Marko Bencun) Pull request description: Split the "-connect" argument parsing out of CConnman and put it into AppInitMain(). Tree-SHA512: f2d3efc4e2c5808ff98696ea20dd96df599bc472ed5afc9c3eea305d94c36a6ab50c632aa05396c7c34d1917d91b1e7ccd725656ff2631e2a36d9eac477455dc

e1d12d3 Add Clang thread safety analysis annotations (furszy) 5716940 net: Add missing locks in net.{cpp,h} (furszy) 8c02b59 net: simplify fRelayTxes flag processing (furszy) 71667df remove unused IsArgSet check (Marko Bencun) 729c63d add m_added_nodes to connman options (Marko Bencun) 8c8ad18 [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest& request) (practicalswift) a13b7c9 Add vConnect to CConnman::Options (Marko Bencun) 987342e ActiveMasternode: fix not initialized socket. (furszy) 8d788ba add SeedNodes to CConnman::Options (Marko Bencun) d9e91ff add Binds, WhiteBinds to CConnman::Options (Marko Bencun) 41c89af add WhitelistedRange to CConnman::Options (Marko Bencun) Pull request description: More groundwork for the LLMQ sessions connections work, built on top of #2586 and #2587 (starts in 10efb72a). Focused on cleaning the connman init/start by decoupling the command line arguments. Backported PRs list: * bitcoin#10467. * bitcoin#10496. * bitcoin#10596. * bitcoin#10977. * bitcoin#11301. * bitcoin#11744 (partially, without the outbound members changes as we don't have them). ACKs for top commit: random-zebra: utACK e1d12d3 Fuzzbawls: ACK e1d12d3 Tree-SHA512: 81a1ab7a1e7f487330354631ee728be9ec78223fe4978c8b9c97b7fbc8d2bfe4f4ea9e88ac4a3d1f0553f7adad871c81261b1a7545bae710a4e3200b8a5031d7

fanquake added the P2P label Jun 15, 2017

sipa reviewed Jun 16, 2017

View reviewed changes

theuni reviewed Jun 20, 2017

View reviewed changes

benma force-pushed the connmanoptions_connect branch from 8466609 to 89bee24 Compare July 17, 2017 22:54

theuni reviewed Jul 18, 2017

View reviewed changes

benma force-pushed the connmanoptions_connect branch from 89bee24 to 4286fd4 Compare July 18, 2017 10:04

theuni reviewed Jul 19, 2017

View reviewed changes

Add vConnect to CConnman::Options

352d582

Split the "-connect" argument parsing out of CConnman and put it into AppInitMain().

benma force-pushed the connmanoptions_connect branch from 4286fd4 to 352d582 Compare July 19, 2017 21:37

theuni reviewed Jul 19, 2017

View reviewed changes

benma mentioned this pull request Jul 24, 2017

Refactor fRelayTxes global to CConnman member #10915

Closed

laanwj merged commit 352d582 into bitcoin:master Sep 6, 2017

furszy mentioned this pull request Nov 5, 2021

[net] Connman options encapsulation PIVX-Project/PIVX#2632

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

benma commented Jun 15, 2017

Uh oh!

benma commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Jun 16, 2017

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benma commented Jul 26, 2017

Uh oh!

theuni commented Jul 31, 2017

Uh oh!

benma commented Jul 31, 2017

Uh oh!

chefsaroar commented Mar 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

theuni Jun 16, 2017 •

edited

Loading