Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: remove dead hardcoded addnodes #2116

Merged
merged 5 commits into from May 2, 2021
Merged

wallet: remove dead hardcoded addnodes #2116

merged 5 commits into from May 2, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 24, 2021

Nobody asked for it but I did it anyway.
Don't know if this will have implication on Testnet if the added nodes don't run Testnet.
Added new nodes and removed dead ones where the hostname resolution failed according to https://addnode.cycy.me

sweede-se added 2 commits April 24, 2021 10:41
Nobody asked for it but I did it anyway.
Don't know if this will have implication on Testnet if the added nodes don't run Testnet.
Added new nodes and removed dead ones where the hostname resolution failed according to https://addnode.cycy.me
@ghost ghost marked this pull request as ready for review April 24, 2021 09:55
Copy link
Contributor

@RoboticMind RoboticMind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to check with all these operators that they're okay with their nodes being added as defaults

src/init.cpp Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Apr 24, 2021

Probably want to check with all these operators that they're okay with their nodes being added as defaults

@RoboticMind When doing this I didn't see that ppl would have a problem with it if they already was on the add node page. The only of the added ones I know who is running is CallMeFoxie. Will check if it's OK, otherwise remove. The ones unknown to me will be removed.

@ghost ghost marked this pull request as draft April 24, 2021 18:04
@ghost
Copy link
Author

ghost commented Apr 24, 2021

CallMeFoxie is OK with being added by default.

@ghost ghost marked this pull request as ready for review April 24, 2021 18:22
@ghost
Copy link
Author

ghost commented Apr 24, 2021

Summary: Two dead nodes removed. Added CallMeFoxie's and my own to the list. Total number of nodes the same but all are now active.

@RoboticMind
Copy link
Contributor

Probably want to check with all these operators that they're okay with their nodes being added as defaults

@RoboticMind When doing this I didn't see that ppl would have a problem with it if they already was on the add node page. The only of the added ones I know who is running is CallMeFoxie. Will check if it's OK, otherwise remove. The ones unknown to me will be removed.

After they get added, there's likely to be some higher traffic, so it could be more than they are ready for at the moment. It's at least worth letting them know beforehand in case they want to do any adjustments to their node

src/init.cpp Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Apr 25, 2021

After they get added, there's likely to be some higher traffic, so it could be more than they are ready for at the moment. It's at least worth letting them know beforehand in case they want to do any adjustments to their node

@RoboticMind They are no longer in the commit.

@ghost ghost marked this pull request as draft April 26, 2021 06:47
@ghost ghost marked this pull request as ready for review April 28, 2021 04:55
@cyrossignol
Copy link
Member

A bit about addnodes... these are peers that a wallet will aggressively maintain connections to. With eight active addnodes in the default list, nodes that cannot accept inbound connections may not connect to a non-default peer. As we can imagine, this isn't great for decentralization. This list was created to help support peer discovery early on, and it has been updated since, but we should be careful about expanding it. Ideally, we should shrink this list and replace it when we've updated the seednode option to support DNS resolution from newer Bitcoin code.

I have no issues with the proposed nodes/operators, and I appreciate the clean-up, but I don't think we want to add more entries to the default list. Many users won't know or bother to change the configuration.

@ghost
Copy link
Author

ghost commented Apr 30, 2021

A bit about addnodes... these are peers that a wallet will aggressively maintain connections to. With eight active addnodes in the default list, nodes that cannot accept inbound connections may not connect to a non-default peer. As we can imagine, this isn't great for decentralization. This list was created to help support peer discovery early on, and it has been updated since, but we should be careful about expanding it. Ideally, we should shrink this list and replace it when we've updated the seednode option to support DNS resolution from newer Bitcoin code.

I have no issues with the proposed nodes/operators, and I appreciate the clean-up, but I don't think we want to add more entries to the default list. Many users won't know or bother to change the configuration.

@cyrossignol The same number of addresses that was added was also removed, so no expansion have been made in the final commit. Do you suggest just removing the non working nodes and not replacing them? I'm up for what ever is best for the network.

@barton2526
Copy link
Member

My opinion is full removal of the dead nodes and no new additions. Hard coded seed nodes should be minimized not extended. There are many security implications of running a hard coded seed node that these new operators may not be aware of.

@ghost
Copy link
Author

ghost commented May 2, 2021

There are no risk that seed.gridcoin.pl that load balances several nodes will take the remaining spots not filled up by addnodes? And if it does, will that not be bad for the decentralisation?

