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

util: Port of ArgsManager and a significant subset of src/util #2146

Merged

Conversation

jamescowens
Copy link
Member

@jamescowens jamescowens commented May 24, 2021

This PR represents a complete port of the current Bitcoin ArgsManager class, along with all supporting functions. The port necessitated porting a large subset of the src/util source files of Bitcoin, and factoring out the corresponding functions from the Gridcoin util.h/cpp. There are still some things left in util.h/cpp, but that should be done in another PR. This one is quite big enough as it is.

Care was taken to ensure that minimal changes were made to the ported functions, but Gridcoin's difference in functionality was respected. Most of that is limited to significantly different AddArg's in init.cpp, modifications to the init code to deal with Gridcoin's relatively primitive initialization sequence, and modifications to the boost unit testing harness to deal with the differences.

I created two new OptionsCategories, SCRAPER and RESEARCHER, to group Gridcoin specific arguments that are not in Bitcoin.

Note that for GetConfigFile(), a net specific call is made, unlike Bitcoin, since we still use a separate config file for mainnet and testnet.

@jamescowens jamescowens self-assigned this May 24, 2021
@jamescowens jamescowens force-pushed the port_system_2 branch 9 times, most recently from 4976c9b to 6c06a2a Compare May 30, 2021 14:45
@jamescowens
Copy link
Member Author

Nearly done. Looks like the ArgsManager doesn't support writing to directly to the configuration file, but instead to a settings file. Not sure I really like that... I have to check on the beacon wizard and the write into the conf file for the wallet mode.

@jamescowens jamescowens force-pushed the port_system_2 branch 2 times, most recently from f1fc84f to b98d7cf Compare May 30, 2021 20:18
@cyrossignol
Copy link
Member

Some non-end-user/service provider node deployments may not have a writable config file (think /etc/gridcoinresearch.conf as conventional for many Linux services or for platforms with automated configuration management). The way I did it for the beacon wizard was really a stop-gap until we had a more flexible framework in place.

@jamescowens
Copy link
Member Author

This should fill the bill then. I have to implement the settings file init. I haven’t wired that in yet. I am also chasing down a regression with the use of gridcoinresearchd for rpc relay and also the rpcport settings not taking.

@jamescowens jamescowens force-pushed the port_system_2 branch 2 times, most recently from ac2738b to abd2812 Compare May 31, 2021 04:45
@jamescowens
Copy link
Member Author

Regression and RPC port issue fixed.

@jamescowens
Copy link
Member Author

The only thing I haven't done is hook in the settings file stuff.

@jamescowens jamescowens marked this pull request as ready for review May 31, 2021 15:02
@jamescowens
Copy link
Member Author

@cyrossignol I hooked in the InitSetting calls. Do you want me to try and convert over the beacon wizard file write to config to setting, or do you want to do it?

@jamescowens jamescowens added enhancement refactor This is for refactoring (if also an enhancement, use that label too). labels May 31, 2021
@jamescowens jamescowens added this to the Ingrid milestone May 31, 2021
@jamescowens
Copy link
Member Author

I changed RewriteConfigurationFileMode to UpdateRWSettingsForMode and hooked it into the RW setting framework. Not sure if that was how you wanted to do it, but it works.

@jamescowens
Copy link
Member Author

jamescowens commented May 31, 2021

Hmm. This has to deal correctly with overridden original entries in the config file. It is not handling the situation where an email="address" is set in the config file, and then you flip to investor mode, and then back to researcher mode. The wizard gets confused on the projects table, probably because of the investor=1 in the rw settings file and the now non-override email from the ro config. I am going to have to look at this closer.

@jamescowens
Copy link
Member Author

Ok. I figured it out.

@jamescowens
Copy link
Member Author

This is ready for an initial look.

@jamescowens
Copy link
Member Author

I went ahead and implemented bool updateRwSettings(const std::vector<std::pair<std::string, util::SettingsValue>>& settings_in) and condensed the rw setting update in researcher.cpp.

Copy link
Member

@cyrossignol cyrossignol left a comment

Choose a reason for hiding this comment

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

Pausing here—getting pulled away. Looks great so far. Grueling to review, so I can only imagine what a pain it was to work on... 🙂

src/gridcoin/researcher.cpp Outdated Show resolved Hide resolved
src/gridcoinresearchd.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Show resolved Hide resolved
src/gridcoinresearchd.cpp Outdated Show resolved Hide resolved
src/gridcoinresearchd.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
src/qt/guiutil.cpp Outdated Show resolved Hide resolved
src/test/util_tests.cpp Show resolved Hide resolved
@cyrossignol
Copy link
Member

Feel free to squash my suggested edits. I don't care about attribution... clean history is more important than some trivial credits 🙂

Copy link
Member

@cyrossignol cyrossignol left a comment

Choose a reason for hiding this comment

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

Looks good.

@jamescowens jamescowens merged commit 2a8de3b into gridcoin-community:development Jun 8, 2021
@jamescowens jamescowens deleted the port_system_2 branch June 13, 2021 00:33
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement refactor This is for refactoring (if also an enhancement, use that label too).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants