Behavior changes are:
- if a tx has a mempool conflict, the wallet will not attempt to
rebroadcast it
- if a txo is spent by a mempool-conflicted tx, that txo is no
longer considered spent
18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d refactor: pass CRecipient to FundTransaction (josibake)
5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 refactor: simplify `CreateRecipients` (josibake)
47353a608dc6e20e5fd2ca53850d6f9aa3240d4a refactor: remove out param from `ParseRecipients` (josibake)
f7384b921c3460c7a3cc7827a68b2c613bd98f8e refactor: move parsing to new function (josibake)
6f569ac903e5ddaac275996a5d0c31b2220b7b81 refactor: move normalization to new function (josibake)
435fe5cd96599c518e26efe444c9d94d1277996b test: add tests for fundrawtx and sendmany rpcs (josibake)
Pull request description:
## Motivation
The primary motivation for this PR is to enable `FundTransaction` to take a vector of `CRecipient` objects to allow passing BIP352 silent payment addresses to RPCs that use `FundTransaction` (e.g. `send`, `walletcreatefundedpsbt`). To do that, SFFO logic needs to be moved out of `FundTransaction` so the `CRecipient` objects with the correct SFFO information can be created and then passed to `FundTransaction`.
As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the `set<int>` method for communicating SFFO.
I'm also not convinced we need to pass a full `CMutableTx` object to `FundTransaction`, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments.
ACKs for top commit:
S3RK:
reACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
josibake:
> According to my `range-diff` nothing changed. reACK [18ad1b9](18ad1b9142)
achow101:
ACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7
e9014042a6bed8c16cc9a31fc35cb709d4b3c766 settings: add auto-generated warning msg for editing the file manually (furszy)
966f5de99a9f5da05c91378ad1e8ea8ed37ac3b3 init: improve corrupted/empty settings file error msg (furszy)
Pull request description:
Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144).
Improving a confusing situation reported by users who did not understand why a
settings parsing error occurred when the file was empty and did not know how to solve it.
Empty setting file could be due (1) corruption or (2) an user manually cleaning up the file content.
In both scenarios, the 'Unable to parse settings file' error does not help the user move forward.
ACKs for top commit:
achow101:
ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766
hebasto:
re-ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766.
ryanofsky:
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766. Just whitespace formatting changes and shortening a test string literal since last review
shaavan:
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766
Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec
d55fdb1a495190e213b1b5127f5d91e4a409765e Move TRACEx parameters to seperate lines (Richard Myers)
2d58629ee63eebc760e2a9226afcd0c46d3ec2bd wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers)
Pull request description:
This is a bugfix for from when [optional was introduced](758501b713) for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0.
I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change.
You can reproduce this bug using the coin-selection-simulation scripts as described in [issue #16](https://github.com/achow101/coin-selection-simulation/issues/16). You can also run the `interface_usdt_coinselection.py` test without the changes to `wallet/spend.cpp`.
ACKs for top commit:
0xB10C:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
achow101:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
murchandamus:
ACK d55fdb1a495190e213b1b5127f5d91e4a409765e
Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
Hopefully, refraining users from modifying the file unless they are
certain about the potential consequences.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
The preceding "Unable to parse settings file" message lacked
the necessary detail and guidance for users on what steps to
take next in order to resolve the startup error.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
If the serialized transaction passed to `fundrawtransaction` contains
duplicates, they will be deserialized and added to the transaction. Add
a test to ensure this behavior is not changed during the refactor.
A user can pass any number of duplicated and unrelated addresses as an
SFFO argument to `sendmany` and the RPC will not throw an error (note,
all the rest of the RPCs which take SFFO as an argument will error if
the user passes duplicates or specifies outputs not present in the
transaction). Add a test to ensure this behavior is not changed during
the refactor.
3ba815b42db74804e341ce15f648c2b297af55ca Make v2transport default for addnode RPC when enabled (Pieter Wuille)
Pull request description:
Since #29058, several types of manually configured connections will attempt v2 connections when `-v2transport` is enabled, except for the `addnode` RPC, as that one has an explicit argument to enable or disable.
Make the default for that RPC match the `-v2transport` setting so the behavior matches that of other manual connections from a user perspective.
ACKs for top commit:
achow101:
ACK 3ba815b42db74804e341ce15f648c2b297af55ca
kristapsk:
ACK 3ba815b42db74804e341ce15f648c2b297af55ca
theStack:
Code-review ACK 3ba815b42db74804e341ce15f648c2b297af55ca
Tree-SHA512: 31ef48cf1e533abb17866020378c004df929e626074dc98b3229fb60a66de58435e95c8fda8d1b463e1208aa39d1f42d239818e7e58595a3738089920598befc
df30247705940c50c5eaafd74e2abbeb8b0cec07 [test] import descriptor wallet with reorged parent + IsFromMe child in mempool (glozow)
c3d02be536ac3f35c10efa03653186a17ebbfc12 [test] rescan legacy wallet with reorged parent + IsFromMe child in mempool (Gloria Zhao)
Pull request description:
Originally motivated by #29019, which reverts back to having `requestMempoolTransactions` emit `transactionAddedToMempool` in `mapTx` default order instead of `GetSortedDepthAndScore` order.
It's important that these notifications happen in topological order, otherwise the wallet rescan may miss transactions that belong to it. Notably, checking whether a transaction `IsFromMe` requires knowing its inputs, which may be from a mempool parent.
When using `mapTx` order, a parent may come later than its child if it was added from a block disconnected in a reorg.
This PR adds a test for this case.
ACKs for top commit:
achow101:
ACK df30247705940c50c5eaafd74e2abbeb8b0cec07
furszy:
Code review ACK df30247705, nits can be disregarded.
Tree-SHA512: 2f1d9ef92313228adbbef94e634e5f7a9ec6e6a2c88e16aa343bdc95ffc9b9f9c82a569b412c9a3841db9d789e52f9283e8b9385731668d59355903e26e58a5d
Test that wallet rescans process transactions topologically, even if a
parent's entry into the mempool is later than that of its child.
This behavior is important because IsFromMe requires the ability to look
up a transaction's inputs.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
0eebd6fe7d01ddc7f6b7f13a6ed6e705c7aeae4e test: Assert that a new tx with a delta of 0 is never added (kevkevin)
cfdbcd19b32fd63954d7947dcc639aef291fb6b2 rpc: exposing modified_fee in getprioritisedtransactions (kevkevin)
252a86729a15e47ed168d8da7c4a8d6113673909 rpc: renaming txid -> transactionid (kevkevin)
2fca6c2dd03c3955d86efb0b8d2a7961e42115fd rpc: changed prioritisation-map -> "" (kevkevin)
3a118e19e100110300d3290d4c1434f963721d94 test: Directly constructing 2 entry map for getprioritisedtransactions (kevkevin)
Pull request description:
In this PR I am addressing some comments in https://github.com/bitcoin/bitcoin/pull/27501 as a followup.
- changed `prioritisation-map` in the `RPCResult` to `""`
- Directly constructing 2 entry map for getprioritisedtransactions in functional tests
- renamed `txid` to `transactionid` in `RPCResult` to be more consistent with naming elsewhere
- exposed the `modified_fee` field instead of having it be a useless arg
- Created a new test that asserts when `prioritisedtransaction` is called with a fee_delta of 0 it is not added to mempool
ACKs for top commit:
glozow:
reACK 0eebd6fe7d01ddc7f6b7f13a6ed6e705c7aeae4e, only change is the doc suggestion
Tree-SHA512: e99056e37a8b1cfc511d87c83edba7c928b50d2cd6c2fd7c038976779850677ad37fddeb2b983e8bc007ca8567eb21ebb78d7eae9b773657c2b297299993ec05
Test that wallet rescans process transactions topologically, even if a
parent's entry into the mempool is later than that of its child.
This behavior is important because IsFromMe requires the ability to look
up a transaction's inputs.
878d914777a03a04ecb84217152e8b7fd73a5062 doc: test: mention OS detection preferences in style guideline (Sebastian Falbesoner)
4c65ac96f8b021c107783adce3e8afe4f8edee6e test: detect OS consistently using `platform.system()` (Sebastian Falbesoner)
37324ae3dfb0e50daaf752dc863a880559fa4637 test: use `skip_if_platform_not_linux` helper where possible (Sebastian Falbesoner)
Pull request description:
There are at least three ways to detect the operating system in Python3:
- `os.name` (https://docs.python.org/3.9/library/os.html#os.name)
- `sys.platform` (https://docs.python.org/3.9/library/sys.html#sys.platform)
- `platform.system()` (https://docs.python.org/3.9/library/platform.html#platform.system)
We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using `platform.system()`, as it appears to be one most consistent and easy to read (see also [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-12-08#989301;) and table below). `sys.platform` is inconsistent as it has the major version number encoded for BSD systems, which doesn't make much sense for e.g. OpenBSD, where there is no concept of major versions, but instead the version is simply increased by 0.1 on each release.
Note that `os.name` is still useful to detect whether we are running a POSIX system (see `BitcoinTestFramework.skip_if_platform_not_posix`), so for this use-case it is kept as only exception. The following table shows values for common operating systems, found via
```
$ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())"
```
| OS | os.name | sys.platform | platform.system() |
|--------------|---------|--------------|--------------------|
| Linux 6.2.0 | posix | linux | Linux |
| MacOS* | posix | darwin | Darwin |
| OpenBSD 7.4 | posix | openbsd7 | OpenBSD |
| Windows* | nt | win32 | Windows |
\* = I neither have a MacOS nor a Windows machine available, so I extracted the values from documentation and our current code. Also I'm relying on CI for testing the relevant code-paths. Having reviewers to this this locally would be very appreciated, if this gets Concept ACKed.
ACKs for top commit:
kevkevinpal:
ACK [878d914](878d914777)
achow101:
ACK 878d914777a03a04ecb84217152e8b7fd73a5062
hebasto:
ACK 878d914777a03a04ecb84217152e8b7fd73a5062, I have reviewed the code and it looks OK.
pablomartin4btc:
tACK 878d914777a03a04ecb84217152e8b7fd73a5062
Tree-SHA512: 24513d493e47f572028c843260b81c47c2c29bfb701991050255c9f9529cd19065ecbc7b3b6e15619da7f3f608b4825c345ce6fee30d8fd1eaadbd08cff400fc
997b9a73e5166b4244f7c5b4fe144d524f3005f4 test: add assumeutxo wallet test (Sjors Provoost)
Pull request description:
Extracted from #28616, this adds a (very) basic wallet test for assume utxo. It checks some circumstances where a backup can and can't be loaded.
ACKs for top commit:
maflcko:
lgtm ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4
achow101:
ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4
theStack:
Code-review ACK 997b9a73e5166b4244f7c5b4fe144d524f3005f4
Tree-SHA512: 69474e56c6a46bb4f30fc54f8e5844766ac2a5f8226bb0b168d11ae1e3d4eae58570c1f1b4cc2b2f6f51b5d0e055bbe2bbd11684265215e01d4eb81ab4b7b0bb
016cc807f77a9128d430a0df1edd133521628a33 test: wallet migration, add coverage for tx extra data (furszy)
Pull request description:
Quick follow-up to #28610, coming from https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938.
Verifying that the 'replaced_by_txid' and 'replaces_txid' tx data is preserved after migration,
as well as the extra tx comments.
ACKs for top commit:
jamesob:
Nice, ACK 016cc807f7
achow101:
ACK 016cc807f77a9128d430a0df1edd133521628a33
pablomartin4btc:
ACK 016cc807f77a9128d430a0df1edd133521628a33
BrandonOdiwuor:
lgtm ACK 016cc807f77a9128d430a0df1edd133521628a33
Tree-SHA512: 697cabece730cbe5c5947bf98455e80a8877c0352fbe2a66362ce5ea530b67882b0bec561a67d48fee200cdad717cd62c57fd809e2a94ff83c3fad30021e1d9e
406b71abcb72f234ddf9245a3f57e748343c774f wallet: Migrate entire address book entries (Andrew Chow)
Pull request description:
Not all of the data in an address book entry was being copied to the watchonly and solvables wallets. This includes information such as whether the address was previously spent, and any receive requests that may exist. A test has been added to check that the previously spent information is copied, although it passes without the changes in this PR since this information is also regenerated when a transaction is loaded/added into a wallet.
ACKs for top commit:
ryanofsky:
Code review ACK 406b71abcb72f234ddf9245a3f57e748343c774f. Just suggested change since last review
furszy:
Code review ACK 406b71ab
Tree-SHA512: 13de42b16a1d8524fe0555764744139566b2e7d29741ceffc1158a905dd537136b762330568b3b5cac28cbee1bfd363a20de97d0a6c5296738cb3aa99133945b
fa46cc22bc696e6845915ae91d6b68e36bf4c242 Remove deprecated -rpcserialversion (MarcoFalke)
Pull request description:
The flag is problematic for many reasons:
* It is deprecated
* It is a global flag, requiring a restart to change, as opposed to a flag that can be set on each RPC invocation
* It may be hidden in config files by accident, hard to debug, causing LND crashes and bugs, see https://github.com/bitcoin/bitcoin/issues/28730#issuecomment-1780940868
* It makes performance improvements harder to implement: https://github.com/bitcoin/bitcoin/pull/17529#issuecomment-556082818
Fix all issues by removing it.
If there is a use-case, likely a per-RPC flag can be added, if needed.
ACKs for top commit:
ajtowns:
crACK fa46cc22bc696e6845915ae91d6b68e36bf4c242
TheCharlatan:
lgtm ACK fa46cc22bc696e6845915ae91d6b68e36bf4c242
Tree-SHA512: 96ba1c60356ce93954fe5c2a59045771c6d1516ad0d9dc436ef1800a1f1b0153f0d5fb78ca99d53ad54ba25fbce36962bdf1d4325aceedfc8154a61347a6a915
Update CheckPackageLimits to use util::Result to pass the error message
instead of out parameter.
Also update test to reflect the error message from `CTxMempool`
`CheckPackageLimits` output.
7b45744df33c6a4759eae1a3984f389cbac837c2 tests: ensure functional tests set permitbaremultisig=1 when needed (Anthony Towns)
7dfabdcf860c529772a54b0e8fa235cbb4a78b4d tests: test both settings for permitbaremultisig in p2sh tests (Anthony Towns)
Pull request description:
Update unit and functional tests so that they continue to work if the default for `-permitbaremultisig` is changed.
ACKs for top commit:
maflcko:
lgtm ACK 7b45744df33c6a4759eae1a3984f389cbac837c2
instagibbs:
crACK 7b45744df3
ajtowns:
> crACK [7b45744](7b45744df3)
achow101:
ACK 7b45744df33c6a4759eae1a3984f389cbac837c2
glozow:
ACK 7b45744df33c6a4759eae1a3984f389cbac837c2, changed default locally and all tests passed
Tree-SHA512: f89f9e2bb11f07662cfd57390196df9e531064e1bd662e1db7dcfc97694394ae5e8014e9d209b9405aa09195bf46fc331b7fba10378065cdb270cbd0669ae904
1ce45baed7dd2da3f1cb85c9c25110e5537451ae rpc: getwalletinfo, return wallet 'birthtime' (furszy)
83c66444d0604f0a9ec3bc3f89d4f1a810b7cda0 test: coverage for wallet birth time interaction with -reindex (furszy)
6f497377aa17cb8a590fd7717fa8ededf4249999 wallet: fix legacy spkm default birth time (furszy)
75fbf444c1e13c6ba0e79a34871534c845a13849 wallet: birth time update during tx scanning (furszy)
b4306e3c8db6cbaedc8845c6d21c750b39f682bf refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy)
Pull request description:
Fixing #28897.
As the user may have imported a descriptor with a timestamp newer
than the actual birth time of the first key (by setting 'timestamp=now'),
the wallet needs to update the birth time when it detects a transaction
older than the oldest descriptor timestamp.
Testing Notes:
Can cherry-pick the test commit on top of master. It will fail there.
ACKs for top commit:
Sjors:
re-utACK 1ce45baed7dd2da3f1cb85c9c25110e5537451ae
achow101:
ACK 1ce45baed7dd2da3f1cb85c9c25110e5537451ae
Tree-SHA512: 10c2382f87356ae9ea3fcb637d7edc5ed0e51e13cc2729c314c9ffb57c684b9ac3c4b757b85810c0a674020b7287c43d3be8273bcf75e2aff0cc1c037f1159f9
ea00f982d21aab51001d422225f00626a74db298 test: fix intermittent error in rpc_net.py (#29030) (Sebastian Falbesoner)
Pull request description:
Asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following getpeerinfo() call, as the debug message is written in the CNode ctor, which means it hasn't necessarily been added to CConnman.m_nodes at this point.
Solve this by using the recently introduced `wait_for_new_peer` helper (see #29006, commit 00e0658e77f66103ebdeb29def99dc9f937c049d), which is more robust.
Fixes#29030.
ACKs for top commit:
maflcko:
lgtm ACK ea00f982d21aab51001d422225f00626a74db298
Tree-SHA512: dda307949a466fb3b24408a8c213d307e0af2155f2e8b4e52c836a22397f9d218bf9d8c54ca55bae62a96d7566f27167db9311dd8801785c327234783af5ed00
Asserting for the debug log message "Added connection peer=" is
insufficient for ensuring that this new connection will show up in a
following getpeerinfo() call, as the debug message is written in the
CNode ctor, which means it hasn't necessarily been added to
CConnman.m_nodes at this point.
Solve this by using the recently introduced `wait_for_new_peer`
helper, which is more robust.
Fixes#29030.
Rather than re-implementing these checks, we can use this test
framework's helper (introduced in commit
c934087b627f7d368458781944f990b0eb479634, PR #24358) called in a test's
`skip_test_if_missing_module` method instead.
00e0658e77f66103ebdeb29def99dc9f937c049d test: fix v2 transport intermittent test failure (#29002) (Sebastian Falbesoner)
Pull request description:
This PR improves the following fragile construct for detection of a new connection to the node under test in `p2p_v2_transport.py`:
6d5790956f/test/functional/p2p_v2_transport.py (L154-L156)
Only relying on the number of peers for that suffers from race conditions, as unrelated previous peers could disconnect at anytime in-between. In the test run in #29002, the following happens:
- `getpeerinfo()` is called the first time -> assigned to `num_peers`
- **previous peer disconnects**, the node's peer count is now `num_peers - 1` (in most test runs, this happens before the first getpeerinfo call)
- new peer connects, the node's peer count is now `num_peers`
- the condition that the node's peer count is `num_peers + 1` is never true, test fails
Use the more robust approach of watching for an increased highest peer id instead (again using the `getpeerinfo` RPC call), with a newly introduced context manager method `TestNode.wait_for_new_peer()`. Note that for the opposite case of a disconnect, no new method is introduced; this is currently used only once in the test and is also simpler.
Still happy to take suggestions for alternative solutions.
Fixes#29002.
ACKs for top commit:
kevkevinpal:
Concept ACK [00e0658](00e0658e77)
maflcko:
Ok, lgtm ACK 00e0658e77f66103ebdeb29def99dc9f937c049d
stratospher:
ACK 00e0658.
Tree-SHA512: 0118b87f54ea5e6e080ff44f29d6af6674c757a588534b3add040da435f4359e71bf85bc0a5eb7170f99cc9956e1a03c35cce653d642d31eed41bbed1f94f44f
97c0dfa8942c7fd62c920deee03b4d0c59aba641 test: Extends MEMPOOL msg functional test (Sergi Delgado Segura)
Pull request description:
Currently, p2p_filter.py::test_msg_mempool is not testing much. This extends the tests so the interaction between sending `MEMPOOL` messages with a filter that does not include all transactions in the mempool reacts, plus how it interacts with `INV` messages, especially after the changes introduced by #27675
ACKs for top commit:
instagibbs:
ACK 97c0dfa894
theStack:
re-ACK 97c0dfa8942c7fd62c920deee03b4d0c59aba641
Tree-SHA512: 746fdc867630f40509e6341f484a238dd855ae6d1be5eca121974491e4ca272dee88af4b90dda55ea9a5a19cbff198fa91ffa0c5bf1ddf0232b2c1b215b05b9a
Currently, p2p_filter.py::test_msg_mempool is not testing much.
This extends the tests so the interaction between sending MEMPOOL messages with
a filter that does not include all transactions in the mempool reacts, plus how
it interacts with INV messages
3ea54e5db7d53da5afa321e1800c29aa269dd3b3 net: Add continuous ASMap health check logging (Fabian Jahr)
28d7e55dff826a69f3f8e58139dbffb611cc5947 test: Add tests for unfiltered GetAddr usage (Fabian Jahr)
b8843d37aed1276ff8527328c956c70c6e02ee13 fuzz: Let fuzzers use filter options in GetAddr/GetAddresses (Fabian Jahr)
e16f420547fc72a5a2902927aa7138e43c0fb7c8 net: Optionally include terrible addresses in GetAddr results (Fabian Jahr)
Pull request description:
There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.
This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.
ACKs for top commit:
achow101:
ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
mzumsande:
ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
brunoerg:
crACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
Tree-SHA512: 777acbfac43cc43ce4a0a3612434e4ddbc65f59ae8ffc9e24f21de09011bccb297f0599cbaa82bcf40ef68e5af582c4e98556379db7ceff7d9f97574a1cf8e09
Only relying on the number of peers for detecting a new connection
suffers from race conditions, as unrelated previous peers could
disconnect at anytime in-between. Use the more robust approach of
watching for an increased highest peer id instead (again using the
`getpeerinfo` RPC call), with a newly introduced context manager
method `TestNode.wait_for_new_peer()`.
Fixes#29009.
Verifying the wallet updates the birth time accordingly when it
detects a transaction with a time older than the oldest descriptor
timestamp.
This could happen when the user blindly imports a descriptor with
'timestamp=now'.
Previously, starting a second bitcoind using the same datadir would
correctly fail to init and shutdown. However during shutdown the PID
file belonging to the first instance would be erroneously removed by
the second process shutting down.
Fix this to only delete the PID file if we created it.