@ghost ghost marked this pull request as draft May 2, 2021 08:07
@nathanielcwm
Copy link
Contributor

Afaik seed.gridcoin.pl acts simply just as a dns server.

If this is the case then the wallet should only attempt to connect to one at a time. @sweede-se

@cyrossignol
Copy link
Member

That's correct--a wallet will connect to the first address resolved from DNS that responds and skip the rest. It looks like seed.gridcoin.pl shuffles the addresses for each lookup to balance the load.

The same number of addresses that was added was also removed, so no expansion have been made in the final commit. Do you suggest just removing the non working nodes and not replacing them?

@sweede-se I recommend that we avoid adding new entries until we lose at least a couple more of the existing defaults. The dead nodes mitigated some of the unwanted effect of the list. I hate to say that after you spent so much time working on the PR... The clean-up is surely still welcome, and we can consider adding the new nodes as seednode entries when we get that working with DNS in the future.

@ghost
Copy link
Author

ghost commented May 2, 2021

@cyrossignol No worries, and not that much time was spent. I'm totally for that the end result should the best for the network, and it seems like we have that solution now. Have made a new commit with the dead nodes removed and no new added.

Don't really know why the checks failed. Can someone enlighten me?

@div72
Copy link
Member

div72 commented May 2, 2021

@sweede-se Related to bintray returning a 403 for boost depends, making a PR.

@ghost
Copy link
Author

ghost commented May 2, 2021

@sweede-se Related to bintray returning a 403 for boost depends, making a PR.

@div72 So I guess it would be ok to mark this PR as Ready for review?

@ghost ghost marked this pull request as ready for review May 2, 2021 19:32
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@jamescowens jamescowens changed the title wallet: update addnode wallet: remove dead addnodes May 2, 2021
@jamescowens jamescowens changed the title wallet: remove dead addnodes wallet: remove dead hardcoded addnodes May 2, 2021
@jamescowens jamescowens merged commit 851744b into gridcoin-community:development May 2, 2021
@ghost ghost deleted the patch-3 branch May 2, 2021 21:42
@ghost ghost restored the patch-3 branch May 2, 2021 21:42
@jamescowens jamescowens assigned ghost May 10, 2021
@jamescowens jamescowens added the deprecation Removed deprecated functionality label May 10, 2021
@jamescowens jamescowens added this to the Ingrid milestone May 10, 2021
jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Aug 1, 2021
Added
 - util, rpc. gui: Changes for snapshotdownload and add feature sync from zero gridcoin-community#2093 (@iFoggz)
 - gui: Implement GUI version of consolidateunspent (coin control part) gridcoin-community#2111 (@jamescowens)
 - gui: Implement consolidateunspent wizard gridcoin-community#2125 (@jamescowens)
 - qt: Add antialiasing to traffic graph widget gridcoin-community#2150 (@barton2526)
 - util: Port of ArgsManager and a significant subset of src/util gridcoin-community#2146 (@jamescowens)
 - doc: add issue templates for bug reports and feature requests gridcoin-community#2147 (@Pythonix)
 - gui, rpc: Implement dynamic stakesplitting control, settings changes via rpc, and dynamic changes to sidestaking via rpc gridcoin-community#2164 (@jamescowens)
 - rpc: Implement getblocksbatch gridcoin-community#2205 (@jamescowens)
 - voting, rpc, gui: Implement demand loading of historical poll by poll id and AVW calculation gridcoin-community#2210 (@jamescowens)
 - gui: Show GUI error dialog if command line parsing fails gridcoin-community#2218 (@jamescowens)
 - gui: Implement close confirmation. gridcoin-community#2216 (@denravonska)
 - build: Use -fstack-reuse=none gridcoin-community#2232 (@barton2526)

