doc: removed help text saying that peers may not connect automatically #29994

kevkevinpal commented at 2:03 PM on April 29, 2024: contributor

Introduced in #23542 and released in version 23.0 there has been significant time since this change (2 years).

This should be removed as it is no longer relevant

DrahtBot commented at 2:03 PM on April 29, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, tdb3, vasild, jonatack, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29775 ([DO NOT MERGE] testnet4 including PoW difficulty adjustment fix by fjahr)

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.

DrahtBot added the label Docs on Apr 29, 2024

kevkevinpal force-pushed on Apr 29, 2024

DrahtBot added the label CI failed on Apr 29, 2024

DrahtBot commented at 2:07 PM on April 29, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/24378891746</sub>

kevkevinpal force-pushed on Apr 29, 2024

in src/init.cpp:540 in 207b62cef3 outdated

 536 | @@ -537,9 +537,7 @@ void SetupServerArgs(ArgsManager& argsman)
 537 |      argsman.AddArg("-peerbloomfilters", strprintf("Support filtering of blocks and transaction with bloom filters (default: %u)", DEFAULT_PEERBLOOMFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 538 |      argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 539 |      argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
 540 | -    // TODO: remove the sentence "Nodes not using ... incoming connections." once the changes from
 541 | -    // https://github.com/bitcoin/bitcoin/pull/23542 have become widespread.
 542 | -    argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>. Nodes not using the default ports (default: %u, testnet: %u, signet: %u, regtest: %u) are unlikely to get incoming connections. Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
 543 | +    argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port>."), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);

stickies-v commented at 3:43 PM on April 29, 2024:

I don't think we should remove default ports or the I2P doc

    argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet: %u, signet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md).", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);

kevkevinpal commented at 4:34 PM on April 29, 2024:

fixed in 95897ff

stickies-v commented at 3:44 PM on April 29, 2024: contributor

Concept ACK. According to https://bitnodes.io/dashboard/, ~85% of public nodes are running 23 or higher.

DrahtBot removed the label CI failed on Apr 29, 2024

doc: removed help text saying that peers may not connect automatically

This help text was introduced about two years ago and now a significant
portion of users are using version 23.0 and higher

95897ff181

kevkevinpal force-pushed on Apr 29, 2024

stickies-v approved

stickies-v commented at 6:02 PM on April 29, 2024: contributor

ACK 95897ff181c0757e445f0e066a2a590a0a0120d2

tdb3 commented at 2:15 AM on April 30, 2024: contributor

ACK for 95897ff181c0757e445f0e066a2a590a0a0120d2.

Thank you for circling back to this. Makes sense to me given 23.0+ adoption. Also like how default port values were kept in the message for clarity.

$ src/bitcoind -help | grep -A3 " -port="
  -port=<port>
       Listen for connections on <port> (default: 8333, testnet: 18333, signet:
       38333, regtest: 18444). Not relevant for I2P (see doc/i2p.md).

glozow commented at 9:20 AM on April 30, 2024: member

lgtm 95897ff181c0757e445f0e066a2a590a0a0120d2, cc @vasild

jonatack commented at 6:27 PM on May 1, 2024: member

I'm not sure this should be removed unless it has been observed to no longer be the case, and in the meantime it does no harm. See also #30014#pullrequestreview-2034095091.

kevkevinpal commented at 6:39 PM on May 1, 2024: contributor

I'm not sure this should be removed unless it has been observed to no longer be the case, and in the meantime it does no harm. See also #30014 (review).

that's fair I can close this for now, if someone does have concrete statistics feel free to pick this up

kevkevinpal closed this on May 1, 2024

vasild commented at 1:24 PM on May 10, 2024: contributor

ACK 95897ff181c0757e445f0e066a2a590a0a0120d2

Why exactly was this closed?

I'm not sure this should be removed unless it has been observed to no longer be the case

According to https://bitnodes.io/dashboard/, ~85% of public nodes are running 23 or higher

Those are (currently) 82.3% of the listening nodes. I think "unlikely to get incoming connections" is not true anymore and this sentence can be dropped.

What about non-listening nodes? Incoming connections come also from them. Maybe assume same distribution of 82.3%? Significant part of them are not Bitcoin Core, I guess, but then the "avoid non-8333 port" thing that was removed in 23.0 was Bitcoin Core specific. Even if none of the non-listening nodes make any connections (!?) then a non-8333 port node will get (a lot of) incoming connections from the rest of the 82.3% of the listening nodes.

Currently my node which listens on non-8333 port has 111 incoming connections:

$ bitcoin-cli getpeerinfo |jq 'map(select(.inbound)) |length'
111

actually, it has been full of incoming connections ever since, even e.g. 2 years ago. Maybe that "unlikely to get incoming connections" was inaccurate in the first place. Should have been "unlikely to get incoming connections from Bitcoin Core".

jonatack commented at 4:29 PM on May 10, 2024: member

@vasild thank you, rectifying my comment.

Relevant change in v23: https://github.com/bitcoin/bitcoin/pull/23542/files#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R2124

New help:

$ ./src/bitcoind -h | grep -A2 "\-port\="
  -port=<port>
       Listen for connections on <port> (default: 8333, testnet: 18333, signet:
       38333, regtest: 18444). Not relevant for I2P (see doc/i2p.md).

ACK 95897ff181c0757e445f0e066a2a590a0a0120d2

kevkevinpal reopened this on May 10, 2024

kevkevinpal commented at 5:00 PM on May 10, 2024: contributor

Why exactly was this closed?

reopened, thanks for providing additional insight! I initially closed since I did not have this info

kristapsk approved

kristapsk commented at 5:07 PM on May 10, 2024: contributor

ACK 95897ff181c0757e445f0e066a2a590a0a0120d2. According to https://bitnodes.io/dashboard/#user-agents stats, most nodes on the network are v23+.

vasild commented at 8:17 AM on May 11, 2024: contributor

Not directly related to this PR, but somewhat on topic:

# total addresses in addrman
$ bitcoin-cli getnodeaddresses 0 |jq 'length' 
73425

# number of addresses with != 8333 port
$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.port != 8333)) |map(select(.network != "i2p")) |length'
3667

