Allow to peers behind NAT to get up to preferred_max connections by hashmap · Pull Request #2543 · mimblewimble/grin

hashmap · 2019-02-08T15:31:16Z

If peer has only outbound connections it's most likely behind NAT and we should not stop it from getting more outbound connections.

With that change a node with a public IP eventually still has 4 outbounds peers, but node behind the NAT jumps higher than 8 from time to time.

If peer has only outbound connections it's mot likely behind NAT and we should not stop it from getting more outbound connections

ignopeverell · 2019-02-09T19:46:16Z

Doesn't that mean that NAT'd peers are always going to be polling others for more peers?

hashmap · 2019-02-09T20:38:27Z

We still have max_preferred_peers limit. As I understand an additional check was added to limit number of outbound peers, it's why stuck with 4 outbounds peers by default no matter what. In my previous pr I increased this limit to min_preferred_peers, with this fix a node with a public IP will stop getting more outbound peers (and eventually will have 4), while natted node will nicely oscillate between max and min pref peers. Also it's more semantically correct to not call a case when we have only outbound peers healthy_mix

ignopeverell · 2019-02-12T23:04:35Z

Why the DNM change? Also I agree this behaves more correctly than previously, but it does look like the current code, with this merged, will keep nagging other peers for more?

hashmap · 2019-02-12T23:14:37Z

I had a glitch on my node, but I see now that it was unrelated to this change. The current code is too modest when runs behind NAT, it doesn't get more than min_peers limit just because we encourage it to get more inbound peers, which is against its nature.

ignopeverell · 2019-02-13T17:58:52Z

We may be slightly talking past each other. I tried this PR and it does the job. But even though I have 17 peers now (more than my preferred count), my node keeps asking other peers for their peer list:

20190213 04:53:39.252 WARN grin_servers::grin::seed - monitor_peers: 0.0.0.0:3414 ask 145.14.158.174:3414 for more peers
...
20190213 04:53:59.281 WARN grin_servers::grin::seed - monitor_peers: 0.0.0.0:3414 ask 145.14.158.174:3414 for more peers
...
20190213 04:54:19.315 WARN grin_servers::grin::seed - monitor_peers: 0.0.0.0:3414 ask 145.14.158.174:3414 for more peers
...

And that for every single peer I'm connected to. This is due to monitor_peers never being short-circuited now on NAT'd peers.

hashmap · 2019-02-13T18:42:38Z

@ignopeverell you are right. I introduced a wrong abstraction in my previous PR by extracting a duplicate code into a function. However in some cases code duplication is just coincidence and may be better than a wrong abstraction.
Anyway, that additional condition is applicable to only one place when we call this function, so I will put it there, not in the function.

hashmap · 2019-02-14T14:00:04Z

Don't see it anymore:

tail -f ~/.grin/main/grin-server.log | grep monitor_peers
20190214 14:56:35.747 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 8 connected (8 most_work). all 11219 = 443 healthy + 0 banned + 10776 defunct
20190214 14:56:35.747 DEBUG grin_servers::grin::seed - monitor_peers: no preferred peers
20190214 14:56:56.087 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 7 connected (7 most_work). all 11219 = 444 healthy + 0 banned + 10775 defunct
20190214 14:56:56.087 DEBUG grin_servers::grin::seed - monitor_peers: no preferred peers
20190214 14:57:16.262 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 10 connected (6 most_work). all 11220 = 425 healthy + 0 banned + 10795 defunct
20190214 14:57:16.262 DEBUG grin_servers::grin::seed - monitor_peers: no preferred peers
20190214 14:57:36.463 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 10 connected (9 most_work). all 11220 = 426 healthy + 0 banned + 10794 defunct
20190214 14:57:36.463 DEBUG grin_servers::grin::seed - monitor_peers: no preferred peers
20190214 14:57:56.573 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 6 connected (6 most_work). all 11220 = 427 healthy + 0 banned + 10793 defunct
20190214 14:57:56.573 DEBUG grin_servers::grin::seed - monitor_peers: no preferred peers
20190214 14:58:16.918 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 7 connected (7 most_work). all 11220 = 404 healthy + 0 banned + 10816 defunct
20190214 14:58:16.918 DEBUG grin_servers::grin::seed - monitor_peers: no preferred peers
20190214 14:58:37.588 DEBUG grin_servers::grin::seed - monitor_peers: on 0.0.0.0:3414, 10 connected (10 most_work). all 11243 = 408 healthy + 0 banned + 10835 defunct

ignopeverell · 2019-02-14T18:49:10Z

 	peers.clean_peers(config.peer_max_count() as usize);

-	if peers.healthy_peers_mix() {
+	if peers.peer_count() > peers.peer_outbound_count() && peers.healthy_peers_mix() {


Should that be >=?

Don’t think so, but not sure anymore:) the logic is - exit if we have at least 1 inbound peer and we have enough outbounds

But for NAT'd peers there'll never be any inbound? So they could have 100 oubounds and this will still never return true?

Oh, wrong place to add this check, thanks for spotting it. It's supposed to be where we connect

antiochp

I know its just a one line change but it feels obfuscated in terms of what we're actually trying to achieve here.

antiochp · 2019-02-25T09:09:42Z


 	// If we have a healthy number of outbound peers then we are done here.
-	if peers.healthy_peers_mix() {
+	if peers.peer_count() > peers.peer_outbound_count() && peers.healthy_peers_mix() {


So we're basically saying we do not need to try initiating more outbound connections if -

at least one inbound connection, and

we have a healthy_peers_mix

And healthy_peers_mix is -

peer_count >= min_preferred, and

outbound_count >= min_preferred / 2

And that first check (at least one inbound connection) is basically to determine if we are behind a NAT or not?

I think I get the logic but it feels confusing.

Is it worth introducing a peer.peer_inbound_count() fn to make this more a little more explicit?
And then branch clearly on that first check - "do this if we think we are a public node, otherwise do this other thing"?

hashmap · 2019-02-25T10:08:49Z

@antiochp

And that first check (at least one inbound connection) is basically to determine if we are behind a NAT or not?

Yes, you can also read it that way - if we have only outbounds peers there is no need to check how healthy the mix is, because there is no mix.

Is it worth introducing a peer.peer_inbound_count() fn to make this more a little more explicit?

That's what I did in the beginning, but such check is pretty expensive

antiochp

👍

…blewimble#2543) Allow to peers behind NAT to get up to preffered_max connections If peer has only outbound connections it's mot likely behind NAT and we should not stop it from getting more outbound connections

@yeastplume

* cleanup legacy "3 dot" check (#2625) * Allow to peers behind NAT to get up to preferred_max connections (#2543) Allow to peers behind NAT to get up to preffered_max connections If peer has only outbound connections it's mot likely behind NAT and we should not stop it from getting more outbound connections * Reduce usage of unwrap in p2p crate (#2627) Also change store crate a bit * Simplify (and fix) output_pos cleanup during chain compaction (#2609) * expose leaf pos iterator use it for various things in txhashset when iterating over outputs * fix * cleanup * rebuild output_pos index (and clear it out first) when compacting the chain * fixup tests * refactor to match on (output, proof) tuple * add comments to compact() to explain what is going on. * get rid of some boxing around the leaf_set iterator * cleanup * [docs] Add switch commitment documentation (#2526) * remove references to no-longer existing switch commitment hash (as switch commitments were removed in ca8447f and moved into the blinding factor of the Pedersen Commitment) * some rewording (points vs curves) and fix of small formatting issues * Add switch commitment documentation * [docs] Documents in grin repo had translated in Korean. (#2604) * Start to M/W intro translate in Korean * translate in Korean * add korean translation on intro * table_of_content.md translate in Korean. * table_of_content_KR.md finish translate in Korean, start to translate State_KR.md * add state_KR.md & commit some translation in State_KR.md * WIP stat_KR.md translation * add build_KR.md && stratum_KR.md * finish translate stratum_KR.md & table_of_content_KR.md * rename intro.KR.md to intro_KR.md * add intro_KR.md file path each language's intro.md * add Korean translation file path to stratum.md & table_of_contents.md * fix difference with grin/master * Fix TxHashSet file filter for Windows. (#2641) * Fix TxHashSet file filter for Windows. * rustfmt * Updating regexp * Adding in test case * Display the current download rate rather than the average when syncing the chain (#2633) * When syncing the chain, calculate the displayed download speed using the current rate from the most recent iteration, rather than the average download speed from the entire syncing process. * Replace the explicitly ignored variables in the pattern with an implicit ignore * remove root = true from editorconfig (#2655) * Add Medium post to intro (#2654) Spoke to @yeastplume who agreed it makes sense to add the "Grin Transactions Explained, Step-by-Step" Medium post to intro.md Open for suggestions on a better location. * add a new configure item for log_max_files (#2601) * add a new configure item for log_max_files * rustfmt * use a constant instead of multiple 32 * rustfmt * Fix the build warning of deprecated trim_right_matches (#2662) * [DOC] state.md, build.md and chain directory documents translate in Korean. (#2649) * add md files for translation. * start to translation fast-sync, code_structure. add file build_KR.md, states_KR.md * add dandelion_KR.md && simulation_KR.md for Korean translation. * add md files for translation. * start to translation fast-sync, code_structure. add file build_KR.md, states_KR.md * add dandelion_KR.md && simulation_KR.md for Korean translation. * remove some useless md files for translation. this is rearrange set up translation order. * add dot end of sentence & translate build.md in korean * remove fast-sync_KR.md * finish build_KR.md translation * finish build_KR.md translation * finish translation state_KR.md & add phrase in state.md to move other language md file * translate blocks_and_headers.md && chain_sync.md in Korean * add . in chain_sync.md , translation finished in doc/chain dir. * fix some miss typos * Api documentation fixes (#2646) * Fix the API documentation for Chain Validate (v1/chain/validate). It was documented as a POST, but it is actually a GET request, which can be seen in its handler ChainValidationHandler * Update the API V1 route list response to include the headers and merkleproof routes. Also clarify that for the chain/outputs route you must specify either byids or byheight to select outputs. * refactor(ci): reorganize CI related code (#2658) Break-down the CI related code into smaller more maintainable pieces. * Specify grin or nanogrins in API docs where applicable (#2642) * Set Content-Type in API client (#2680) * Reduce number of unwraps in chain crate (#2679) * fix: the restart of state sync doesn't work sometimes (#2687) * let check_txhashset_needed return true on abnormal case (#2684) * Reduce number of unwwaps in api crate (#2681) * Reduce number of unwwaps in api crate * Format use section * Small QoL improvements for wallet developers (#2651) * Small changes for wallet devs * Move create_nonce into Keychain trait * Replace match by map_err * Add flag to Slate to skip fee check * Fix secp dependency * Remove check_fee flag in Slate * Add Japanese edition of build.md (#2697) * catch the panic to avoid peer thread quit early (#2686) * catch the panic to avoid peer thread quit before taking the chance to ban * move catch wrapper logic down into the util crate * log the panic info * keep txhashset.rs untouched * remove a warning * [DOC] dandelion.md, simulation.md ,fast-sync.md and pruning.md documents translate in Korean. (#2678) * Show response code in API client error message (#2683) It's hard to investigate what happens when an API client error is printed out * Add some better logging for get_outputs_by_id failure states (#2705) * Switch commitment doc fixes (#2645) Fix some typos and remove the use of parentheses in a couple of places to make the reading flow a bit better. * docs: update/add new README.md badges (#2708) Replace existing badges with SVG counterparts and add a bunch of new ones. * Update intro.md (#2702) Add mention of censoring attack prevented by range proofs * use sandbox folder for txhashset validation on state sync (#2685) * use sandbox folder for txhashset validation on state sync * rustfmt * use temp directory as the sandbox instead actual db_root txhashset dir * rustfmt * move txhashset overwrite to the end of full validation * fix travis-ci test * rustfmt * fix: hashset have 2 folders including txhashset and header * rustfmt * (1)switch to rebuild_header_mmr instead of copy the sandbox header mmr (2)lock txhashset when overwriting and opening and rebuild * minor improve on sandbox_dir * add Japanese edition of state.md (#2703) * Attempt to fix broken TUI locale (#2713) Can confirm that on the same machine 1.0.2 TUI looks great and is broken on the current master. Bump of `cursive` version fixed it for me. Fixes #2676 * clean the header folder in sandbox (#2716) * forgot to clean the header folder in sandbox in #2685 * Reduce number of unwraps in servers crate (#2707) It doesn't include stratum server which is sufficiently changed in 1.1 branch and adapters, which is big enough for a separate PR. * rustfmt * change version to beta

…blewimble#2543) Allow to peers behind NAT to get up to preffered_max connections If peer has only outbound connections it's mot likely behind NAT and we should not stop it from getting more outbound connections

@yeastplume

* cleanup legacy "3 dot" check (mimblewimble#2625) * Allow to peers behind NAT to get up to preferred_max connections (mimblewimble#2543) Allow to peers behind NAT to get up to preffered_max connections If peer has only outbound connections it's mot likely behind NAT and we should not stop it from getting more outbound connections * Reduce usage of unwrap in p2p crate (mimblewimble#2627) Also change store crate a bit * Simplify (and fix) output_pos cleanup during chain compaction (mimblewimble#2609) * expose leaf pos iterator use it for various things in txhashset when iterating over outputs * fix * cleanup * rebuild output_pos index (and clear it out first) when compacting the chain * fixup tests * refactor to match on (output, proof) tuple * add comments to compact() to explain what is going on. * get rid of some boxing around the leaf_set iterator * cleanup * [docs] Add switch commitment documentation (mimblewimble#2526) * remove references to no-longer existing switch commitment hash (as switch commitments were removed in ca8447f and moved into the blinding factor of the Pedersen Commitment) * some rewording (points vs curves) and fix of small formatting issues * Add switch commitment documentation * [docs] Documents in grin repo had translated in Korean. (mimblewimble#2604) * Start to M/W intro translate in Korean * translate in Korean * add korean translation on intro * table_of_content.md translate in Korean. * table_of_content_KR.md finish translate in Korean, start to translate State_KR.md * add state_KR.md & commit some translation in State_KR.md * WIP stat_KR.md translation * add build_KR.md && stratum_KR.md * finish translate stratum_KR.md & table_of_content_KR.md * rename intro.KR.md to intro_KR.md * add intro_KR.md file path each language's intro.md * add Korean translation file path to stratum.md & table_of_contents.md * fix difference with grin/master * Fix TxHashSet file filter for Windows. (mimblewimble#2641) * Fix TxHashSet file filter for Windows. * rustfmt * Updating regexp * Adding in test case * Display the current download rate rather than the average when syncing the chain (mimblewimble#2633) * When syncing the chain, calculate the displayed download speed using the current rate from the most recent iteration, rather than the average download speed from the entire syncing process. * Replace the explicitly ignored variables in the pattern with an implicit ignore * remove root = true from editorconfig (mimblewimble#2655) * Add Medium post to intro (mimblewimble#2654) Spoke to @yeastplume who agreed it makes sense to add the "Grin Transactions Explained, Step-by-Step" Medium post to intro.md Open for suggestions on a better location. * add a new configure item for log_max_files (mimblewimble#2601) * add a new configure item for log_max_files * rustfmt * use a constant instead of multiple 32 * rustfmt * Fix the build warning of deprecated trim_right_matches (mimblewimble#2662) * [DOC] state.md, build.md and chain directory documents translate in Korean. (mimblewimble#2649) * add md files for translation. * start to translation fast-sync, code_structure. add file build_KR.md, states_KR.md * add dandelion_KR.md && simulation_KR.md for Korean translation. * add md files for translation. * start to translation fast-sync, code_structure. add file build_KR.md, states_KR.md * add dandelion_KR.md && simulation_KR.md for Korean translation. * remove some useless md files for translation. this is rearrange set up translation order. * add dot end of sentence & translate build.md in korean * remove fast-sync_KR.md * finish build_KR.md translation * finish build_KR.md translation * finish translation state_KR.md & add phrase in state.md to move other language md file * translate blocks_and_headers.md && chain_sync.md in Korean * add . in chain_sync.md , translation finished in doc/chain dir. * fix some miss typos * Api documentation fixes (mimblewimble#2646) * Fix the API documentation for Chain Validate (v1/chain/validate). It was documented as a POST, but it is actually a GET request, which can be seen in its handler ChainValidationHandler * Update the API V1 route list response to include the headers and merkleproof routes. Also clarify that for the chain/outputs route you must specify either byids or byheight to select outputs. * refactor(ci): reorganize CI related code (mimblewimble#2658) Break-down the CI related code into smaller more maintainable pieces. * Specify grin or nanogrins in API docs where applicable (mimblewimble#2642) * Set Content-Type in API client (mimblewimble#2680) * Reduce number of unwraps in chain crate (mimblewimble#2679) * fix: the restart of state sync doesn't work sometimes (mimblewimble#2687) * let check_txhashset_needed return true on abnormal case (mimblewimble#2684) * Reduce number of unwwaps in api crate (mimblewimble#2681) * Reduce number of unwwaps in api crate * Format use section * Small QoL improvements for wallet developers (mimblewimble#2651) * Small changes for wallet devs * Move create_nonce into Keychain trait * Replace match by map_err * Add flag to Slate to skip fee check * Fix secp dependency * Remove check_fee flag in Slate * Add Japanese edition of build.md (mimblewimble#2697) * catch the panic to avoid peer thread quit early (mimblewimble#2686) * catch the panic to avoid peer thread quit before taking the chance to ban * move catch wrapper logic down into the util crate * log the panic info * keep txhashset.rs untouched * remove a warning * [DOC] dandelion.md, simulation.md ,fast-sync.md and pruning.md documents translate in Korean. (mimblewimble#2678) * Show response code in API client error message (mimblewimble#2683) It's hard to investigate what happens when an API client error is printed out * Add some better logging for get_outputs_by_id failure states (mimblewimble#2705) * Switch commitment doc fixes (mimblewimble#2645) Fix some typos and remove the use of parentheses in a couple of places to make the reading flow a bit better. * docs: update/add new README.md badges (mimblewimble#2708) Replace existing badges with SVG counterparts and add a bunch of new ones. * Update intro.md (mimblewimble#2702) Add mention of censoring attack prevented by range proofs * use sandbox folder for txhashset validation on state sync (mimblewimble#2685) * use sandbox folder for txhashset validation on state sync * rustfmt * use temp directory as the sandbox instead actual db_root txhashset dir * rustfmt * move txhashset overwrite to the end of full validation * fix travis-ci test * rustfmt * fix: hashset have 2 folders including txhashset and header * rustfmt * (1)switch to rebuild_header_mmr instead of copy the sandbox header mmr (2)lock txhashset when overwriting and opening and rebuild * minor improve on sandbox_dir * add Japanese edition of state.md (mimblewimble#2703) * Attempt to fix broken TUI locale (mimblewimble#2713) Can confirm that on the same machine 1.0.2 TUI looks great and is broken on the current master. Bump of `cursive` version fixed it for me. Fixes mimblewimble#2676 * clean the header folder in sandbox (mimblewimble#2716) * forgot to clean the header folder in sandbox in mimblewimble#2685 * Reduce number of unwraps in servers crate (mimblewimble#2707) It doesn't include stratum server which is sufficiently changed in 1.1 branch and adapters, which is big enough for a separate PR. * rustfmt * change version to beta

Allow to peers behind NAT to get up to preffered_max connections

40820d9

If peer has only outbound connections it's mot likely behind NAT and we should not stop it from getting more outbound connections

hashmap added the enhancement label Feb 8, 2019

hashmap changed the title ~~Allow to peers behind NAT to get up to preffered_max connections~~ [DNM] Allow to peers behind NAT to get up to preffered_max connections Feb 12, 2019

hashmap changed the title ~~[DNM] Allow to peers behind NAT to get up to preffered_max connections~~ Allow to peers behind NAT to get up to preffered_max connections Feb 12, 2019

Alter only peer connection logic

7ec3f35

ignopeverell reviewed Feb 14, 2019

View reviewed changes

Move the condition to the right place

5ad4c0a

hashmap added this to the 1.0.2 milestone Feb 19, 2019

hashmap requested a review from antiochp February 24, 2019 21:00

antiochp reviewed Feb 25, 2019

View reviewed changes

antiochp changed the title ~~Allow to peers behind NAT to get up to preffered_max connections~~ Allow to peers behind NAT to get up to preferred_max connections Feb 25, 2019

antiochp approved these changes Feb 25, 2019

View reviewed changes

hashmap modified the milestones: 1.0.2, 1.0.3 Feb 25, 2019

hashmap merged commit 224a315 into mimblewimble:master Feb 25, 2019

hashmap deleted the more-outbound-nat branch February 25, 2019 15:29

Conversation

hashmap commented Feb 8, 2019

Uh oh!

ignopeverell commented Feb 9, 2019

Uh oh!

hashmap commented Feb 9, 2019

Uh oh!

ignopeverell commented Feb 12, 2019

Uh oh!

hashmap commented Feb 12, 2019

Uh oh!

ignopeverell commented Feb 13, 2019

Uh oh!

hashmap commented Feb 13, 2019

Uh oh!

hashmap commented Feb 14, 2019

Uh oh!

ignopeverell Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

hashmap Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

ignopeverell Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

hashmap Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

antiochp left a comment

Choose a reason for hiding this comment

Uh oh!

antiochp Feb 25, 2019 • 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!

hashmap commented Feb 25, 2019

Uh oh!

antiochp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

antiochp Feb 25, 2019 •

edited

Loading