Merge settings one place instead of five places by ryanofsky · Pull Request #15934 · bitcoin/bitcoin

ryanofsky · 2019-05-01T21:48:50Z

This is a refactoring-only change that makes it easier to add a new settings source.

This PR doesn't change behavior. The util_ArgsMerge and util_ChainMerge tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.

This change:

Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from Add <datadir>/settings.json persistent settings storage #15935).
Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places (GetArg, GetNetBoolArg, GetArgs, IsArgNegated, GetUnsuitableSectionOnlyArgs) in inconsistent ways.
Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line ignored arguments and more ignored arguments, and config file reverse precedence, inconsistently applied top-level settings, and zombie values.

The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent -wallet setting are pretty straightforward and show how the new code can be extended:

70675c3 from Add <datadir>/settings.json persistent settings storage #15935 – Add <datadir>/settings.json persistent settings storage
04c80c4 from Add loadwallet and createwallet load_on_startup options #15937 – Add loadwallet and createwallet RPC load_on_startup options

DrahtBot · 2019-05-02T00:43:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17385 (WIP: refactor: Use our own integer parsing/formatting everywhere by laanwj)
#15454 (Remove the automatic creation and loading of the default wallet by achow101)
#14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
#11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)

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.

promag

I had a refactor (which I did't submit) that supported chaining ArgsManager. The idea was to support changing some args when calling some RPC, so a ArgsManager is created with the "overridden" args and passed thru. Is this something you are considering supporting or do you see a different approach?

Concept ACK.

src/util/system.cpp

ryanofsky · 2019-05-08T20:45:53Z

re: #15934 (review) from promag

I had a refactor (which I did't submit) that supported chaining ArgsManager. The idea was to support changing some args when calling some RPC, so a ArgsManager is created with the "overridden" args and passed thru. Is this something you are considering supporting or do you see a different approach?

This change does make it easier to add new settings sources (with consistent handling of negated args and things), so it should be compatible with your idea and maybe helpful.

Depending on the situation, I think having chained or scoped settings could be a good idea or not. I do think that in wallet code and application code generally it's good to get away from using key-value storage classes like ArgsManager or UniValue as quickly as possible, and switch to more direct representations like CCoinControl that are type safe and can be accessed more simply.

fanquake · 2019-05-24T13:58:49Z

Concept ACK

jamesob

Tested ACK 1d543ad

Generated and reviewed the test output locally. Mucked around with various argument formulations using the following config file:

dbcache=100
[main]
dbcache=200
[test]
dbcache=300

and commandline invocations e.g.

./src/bitcoind -conf=$(pwd)/test.conf -dbcache=1000 -dbcache=500 | grep Using

to verify dbcache being set as expected.

This is a well-written change that cleans up a lot of gnarly, duplicated settings munging. It explicitly outlines surprising corner cases in existing behavior (with docs too), and makes reasoning about settings merge order easier. This change also introduces substantial test coverage to settings management (util::Settings).

After this is merged, adding a read-write settings file (whether it's JSON or something else) will be much easier.

src/util/settings.cpp

src/util/system.cpp

jamesob · 2019-05-28T15:44:35Z

src/util/system.cpp

+        // Weird behavior preserved for backwards compatibility: command line
+        // options with section prefixes are allowed but ignored. It would be
+        // better if these options triggered the IsArgKnown error below, or were
+        // actually used instead of silently ignored.


Thanks for the nice comment. Potentially out of scope: could we log warnings for this instead of silently ignoring?

re: #15934 (comment)

Potentially out of scope: could we log warnings for this instead of silently ignoring?

I'm planning on making followup PRs that simplify and clean up all these "Weird behavior preserved for backwards compatibility" instances. I'd rather not add warnings in this PR partly because I'm disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.

jamesob · 2019-05-28T18:32:51Z

src/test/settings_tests.cpp

+// test parses and merges settings, representing the results as strings that get
+// compared against an expected hash. To debug, the result strings can be dumped
+// to a file (see comments below).
+BOOST_FIXTURE_TEST_CASE(Merge, MergeTestingSetup)


Cool test! The formatted output is really helpful. Encourage other reviewers to run and inspect with

SETTINGS_MERGE_TEST_OUT=results.txt ./src/test/test_bitcoin --run_test=settings_tests/Merge

src/test/util.h

src/test/settings_tests.cpp

ryanofsky

Updated 1d543ad -> 2dfeff1 (pr/mergeset.6 -> pr/mergeset.7), compare) with suggested changes.
Rebased 2dfeff1 -> 955c782 (pr/mergeset.7 -> pr/mergeset.8) to share common code with #15988.

src/test/settings_tests.cpp

src/util/settings.cpp

src/util/system.cpp

ryanofsky · 2019-05-29T15:49:15Z

src/util/system.cpp

+        // Weird behavior preserved for backwards compatibility: command line
+        // options with section prefixes are allowed but ignored. It would be
+        // better if these options triggered the IsArgKnown error below, or were
+        // actually used instead of silently ignored.


re: #15934 (comment)

Potentially out of scope: could we log warnings for this instead of silently ignoring?

I'm planning on making followup PRs that simplify and clean up all these "Weird behavior preserved for backwards compatibility" instances. I'd rather not add warnings in this PR partly because I'm disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.

src/test/util.h

jamesob · 2019-05-29T18:25:42Z

re-tACK 955c782 based on the interdiff and running an abbreviated version of the testing above.

ryanofsky · 2019-06-27T20:49:15Z

Rebased 955c782 -> 14a6dfc (pr/mergeset.8 -> pr/mergeset.9) due to conflict with #16278

jamesob · 2019-06-27T21:29:17Z

reACK 14a6dfc based on interdiff. Only change since pr/mergeset.8 is a trivial LogPrintf fix.

ariard · 2019-11-08T17:51:01Z

ACK 083c954

I've reviewed that's new code is doing what it says to do. I think that weird behaviors are compatibility-maintained but at least if we have a regression they are well-documented and so we'll know where to look, plus as said by James this part of code is low-risk so it's worth moving forward.

Great work Russ, and I'm concept ACK future PRs to remove/squeeze weird behaviors!

jnewbery

ACK 083c954

jnewbery · 2019-11-08T16:56:26Z

src/util/system.cpp

-                error += "-includeconf cannot be used from commandline; -includeconf=" + ic + "\n";
-            }
-            return false;
+    bool success = true;


nit (please don't change in this PR. Leave for a follow-up, if at all): 'we clear it' in the comment above is inaccurate. If an -includeconf is found on the command line then we return false rather than silently removing the bad config. Change that comment to // Do not allow -includeconf from the command line (except for -noincludeconf)

In fact, I don't think we need the local success variable at all. You could just return false from the inner for loop. I don't think the user needs to be told about the multiple -includeconf command line arguments they've used. Just alerting about the first should be enough.

ryanofsky · 2019-11-08T18:54:34Z

Thanks! Will leave PR at 083c954. Some notes for future followup:

Merge settings one place instead of five places #15934 (comment) - s/conf file/included conf file/
Merge settings one place instead of five places #15934 (comment) - s/, so we clear it here// and early return

jnewbery · 2019-11-08T19:59:24Z

Other potential future follow-ups:

Don't throw in GetChainName()

(

bitcoin/src/util/system.cpp

Line 970 in 8021392

    
           throw std::runtime_error("Invalid combination of -regtest, -testnet and -chain. Can use at most one.");

). I think the calling code was originally supposed to catch this (eg

bitcoin/src/bitcoind.cpp

Line 88 in 8021392

SelectParams(gArgs.GetChainName());

), but this is no longer the case because GetChainName() is called in ReadConfigFiles(), and so starting with -regtest -testnet results in an unhandled exception.

Remove ArgsManagerHelper()

Here in master:

bitcoin/src/util/system.cpp

Line 165 in 8021392

class ArgsManagerHelper {

. After this PR there are only two functions in this class. Just move them into ArgsManager(). The class was only added to speed up rebuilding (#11862 (comment)), which doesn't seem like a good reason to keep it.

Move the ArgsManager code into util/settings ?

jamesob

ACK 083c954

Reviewed diff-of-diffs both on Github and locally. Only changes since my last ACK are those discussed above. Nice job persevering, @ryanofsky, and thanks to @jnewbery @ariard for good feedback.

jamesob · 2019-11-08T21:07:01Z

src/util/settings.cpp

+        // precedence over early settings, but for backwards compatibility in
+        // the config file the precedence is reversed for all settings except
+        // chain name settings.
+        const bool reverse_precedence = (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && !get_chain_name;


These long lines are killin' me but I'll bite my tongue in the interest of a merge.

Will fix with the other followups, but if it in case its useful https://greasyfork.org/en/scripts/18789-github-toggle-code-wrap seems to work well. (Found through https://github.com/StylishThemes/GitHub-code-wrap)

083c954 Add settings_tests (Russell Yanofsky) 7f40528 Deduplicate settings merge code (Russell Yanofsky) 9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky) e2e37cf Remove includeconf nested scope (Russell Yanofsky) 5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky) dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky) Pull request description: This is a refactoring-only change that makes it easier to add a new settings source. This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change. This change: - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from #15935). - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways. - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108). The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended: * 70675c3 from #15935 – _Add \<datadir>/settings.json persistent settings storage_ * 04c80c4 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_ ACKs for top commit: ariard: ACK 083c954 jnewbery: ACK 083c954 jamesob: ACK 083c954 Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659

Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

includeconf -> conf_file_names to_include -> conf_file_name include_config -> conf_file_stream Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

Easier to review ignoring whitespace Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

083c954 Add settings_tests (Russell Yanofsky) 7f40528 Deduplicate settings merge code (Russell Yanofsky) 9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky) e2e37cf Remove includeconf nested scope (Russell Yanofsky) 5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky) dc8e1e7 Clarify emptyIncludeConf logic (Russell Yanofsky) Pull request description: This is a refactoring-only change that makes it easier to add a new settings source. This PR doesn't change behavior. The [`util_ArgsMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in bitcoin#15869 and bitcoin#15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change. This change: - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3 from bitcoin#15935). - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L343-L352)) in inconsistent ways. - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L101-L108). The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended: * 70675c3 from bitcoin#15935 – _Add \<datadir>/settings.json persistent settings storage_ * 04c80c4 from bitcoin#15937 – _Add loadwallet and createwallet RPC load_on_startup options_ ACKs for top commit: ariard: ACK 083c954 jnewbery: ACK 083c954 jamesob: ACK 083c954 Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659

Suggested by James O'Beirne <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

Suggested by Antoine Riard <[email protected]> bitcoin#15934 (comment) and John Newbery <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

Rename suggested by João Barbosa <[email protected]> bitcoin#16545 (comment) This also gets rid of ArgsManager::NONE constant, which was an implementation detail not meant to be used by ArgsManager callers. Finally this reverts a change from 7f40528 bitcoin#15934 adding "-" characters to argument names. Better for GetArgFlags to require "-" prefixes for consistency with other argument methods, and to be more efficient later when GetArg functions need to call GetArgFlags. This commit does not change behavior.

Suggested by John Newbery <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

Rename suggested by João Barbosa <[email protected]> bitcoin#16545 (comment) This also gets rid of ArgsManager::NONE constant, which was an implementation detail not meant to be used by ArgsManager callers. Finally this reverts a change from 7f40528 bitcoin#15934 adding "-" characters to argument names. Better for GetArgFlags to require "-" prefixes for consistency with other ArgsManager methods, and to be more efficient later when GetArg functions need to call GetArgFlags (bitcoin#16545) This commit does not change behavior.

Suggested by John Newbery <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

ryanofsky · 2019-11-18T16:40:57Z

I've been working on some followups for this PR since it's been merged. #17473 has various refactoring cleanups suggested above, and #17508 is tracking cleanup of confusing settings behaviors.

Going through all the comments in this issue, I think the only suggestions left not addressed by #17473 and #17508 are:

Fixing return InitError in a loop: Merge settings one place instead of five places #15934 (comment)
Simplifying -includeconf errors: Merge settings one place instead of five places #15934 (comment)
Fixing uncaught exceptions thrown by GetChainName: Merge settings one place instead of five places #15934 (comment)
Moving ArgsManager settings code from system.cpp to settings.cpp: Merge settings one place instead of five places #15934 (comment)

I don't have plans to work on these last few things

ariard · 2019-11-18T17:37:09Z

Maybe tag them good_first_issue or up_to_grabs to let people find them

Suggested by James O'Beirne <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

maflcko · 2021-01-30T09:20:33Z

Hidden by GitHub, but the "Up for grabs" refers to #15934 (comment)

This was referenced May 1, 2019

Add <datadir>/settings.json persistent settings storage #15935

Merged

interfaces: Expose settings.json methods to GUI #15936

Merged

Add loadwallet and createwallet load_on_startup options #15937

Merged

DrahtBot added Build system Tests Utils/log/libs labels May 1, 2019

promag reviewed May 2, 2019

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

src/util/system.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/mergeset branch 5 times, most recently from 19e84b7 to 1d543ad Compare May 8, 2019 15:59

ryanofsky changed the title ~~WIP: Dedup settings merge code~~ Dedup settings merge code May 14, 2019

ryanofsky marked this pull request as ready for review May 14, 2019 14:53

ryanofsky mentioned this pull request May 28, 2019

Docs: Add note on precedence of options in bitcoin.conf #16076

Closed

jamesob approved these changes May 28, 2019

View reviewed changes

ryanofsky force-pushed the pr/mergeset branch from 1d543ad to 955c782 Compare May 29, 2019 17:35

ryanofsky changed the title ~~Dedup settings merge code~~ Separate settings merging from parsing May 29, 2019

ryanofsky commented May 29, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jun 27, 2019

ryanofsky force-pushed the pr/mergeset branch from 955c782 to 14a6dfc Compare June 27, 2019 18:02

DrahtBot removed the Needs rebase label Jun 27, 2019

DrahtBot added the Needs rebase label Jun 28, 2019

ryanofsky force-pushed the pr/mergeset branch from 14a6dfc to d074e43 Compare June 28, 2019 14:08

jnewbery reviewed Nov 8, 2019

View reviewed changes

jamesob approved these changes Nov 8, 2019

View reviewed changes

laanwj merged commit 083c954 into bitcoin:master Nov 8, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 9, 2019

Clarify emptyIncludeConf logic

dfc9148

Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 9, 2019

Remove includeconf nested scope

0bc925d

Easier to review ignoring whitespace Suggestion from John Newbery <[email protected]> in bitcoin#15934 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019

refactor: Clean up long lines in settings code

fb4e025

Suggested by James O'Beirne <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019

refactor: Get rid of ArgsManagerHelper class

6f018d3

Suggested by John Newbery <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

ryanofsky mentioned this pull request Nov 14, 2019

refactor: Settings code cleanups #17473

Merged

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 14, 2019

refactor: Get rid of ArgsManagerHelper class

db458b3

Suggested by John Newbery <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

Sjors mentioned this pull request Nov 15, 2019

[wip] util: Improve evaluation of includeconf lines in network sections of the config file #14866

Closed

ryanofsky mentioned this pull request Nov 18, 2019

Simpler settings interpretation #17508

Closed

ryanofsky added the Up for grabs label Nov 18, 2019

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 18, 2019

refactor: Clean up long lines in settings code

3f7dc9b

Suggested by James O'Beirne <[email protected]> bitcoin#15934 (comment) This commit does not change behavior.

ryanofsky mentioned this pull request Jan 9, 2020

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

fanquake mentioned this pull request Jan 31, 2021

refactor: don't throw in GetChainName() #21039

Closed

Sjors mentioned this pull request Sep 29, 2021

Deprecate main and test -chain params #23090

Closed

kwvg mentioned this pull request Jun 2, 2022

backport: bitcoin#15934, #15864, #19188, #18338, #19413, #18571, #18575 (deglobalization part 3) dashpay/dash#4844

Merged

Conversation

ryanofsky commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented May 8, 2019

Uh oh!

fanquake commented May 24, 2019

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamesob May 28, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 29, 2019

Choose a reason for hiding this comment

Uh oh!

jamesob May 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky May 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented May 29, 2019

Uh oh!

ryanofsky commented Jun 27, 2019

Uh oh!

jamesob commented Jun 27, 2019

Uh oh!

ariard commented Nov 8, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 8, 2019

Uh oh!

jnewbery commented Nov 8, 2019

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 18, 2019

Uh oh!

ariard commented Nov 18, 2019

Uh oh!

maflcko commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

ryanofsky commented May 1, 2019 •

edited

Loading

DrahtBot commented May 2, 2019 •

edited

Loading