# distribution of != 8333 ports
$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.port != 8333)) |map(select(.network != "i2p")) |.[].port' |sort |uniq -c |sort -r
1952 39388
 158 8332
 151 9333
 143 8334
 128 0
  75 8446
  75 18333
  73 8335
  70 12333
  57 8433
  38 10001
  24 7333
  24 20008
  23 8338
  22 8444
  17 8331
  17 30034
  16 8303
  14 8330
  12 8555
  12 8336
  11 28333
  10 6333
   9 8393
   9 5001
   8 8343
   7 9001
   7 8337
   7 18888
   6 8833
   6 20011
   6 11080
   5 8443
   5 8000
   5 38330
   5 20000
   4 9334
   4 8666
   4 8341
   4 8060
   4 58333
   4 48732
   4 25001
   4 2033
   4 2008
   4 2001
   4 14333
   4 12772
   4 12715
   4 11520
   4 10333
   3 8889
   3 8533
   3 8531
   3 8442
   3 8363
   3 8352
   3 8344
   3 8339
   3 8321
   3 8313
   3 8233
   3 61509
   3 60221
   3 59050
   3 57457
   3 57454
   3 57453
   3 57080
   3 55000
   3 48333
   3 40000
   3 39746
   3 34241
   3 3000
   3 29421
   3 25002
   3 24081
   3 21001
   3 19070
   3 18334
   3 18332
   2 9332
   2 8434
   2 8395
   2 8394
   2 8392
   2 8391
   2 8345
   2 8340
   2 8255
   2 8253
   2 8183
   2 8101
   2 8092
   2 7833
   2 7332
   2 64393
   2 60938
   2 6000
   2 5941
   2 58753
   2 5866
   2 58148
   2 58124
   2 57459
   2 57456
   2 55544
   2 54886
   2 5141
   2 50021
   2 48539
   2 38333
   2 37777
   2 35000
   2 3335
   2 32000
   2 30008
   2 29256
   2 28833
   2 28633
   2 28332
   2 25003
   2 24585
   2 24355
   2 22220
   2 21066
   2 21002
   2 20066
   2 20018
   2 2000
   2 19331
   2 19080
   2 19030
   2 19010
   2 19000
   2 18734
   2 18444
   2 18104
   2 18001
   2 15718
   2 14787
   2 12853
   2 12112
   2 12000
   2 11333
   2 11108
   2 10313
   2 10100
   1 9933
   1 9876
   1 9713
   1 9654
   1 9600
   1 9501
   1 9444
   1 9421
   1 9393
   1 9388
   1 9313
   1 9250
   1 9199
   1 9170
   1 9154
   1 9090
   1 9020
   1 8963
   1 8888
   1 8887
   1 8885
   1 8837
   1 8800
   1 8777
   1 8765
   1 8663
   1 8633
   1 8530
   1 8520
   1 8518
   1 8517
   1 8512
   1 8511
   1 8501
   1 8500
   1 8401
   1 8373
   1 8361
   1 8355
   1 8354
   1 8353
   1 8348
   1 8347
   1 8346
   1 8342
   1 8316
   1 8267
   1 8222
   1 8180
   1 8145
   1 8034
   1 8031
   1 8022
   1 8001
   1 7907
   1 7870
   1 7777
   1 7555
   1 7433
   1 7012
   1 65368
   1 65333
   1 65287
   1 6433
   1 64264
   1 6359
   1 63229
   1 62333
   1 61977
   1 61567
   1 61112
   1 61111
   1 60969
   1 60501
   1 60373
   1 60104
   1 59416
   1 59219
   1 59077
   1 58787
   1 57961
   1 57455
   1 57452
   1 57451
   1 56447
   1 55222
   1 54774
   1 54447
   1 531
   1 52853
   1 52101
   1 5110
   1 50501
   1 5000
   1 49618
   1 48331
   1 48123
   1 48001
   1 46552
   1 46220
   1 44853
   1 42434
   1 42408
   1 42211
   1 42197
   1 40153
   1 39765
   1 38945
   1 38000
   1 35703
   1 33890
   1 3333
   1 3317
   1 32768
   1 31243
   1 31239
   1 31001
   1 30200
   1 3012
   1 30112
   1 30111
   1 29334
   1 29016
   1 28884
   1 2866
   1 28643
   1 28011
   1 27777
   1 27269
   1 27001
   1 24001
   1 22311
   1 22146
   1 22002
   1 21118
   1 20993
   1 20250
   1 20202
   1 20101
   1 20099
   1 20001
   1 19911
   1 19773
   1 19333
   1 19100
   1 19060
   1 19050
   1 19040
   1 19020
   1 18365
   1 18335
   1 18331
   1 18300
   1 18276
   1 17777
   1 17482
   1 1691
   1 16588
   1 15698
   1 15478
   1 15426
   1 14888
   1 14878
   1 14584
   1 12905
   1 12345
   1 1031
   1 10303
   1 1026
   1 10050
   1 10020
   1 10011
   1 10000

jonatack commented at 7:10 PM on May 11, 2024: member

@vasild interesting 👍

here on a non-clearnet node (with addnode to a colleague via ipv4)

$ ./src/bitcoin-cli getnodeaddresses 0 |jq 'length'                                                            
19787

$ ./src/bitcoin-cli getnodeaddresses 0 |jq 'map(select(.port != 8333)) |map(select(.network != "i2p")) |length'
16

$ ./src/bitcoin-cli getnodeaddresses 0 |jq 'map(select(.port != 8333)) |map(select(.network != "i2p")) |.[].port' |sort |uniq -c |sort -r
   4 8334
   1 9333
   1 9185
   1 9180
   1 9050
   1 8434
   1 8335
   1 8332
   1 8330
   1 58333
   1 42434
   1 28001
   1 14388

tdb3 commented at 6:13 PM on May 12, 2024: contributor

Additional data. Two Tor-only nodes (v27.0):

$ bitcoin-cli getnodeaddresses 0 | jq 'length'
16551
$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.port != 8333)) |map(select(.network != "i2p")) |length'
18
$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.port != 8333)) |map(select(.network != "i2p")) |.[].port' |sort |uniq -c |sort -r
      6 8334
      1 9333
      1 9185
      1 9180
      1 9050
      1 8433
      1 8335
      1 8332
      1 8330
      1 58333
      1 42434
      1 28001
      1 14388

$ bitcoin-cli getnodeaddresses 0 | jq 'length'
16347
$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.port != 8333)) |map(select(.network != "i2p")) |length'
16
$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.port != 8333)) |map(select(.network != "i2p")) |.[].port' |sort |uniq -c |sort -r
      5 8334
      1 9333
      1 9185
      1 9180
      1 8434
      1 8335
      1 8332
      1 8330
      1 58333
      1 42434
      1 28001
      1 14388

glozow merged this on May 13, 2024

glozow closed this on May 13, 2024

bitcoin locked this on May 14, 2025