Changed
 - doc: Update build doc gridcoin-community#2078 (@iFoggz)
 - gui: Normalize button and input control appearance gridcoin-community#2096 (@cyrossignol)
 - consensus: Implement GetMinimumRequiredConnectionsForStaking gridcoin-community#2097 (@jamescowens)
 - refactor: move CTransaction to primitives gridcoin-community#2006 (@div72)
 - consensus, refactor, test: Merkle gridcoin-community#2094 (@div72)
 - gui: Update diagnostics gridcoin-community#2095 (@jamescowens)
 - gui: Refresh UI styles and sidebar/statusbar design gridcoin-community#2102 (@cyrossignol)
 - gui: Set standard base Qt style on Windows and macOS gridcoin-community#2114 (@cyrossignol)
 - build, refactor: bump to C++17 gridcoin-community#2113 (@div72)
 - util, rpc, gui: Implement GetMaxInputsForConsolidationTxn() gridcoin-community#2119 (@jamescowens)
 - gui: Refresh overview page design gridcoin-community#2117 (@cyrossignol)
 - depends: change boost mirror gridcoin-community#2122 (@div72)
 - refactor: small cleanup gridcoin-community#2123 (@div72)
 - build: Update depends Qt recipe to version 5.12.10 gridcoin-community#2129 (@cyrossignol)
 - build: Bump Codespell to 2.0.0 gridcoin-community#2135 (@barton2526)
 - gui: Refresh "send coins" page design gridcoin-community#2126 (@cyrossignol)
 - gui: Optimize locks to improve responsiveness gridcoin-community#2137 (@cyrossignol)
 - gui: Refresh "receive payment" page design gridcoin-community#2138 (@cyrossignol)
 - gui: Add empty placeholder to recent transactions list gridcoin-community#2140 (@cyrossignol)
 - gui: Refresh transaction history page design gridcoin-community#2143 (@cyrossignol)
 - gui: Refresh address book page design gridcoin-community#2145 (@cyrossignol)
 - doc: Update http to https where possible gridcoin-community#2148 (@barton2526)
 - depends: Update dependencies gridcoin-community#2153 (@barton2526)
 - depends: Bump python to 3.6 gridcoin-community#2159 (@barton2526)
 - test: Update cppcheck linter to c++17 gridcoin-community#2157 (@barton2526)
 - test: Drop Travis specific workarounds, Mention commit id in error, Fix typos, Update spellcheck ignore words gridcoin-community#2158 (@barton2526)
 - gui: Overhaul the voting UI gridcoin-community#2151 (@cyrossignol)
 - wallet: simplify nTimeSmart calculation gridcoin-community#2144 (@div72)
 - gui: Refresh checkbox and radio button styles gridcoin-community#2170 (@cyrossignol)
 - build: Bump libevent to 2.1.11 gridcoin-community#2172 (@barton2526)
 - build: Update native_mac_alias, Remove big sur patch file in qt recipe gridcoin-community#2173 (@barton2526)
 - docs: Misc Grammar gridcoin-community#2176 (@barton2526)
 - build: miniupnpc 2.2.2 gridcoin-community#2179 (@barton2526)
 - rpc: Refresh rainbymagnitude gridcoin-community#2163 (@jamescowens)
 - util: optimize HexStr gridcoin-community#2185 (@div72)
 - refactor: misc style changes gridcoin-community#2177 (@div72)
 - rpc: consolidatemsunspent changes. gridcoin-community#2136 (@iFoggz)
 - refactor: Replace "GlobalStatus" state management gridcoin-community#2183 (@cyrossignol)
 - rpc, util: Remove use of ArgsManager::NETWORK_ONLY for now gridcoin-community#2190 (@jamescowens)
 - doc: Replace hidden service with onion service, Capitalize "Tor" gridcoin-community#2193 (@barton2526)
 - gui: Update Qt Linguist localization files gridcoin-community#2192 (@cyrossignol)
 - script: Shell script cleanups gridcoin-community#2195 (@barton2526)
 - build: set minimum required Boost to 1.58.0 gridcoin-community#2194 (@barton2526)
 - build, util: Prevent execution for Windows versions less than Windows 7 gridcoin-community#2203 (@jamescowens)
 - build: Tweak NSIS Windows installer gridcoin-community#2204 (@jamescowens)
 - build: Add bison in depends gridcoin-community#2206 (@iFoggz)
 - build: macOS toolchain bump gridcoin-community#2207 (@div72)
 - doc: Update build-unix.md gridcoin-community#2212 (@springfielddatarecovery)
 - build: Bump minimum python version to 3.6, Remove python2 references gridcoin-community#2219 (@barton2526)
 - depends: Change openSSL source path to Github gridcoin-community#2237 (@barton2526)
 - doc: Fix typo in bug report template gridcoin-community#2243 (@jamescowens)
 - ci: fold depends output gridcoin-community#2244 (@div72)

