8466329127 chain: simplify `deleteRwSettings` code and improve it's doc (ismaelsadeeq)
f8d91f49c7 chain: dont check for null settings value in `overwriteRwSetting` (ismaelsadeeq)
df601993f2 chain: ensure `updateRwSetting` doesn't update to a null settings (ismaelsadeeq)
c8e2eeeffb chain: uniformly use `SettingsAction` enum in settings methods (ismaelsadeeq)
1e9e735670 chain: move new settings safely in `overwriteRwSetting` (ismaelsadeeq)
1c409004c8 test: remove wallet context from `write_wallet_settings_concurrently` (ismaelsadeeq)
Pull request description:
This PR addresses the remaining review comments from #30697
1. Disallowed overwriting settings values with a `null` value.
2. Uniformly used the `SettingsAction` enum in all settings methods instead of a boolean parameter.
3. Updated `overwriteRwSetting` to receive the `common::SettingsValue` parameter by value, enabling it to be moved safely.
4. Removed wallet context from the `write_wallet_settings_concurrently` unit test, as it is not needed.
ACKs for top commit:
achow101:
ACK 8466329127
ryanofsky:
Code review ACK 8466329127. Looks good, thanks for taking suggestions and applying them to the right commits. Only changes since last review were documentation improvements and simplifying delete method.
furszy:
Code review ACK 8466329127
Tree-SHA512: baf2f59ed5aac4a4bda0c84fb6554a466a40d1f7b52b61dc2ff293d83ae60e82b925b7003237b633fecb65eba3a4c108e69166046895d1295809fbe0de67b052
ee47ca29d6 init: fix fatal error on '-wallet' negated option value (furszy)
Pull request description:
Currently, if users provide a double negated value such as '-nowallet=0' or a non-boolean
convertible value to a negated option such as '-nowallet=not_a_boolean', the initialization
process results in a fatal error, causing an unclean shutdown and displaying a poorly
descriptive error message:
"JSON value of type bool is not of expected type string." (On bitcoind. The GUI
does not display any error msg - upcoming PR -).
This PR fixes the issue by ensuring that only string values are returned in the
the "wallet" settings list, failing otherwise. It also improves the clarity of the
returned error message.
Note:
This bug was introduced in https://github.com/bitcoin/bitcoin/pull/22217. Where the `GetArgs("-wallet")` call was
replaced by `GetSettingsList("-wallet")`.
ACKs for top commit:
achow101:
ACK ee47ca29d6
ryanofsky:
Code review ACK ee47ca29d6, just adding the suggested test since last review
TheCharlatan:
ACK ee47ca29d6
ismaelsadeeq:
Tested ACK ee47ca29d6
Tree-SHA512: 5f01076f74a048019bb70791160f0accc2db7a457d969cb23687bed81ccbbdec1dda68311e7c6e2dd56250e23e8d926d4066e5014b2a99a2fc202e24ed264fbd
Because we don't have type checking for command-line/settings/config
args, strings are interpreted as 'false' for non-boolean args.
By convention, this "forces" us to interpret negated strings as 'true',
which conflicts with the negated option definition in all the settings
classes (they expect negated options to always be false and ignore any
other value preceding them). Consequently, when retrieving all "wallet"
values from the command-line/settings/config, we also fetch the negated
string boolean value, which is not of the expected 'string' type.
This mismatch leads to an internal fatal error, resulting in an unclean
shutdown during initialization. Furthermore, this error displays a poorly
descriptive error message:
"JSON value of type bool is not of expected type string"
This commit fixes the fatal error by ensuring that only string values are
returned in the "wallet" settings list, failing otherwise. It also improves
the clarity of the returned error message.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
- Settings updates were not thread-safe, as they were executed in
three separate steps:
1) Obtain settings value while acquiring the settings lock.
2) Modify settings value.
3) Overwrite settings value while acquiring the settings lock.
This approach allowed concurrent threads to modify the same base value
simultaneously, leading to data loss. When this occurred, the final
settings state would only reflect the changes from the last thread
that completed the operation, overwriting updates from other threads.
Fix this by making the settings update operation atomic.
- Add test coverage for this behavior.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
Too early NotifyWalletLoaded() call in CWallet::Create() results the
notification goes before DescriptorScriptPubKeyMans were created and
added to an encrypted wallet.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
This should also fix an init error if a -walletdir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This change passes canonical
paths to fs calls validating the -walletdir path to fix this.
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
No longer create a default wallet. The default wallet will still be
loaded if it exists and not other wallets were specified (anywhere,
including settings.json, bitcoin.conf, and command line).
Tests are updated to be started with -wallet= if they need the default
wallet.
Added test to wallet_startup.py testing that no default wallet is
created and that it is loaded if it exists and no other wallets were
specified.
Checks are now consolidated in MakeBerkeleyDatabase function instead of
happening in higher level code.
This commit does not change behavior except for error messages which now
include more complete information.
This removes a source of complexity and indirection that makes it harder to
understand path checking code. Path checks will be simplified in upcoming
commits.
There is no change in behavior in this commit other than a slightly more
descriptive error message in `loadwallet` if the default "" wallet can't be
found. (The error message is improved more in upcoming commit "wallet: Remove
path checking code from loadwallet RPC".)
Enable the GUI to also use the load_on_startup feature.
Wallets loaded in the GUI always have load_on_startup=true.
When they are unloaded from the GUI, load_on_startup=false.
To facilitate this change, UpdateWalletSetting is moved into the wallet
module and called from within LoadWallet, RemoveWallet, and
Createwallet. This change does not actually touch the GUI code but
rather the wallet functions that are shared between the GUI and RPC.
This maintains a persistent list of wallets stored in settings that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in #15006, but it's reasonable to expose this feature by RPC as well.
