eefe56967b bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b0 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky)
Pull request description:
Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen:
- One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning.
- Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored.
This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored.
ACKs for top commit:
pinheadmz:
re-ACK eefe56967b
willcl-ark:
re-ACK eefe56967b
TheCharlatan:
ACK eefe56967b
Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b
33e2b82a4f wallet, bench: Remove unused database options from WalletBenchLoading (Andrew Chow)
80ace042d8 tests: Modify records directly in wallet ckey loading test (Andrew Chow)
b3bb17d5d0 tests: Update DuplicateMockDatabase for MockableDatabase (Andrew Chow)
f0eecf5e40 scripted-diff: Replace CreateMockWalletDB with CreateMockableWalletDB (Andrew Chow)
075962bc25 wallet, tests: Include wallet/test/util.h (Andrew Chow)
14aa4cb1e4 wallet: Move DummyDatabase to salvage (Andrew Chow)
f67a385556 wallet, tests: Replace usage of dummy db with mockable db (Andrew Chow)
33c6245ac1 Introduce MockableDatabase for wallet unit tests (Andrew Chow)
Pull request description:
For the wallet's unit tests, we currently use either `DummyDatabase` or memory-only versions of either BDB or SQLite. The tests that use `DummyDatabase` just need a `WalletDatabase` so that the `CWallet` can be constructed, while the tests using the memory-only databases just need a backing data store. There is also a `FailDatabase` that is similar to `DummyDatabase` except it fails be default or can have a configured return value. Having all of these different database types can make it difficult to write tests, particularly tests that work when either BDB or SQLite is disabled.
This PR unifies all of these different unit test database classes into a single `MockableDatabase`. Like `DummyDatabase`, most functions do nothing and just return true. Like `FailDatabase`, the return value of some functions can be configured on the fly to test various failure cases. Like the memory-only databases, records can actually be written to the `MockableDatabase` and be retrieved later, but all of this is still held in memory. Using `MockableDatabase` completely removes the need for having BDB or SQLite backed wallets in the unit tests for the tests that are not actually testing specific database behaviors.
Because `MockableDatabase`s can be created by each unit test, we can also control what records are stored in the database. Records can be added and removed externally from the typical database modification functions. This will give us greater ability to test failure conditions, particularly those involving corrupted records.
Possible alternative to #26644
ACKs for top commit:
furszy:
ACK 33e2b82
TheCharlatan:
ACK 33e2b82a4f
Tree-SHA512: c2b09eff9728d063d2d4aea28a0f0e64e40b76483e75dc53f08667df23bd25834d52656cd4eafb02e552db0b9e619cfdb1b1c65b26b5436ee2c971d804768bcc
This is a follow-up to previous commits moving the chain constants out
of chainparamsbase.
The script removes the chainparamsbase header in all files where it is
included, but not used. This is done by filtering against all defined
symbols of the header as well as its respective .cpp file.
The kernel chainparams now no longer relies on chainparamsbase.
-BEGIN VERIFY SCRIPT-
sed -i '/#include <chainparamsbase.h>/d' $( git grep -l 'chainparamsbase.h' | xargs grep -L 'CBaseChainParams\|CreateBaseChainParams\|SetupChainParamsBaseOptions\|BaseParams\|SelectBaseParams\|chainparamsbase.cpp' )
-END VERIFY SCRIPT-
This commit effectively moves the definition of these constants
out of the chainparamsbase to their own file.
Using the ChainType enums provides better type safety compared to
passing around strings.
The commit is part of an ongoing effort to decouple the libbitcoinkernel
library from the ArgsManager and other functionality that should not be
part of the kernel library.
fa83fb3161 wallet: Use steady clock to calculate number of derive iterations (MarcoFalke)
fa2c099cec wallet: Use steady clock to measure scanning duration (MarcoFalke)
fa97621804 qt: Use steady clock to throttle GUI notifications (MarcoFalke)
fa1d8044ab test: Use steady clock in index tests (MarcoFalke)
fa454dcb20 net: Use steady clock in InterruptibleRecv (MarcoFalke)
Pull request description:
`GetTimeMillis` has multiple issues:
* It doesn't denote the underlying clock type
* It isn't type-safe
* It is used incorrectly in places that should use a steady clock
Fix all issues here.
ACKs for top commit:
willcl-ark:
ACK fa83fb3161
martinus:
Code review ACK fa83fb3161, also ran all tests. All usages of the steady_clock are just for duration measurements, so the change to a different epoch is ok.
Tree-SHA512: 5ec4fede8c7f97e2e08863c011856e8304f16ba30a68fdeb42f96a50a04961092cbe46ccf9ea6ac99ff5203c09f9e0924eb483eb38d7df0759addc85116c8a9f
Since we have a mockable wallet database, we don't really need to be
using BDB or SQLite's in-memory database capabilities. It doesn't really
help us to be using those as we aren't doing anything that requires one
type of db over the other, and will just prefer SQLite if it's
available.
MockableDatabase is suitable for these uses, so use
CreateMockableWalletDatabase to use that.
-BEGIN VERIFY SCRIPT-
sed -i "s/CreateMockWalletDatabase(options)/CreateMockableWalletDatabase()/" $(git grep -l "CreateMockWalletDatabase(options)" -- ":(exclude)src/wallet/walletdb.*")
sed -i "s/CreateMockWalletDatabase/CreateMockableWalletDatabase/" $(git grep -l "CreateMockWalletDatabase" -- ":(exclude)src/wallet/walletdb.*")
-END VERIFY SCRIPT-
Currently debug.log will show the wrong bitcoin.conf config file path when
bitcoind is invoked without -conf or -datadir arguments, and there's a default
bitcoin.conf file which specifies another datadir= location. When this happens,
the debug.log will include an incorrect "Config file:" line referring to a
bitcoin.conf file in the other datadir, instead of the referring to the actual
configuration file in the default datadir which was parsed.
The bad log print was reported and originally fixed in
https://github.com/bitcoin/bitcoin/pull/27303 by
Matthew Zipkin <pinheadmz@gmail.com>
This PR takes a slightly different approach to fixing the bug, trying to avoid
future bugs by not allowing the GetConfigFilePath function to be called before
the the configuration is parsed, and deleting GetConfigFile function which
could be confused with GetConfigFilePath. It also includes a test for the bug
which the original fix did not have.
Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
be55f545d5 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
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](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d5🚲
ryanofsky:
Code review ACK be55f545d5. Just small cleanups since the last review.
hebasto:
ACK be55f545d5, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
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.
Instead of storing and passing around fixed strings for the purpose of
an address, use an enum.
This also rationalizes the CAddressBookData struct, documenting all fields and
making them public, and simplifying the representation to avoid bugs like
https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114 and make
it not possible to invalid address data like change addresses with labels.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
68eed5df86 test,gui: add coverage for PSBT creation on legacy watch-only wallets (furszy)
306aab5bb4 test,gui: decouple widgets and model into a MiniGui struct (furszy)
2f76ac0383 test,gui: decouple chain and wallet initialization from test case (furszy)
cd98b71739 gui: 'getAvailableBalance', include watch only balance (furszy)
74eac3a82f test: add coverage for 'useAvailableBalance' functionality (furszy)
dc1cc1c359 gui: bugfix, getAvailableBalance skips selected coins (furszy)
Pull request description:
Fixes https://github.com/bitcoin-core/gui/issues/688 and https://github.com/bitcoin/bitcoin/issues/26687.
First Issue Description (https://github.com/bitcoin-core/gui/issues/688):
The previous behavior for `getAvailableBalance`, when the coin control had selected coins, was to return the sum of them. Instead, we are currently returning the wallet's available total balance minus the selected coins total amount.
Reason:
Missed to update the `GetAvailableBalance` function to include the coin control selected coins on #25685.
Context:
Since #25685 we skip the selected coins inside `AvailableCoins`, the reason is that there is no need to waste resources walking through the entire wallet's txes map just to get coins that could have gotten by just doing a simple `mapWallet.find`).
Places Where This Generates Issues (only when the user manually select coins via coin control):
1) The GUI balance check prior the transaction creation process.
2) The GUI "useAvailableBalance" functionality.
Note 1:
As the GUI uses a balance cache since https://github.com/bitcoin-core/gui/pull/598, this issue does not affect the regular spending process. Only arises when the user manually select coins.
Note 2:
Added test coverage for the `useAvailableBalance` functionality.
----------------------------------
Second Issue Description (https://github.com/bitcoin/bitcoin/issues/26687):
As we are using a cached balance on `WalletModel::getAvailableBalance`,
the function needs to include the watch-only available balance for wallets
with private keys disabled.
ACKs for top commit:
Sjors:
tACK 68eed5df86
achow101:
ACK 68eed5df86
theStack:
ACK 68eed5df86
Tree-SHA512: 674f3e050024dabda2ff4a04b9ed3750cf54a040527204c920e1e38bd3d7f5fd4d096e4fd08a0fea84ee6abb5070f022b5c0d450c58fd30202ef05ebfd7af6d3
The following cases were covered:
Case 1: No coin control selected coins.
- 'useAvailableBalance' should fill the amount edit box with the total available balance.
Case 2: With coin control selected coins.
- 'useAvailableBalance' should fill the amount edit box with the sum of the selected coins values.
The previous behavior for getAvailableBalance when coin control
has selected coins was to return the sum of them. Instead, we
are currently returning the wallet's available total balance minus
the selected coins total amount.
This turns into a GUI-only issue for the "use available balance"
button when the user manually select coins in the send screen.
Reason:
We missed to update the GetAvailableBalance function to include
the coin control selected coins on #25685.
Context:
Since #25685 we skip the selected coins inside `AvailableCoins`,
the reason is that there is no need to traverse the wallet's
txes map just to get coins that can directly be fetched by
their id.
00e9b97f37 refactor: Move fs.* to util/fs.* (TheCharlatan)
106b46d9d2 Add missing fs.h includes (TheCharlatan)
b202b3dd63 Add missing cstddef include in assumptions.h (TheCharlatan)
18fb36367a refactor: Extract util/fs_helpers from util/system (Ben Woosley)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by empact and is taken from its parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (https://github.com/bitcoin/bitcoin/pull/25290, https://github.com/bitcoin/bitcoin/pull/25487, https://github.com/bitcoin/bitcoin/pull/25527, https://github.com/bitcoin/bitcoin/pull/25862, https://github.com/bitcoin/bitcoin/pull/26177, and https://github.com/bitcoin/bitcoin/pull/27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in https://github.com/bitcoin/bitcoin/pull/27238.
#### Changes
Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files.
There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory.
Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed.
ACKs for top commit:
hebasto:
ACK 00e9b97f37
Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666
9a1d73fdff Fix segfault when shutdown during wallet open (John Moffett)
Pull request description:
Fixes #689
## Summary
If you open a wallet and send a shutdown signal during that process, you'll get a segfault when the wallet finishes opening. That's because the `WalletController` object gets deleted manually in bitcoin.cpp during shutdown, but copies of the pointer (and pointers to child objects) are dangling in various places and are accessed in queued events after the deletion.
## Details
The issue in #689 is caused by the following sequence of events:
1. Wallet open modal dialog is shown and worker thread does the actual work.
2. Every 200ms, the main event loop checks to see if a shutdown has been requested, but only if a modal is not being shown.
3. Request a shutdown while the modal window is shown.
4. The wallet open process completes, the modal window is dismissed, and various `finish` signals are sent.
5. During handling of one of the `finish` signals, `qApp->processEvents()` is [called](e9262ea32a/src/qt/sendcoinsdialog.cpp (L603)), which causes the main event loop to detect the shutdown (now that the modal window has been dismissed). The `WalletController` and all the `WalletModel`s are [deleted](65de8eeeca/src/qt/bitcoin.cpp (L394-L401)).
6. Control returns to the `finish` method, which eventually tries to send a [signal](e9262ea32a/src/qt/sendcoinsdialog.cpp (L167)) from a wallet model, but it's been deleted already (and the signal is sent from a now-[dangling](d8bdee0fc8/src/qt/walletview.cpp (L65)) pointer).
The simplest fix for that is to change the `qApp->processEvents()` into a `QueuedConnection` call. (The `qApp->processEvents() was a [workaround](https://github.com/bitcoin/bitcoin/pull/593#issuecomment-3050699) to get the GUI to scroll to the last item in a list that just got added, and this is just a safer way of doing that).
However, once that segfault is fixed, another segfault occurs due to some queued wallet events happening after the wallet controller object is deleted here:
65de8eeeca/src/qt/bitcoin.cpp (L394-L401)
Since `m_wallet_controller` is a copy of that pointer in `bitcoingui.cpp`, it's now dangling and `if(null)` checks won't work correctly. For instance, this line:
65de8eeeca/src/qt/bitcoingui.cpp (L413)
sets up a `QueuedConnection` to `setCurrentWallet`, but by the time control reaches that method (one event cycle after shutdown deleted `m_wallet_controller` in `bitcoin.cpp`), the underlying objects have been destroyed (but the pointers are still dangling).
Ideally, we'd use a `QPointer` or `std::shared_ptr / std::weak_ptr`s for these, but the changes would be more involved.
This is a minimal fix for the issues. Just set `m_wallet_controller` to `nullptr` in `bitcoingui.cpp`, check its value in a couple places, and avoid a use of `qApp->processEvents`.
ACKs for top commit:
hebasto:
ACK 9a1d73fdff, I have reviewed the code and it looks OK.
furszy:
ACK 9a1d73fdff
Tree-SHA512: a1b94676eb2fcb7606e68fab443b1565b4122aab93c35382b561842a049f4b43fecc459535370d67a64d6ebc4bcec0ebcda981fff633ebd41bdba6f7093ea540
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.
This is an extraction of filesystem related functions from util/system
into their own utility file.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving these functions out of system.h allows including them from a
separate source file without including the ArgsManager definitions from
system.h.
This is a minimal extraction of a single function, but also the only use
of std::exception in util/system.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving the function out of system.h allows including it from a separate
source file without including the ArgsManager definitions from system.h.
4be57a5df1 gui: fix comments for BanTableModel and BanTablePriv::refreshBanlist() (Vasil Dimov)
a981af4e6f gui: use the stored CSubNet entry when unbanning (Vasil Dimov)
Pull request description:
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
ACKs for top commit:
furszy:
utACK 4be57a5d
mzumsande:
Tested ACK 4be57a5df1
Tree-SHA512: b783c18c9d676aa9486cff2d27039dd5c5ef3f1cc67e5056a2be68e35930926f368f26dacdf4f3d394a1f73e3e28f42dc8a6936cd1765c6e6e60695c7b4d78af
802cc1ef53 Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c671 Add InitError(error, details) overload (Ryan Ofsky)
3db2874bd7 Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df90b9 scripted-diff: Remove double newlines after some init errors (Ryan Ofsky)
Pull request description:
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.
Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the GUI error text has changed from "Error: Cannot parse configuration file:" to "Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the error text has changed from "Failed loading settings file" to "Settings file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the error text has changed from "Failed saving settings file" to "Settings file could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read), there is an normal error dialog showing "Error: filesystem error: status: Permission denied [.../settings.json]", instead of an uncaught exception.
ACKs for top commit:
Sjors:
Light review ACK 802cc1ef53
TheCharlatan:
ACK 802cc1ef53
achow101:
ACK 802cc1ef53
Tree-SHA512: 9c78d277e9ed595fa8ce286b97d2806e1ec06ddbbe7bd3434bd9dd7b456faf8d989f71231e97311f36edb9caaec645a50c730bd7514b8e0fe6e6f7741b13d981
The previous code visualized the `CSubNet` object as string, then
parsed that string back to `CSubNet`. This is sub-optimal given that
the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