Removed
 - wallet: remove dead hardcoded addnodes gridcoin-community#2116 (@sweede-se)
 - rpc: Remove readconfig gridcoin-community#2248 (@jamescowens)
 - rpc: Remove obsolete comparesnapshotaccrual RPC function gridcoin-community#2100 (@jamescowens)
 - rpc: Remove memorypool RPC Command gridcoin-community#2214 (@RoboticMind)
 - rpc: Remove deprecated RPC commands gridcoin-community#2101 (@jamescowens)
 - Remove CCT from README, add Discord gridcoin-community#2134 (@barton2526)
 - refactor: Remove obsolete pubsub method definitions gridcoin-community#2191 (@barton2526)
 - refactor: Remove msMiningErrorsIncluded & msMiningErrorsExcluded gridcoin-community#2215 (@RoboticMind)
 - qt: Remove obsolete topLevelWidget(), Remove obsolete QRegExpValidator gridcoin-community#2198 (@barton2526)
 - net: Drop support of the insecure miniUPnPc versions gridcoin-community#2178 (@barton2526)
 - log: remove deprecated db log category gridcoin-community#2201 (@barton2526)
 - doc: Remove CCT from README and release process docs gridcoin-community#2175 (@barton2526)
 - build: Remove travis references gridcoin-community#2156 (@barton2526)

Fixed
 - gui: Fix macOS and designer font sizes gridcoin-community#2098 (@cyrossignol)
 - gui: Have the TrafficGraphWidget respect the selected stylesheet. gridcoin-community#2107 (@jamescowens)
 - gui: Fix macOS display inconsistencies gridcoin-community#2106 (@cyrossignol)
 - gui: Fix RPC console auto-complete background color gridcoin-community#2108 (@cyrossignol)
 - gui: Avoid reloading redundant stylesheets gridcoin-community#2109 (@cyrossignol)
 - gui: Fix "no active beacon" status gridcoin-community#2110 (@cyrossignol)
 - gui: Fix dark theme link text color visibility gridcoin-community#2115 (@cyrossignol)
 - scraper, util, qt: Fix several deprecations and warnings gridcoin-community#2131 (@jamescowens)
 - gui: Fix duplicate time in GUIUtil::dateTimeStr() gridcoin-community#2139 (@cyrossignol)
 - gui: Fix debug console traffic graph legend colors gridcoin-community#2142 (@cyrossignol)
 - gui: Fix nomenclature gridcoin-community#2104 (@jamescowens)
 - doc: Fix Typos gridcoin-community#2149 (@barton2526)
 - doc: Fix "master" branch build status badge in readme gridcoin-community#2167 (@cyrossignol)
 - gui: Fix Inter font rendering on Windows with FreeType gridcoin-community#2169 (@cyrossignol)
 - gui: Fix assert on non-existent data directory and GUI datadir chooser corner case issues gridcoin-community#2174 (@jamescowens)
 - gui: Fix display artifact in poll loading indicator gridcoin-community#2180 (@cyrossignol)
 - rpc, logging: Minor fixes for sidestake logging gridcoin-community#2187 (@jamescowens)
 - gui: Fix fractional scaling for dialog sizes gridcoin-community#2189 (@cyrossignol)
 - doc: Random fixes gridcoin-community#2197 (@barton2526)
 - doc: getbalance should say GRC not "btc" gridcoin-community#2199 (@barton2526)
 - net: Add missing verification of IPv6 address in CNetAddr::GetIn6Addr¦ gridcoin-community#2200 (@barton2526)
 - doc: remove duplicate line from .gitignore gridcoin-community#2202 (@Pythonix)
 - util: Tweak exception handling in MilliTimer class to eliminate compiler warnings gridcoin-community#2233 (@jamescowens)
 - depends: patch missing include in qt gridcoin-community#2234 (@div72)
 - wallet, rpc: Check each input for IsMine() in GetAddressGroupings gridcoin-community#2242 (@jamescowens)
 - util, qt: Fix snapshot download gridcoin-community#2246 (@jamescowens)
 - gui: Fix Column Widths in RPC Console. Elide long strings in their center. Indent user agent. gridcoin-community#2241 (@barton2526)
 - qt: Fix crash during download snapshot on macOS gridcoin-community#2250 (@jamescowens)
 - qt: Don't allow to open the debug window during splashscreen & verification state gridcoin-community#2245 (@barton2526)
 - gui: Fix address book selected model record when editing gridcoin-community#2253 (@cyrossignol)
 - researcher: Check wallet status before beacon renewal gridcoin-community#2254 (@cyrossignol)
 - qt: Prevent pasting (no label) as label in consolidation transaction gridcoin-community#2255 (@jamescowens)
@ghost ghost deleted the patch-3 branch March 18, 2022 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Removed deprecated functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants