faa18ca046 wallet: Erase wtxOrderd wtx pointer on removeprunedfunds (MarcoFalke)
Pull request description:
This prevents segfaults, when reading from the freed memory.
Tree-SHA512: 04f8190dea7901cf1cc298d5db98c83b02858f27114c5ef4da738accd176d6647d6b81f3dc39f3d5912b1a981cf0599370fd391c4154ffbde97afc1fac389123
9b72c988a0 scripted-diff: Avoid temporary copies when looping over std::map (Ben Woosley)
Pull request description:
The ::value_type of the std::map/std::multimap/std::unordered_map containers is
std::pair<const Key, T>. Dropping the const results in an unnecessary copy,
for example in C++11 range-based loops.
For this I started with a more general scripted diff, then narrowed it down
based on the inspection showing that all actual map/multimap/unordered_map
variables used in loops start with m or have map in the name.
Tree-SHA512: b656d66b69ffa1eb954124aa8ae2bc5436ca50262abefa93bdda55cfcdaffc5ff90cd40539051a2bd06355ba69ddf245265cc8764eebff66d761b3aec06155a9
fa7a6cf1b3 policy: Treat segwit as always active (MarcoFalke)
Pull request description:
Now that segwit is active for a long time, there is no need to reject transactions with the reason that segwit hasn't activated.
Strictly speaking, this is a bug fix, because with the release of 0.16, we create segwit transactions in our wallet by default without checking if they are allowed by local policy.
More broadly, this simplifies the code as if "premature witness" was always set to true with the corresponding command line args.
Tree-SHA512: 484c26aa3a66faba6b41e8554a91a29bfc15fbf6caae3d5363a3966283143189c4bd5333a610b0669c1238f75620691264e73f6b9f1161cdacf7574d946436da
The ::value_type of the std::map/std::multimap/std::unordered_map containers is
std::pair<const Key, T>. Dropping the const results in an unnecessary copy,
for example in C++11 range-based loops.
For this I started with a more general scripted diff, then narrowed it down
based on the inspection showing that all actual map/multimap/unordered_map
variables used in loops start with m or have map in the name.
-BEGIN VERIFY SCRIPT-
sed -i -E 's/for \(([^<]*)std::pair<([^c])(.+) : m/for (\1std::pair<const \2\3 : m/' src/*.cpp src/**/*.cpp
sed -i -E 's/for \(([^<]*)std::pair<([^c])(.+) : (.*)map/for (\1std::pair<const \2\3 : \4map/' src/*.cpp src/**/*.cpp
-END VERIFY SCRIPT-
16e3cd380a Clarify include recommendation (practicalswift)
6d10f43738 Enforce the use of bracket syntax includes ("#include <foo.h>") (practicalswift)
906bee8e5f Use bracket syntax includes ("#include <foo.h>") (practicalswift)
Pull request description:
When analysing includes in the project it is often assumed that the preferred bracket include syntax (`#include <foo.h>`) mentioned in `developer-docs.md` is used consistently. @sipa:s excellent circular dependencies script [`circular-dependencies.py`](50c69b7801/contrib/devtools/circular-dependencies.py) (#13228) is an example of a script making this reasonable assumption.
This PR enables automatic Travis checking of the include syntax making sure that the bracket syntax includes (`#include <foo.h>`) is used consistently.
Tree-SHA512: a414921aabe8e487ebed42f3f1cbd02fecd1add385065c1f2244cd602c31889e61fea5a801507ec501ef9bd309b05d3c999f915cec1c2b44f085bb0d2835c182
f77e1d34fd test: Add MempoolAncestryTests (Karl-Johan Alm)
a08d76bcfe mempool: Calculate descendant maximum thoroughly (Karl-Johan Alm)
6d3568371e wallet: Switch to using ancestor/descendant limits (Karl-Johan Alm)
6888195b06 wallet: Strictly greater than for ancestor caps (Karl-Johan Alm)
322b12ac4e Remove deprecated TransactionWithinChainLimit (Karl-Johan Alm)
4784751547 Switch to GetTransactionAncestry() in OutputEligibleForSpending (Karl-Johan Alm)
475a385a80 Add GetTransactionAncestry to CTxMemPool for general purpose chain limit checking (Karl-Johan Alm)
46847d69d2 mempool: Fix max descendants check (Karl-Johan Alm)
b9ef21dd72 mempool: Add explicit max_descendants (Karl-Johan Alm)
Pull request description:
Currently, `TransactionWithinChainLimit` is restricted to single-output use, and needs to be called every time for different limits. If it is replaced with a chain limit value calculator, that can be called once and reused, and is generally more flexible (see e.g. #12257).
Update: this PR now corrects usage of max ancestors / max descendants, including calculating the correct max descendant value, as advertised for the two limits.
~~This change also makes `nMaxAncestors` signed, as the replacement method will return `-1` for "not in the mempool", which is different from "0", which means "no ancestors/descendants in mempool".~~
~~This is a subset of #12257.~~
Tree-SHA512: aa59c849360542362b3126c0e29d44d3d58f11898e277d38c034dc4b86a5b4500f77ac61767599ce878c876b5c446fec9c02699797eb2fa41e530ec863a00cf9
67e0e04140 [wallet] [docs] Update release notes for removing `getlabeladdress` (John Newbery)
81608178cf [wallet] [rpc] Remove getlabeladdress RPC (John Newbery)
Pull request description:
labels are associated with addresses (rather than addresses being
associated with labels, as was the case with accounts). The
getlabeladdress does not make sense in this model, so remove it.
getaccountaddress is still supported for one release as the accounts
API is deprecated.
Tree-SHA512: 7f45d0456248ebcc4e54dd34e2578a09a8ea8e4fceda75238ccea9d731dc99a3f3c0519b18a9739de17d2e6e59c9c2259ba67c9ae2e3cb2a40ddb14b9193fe29
Instead of combining the -limitancestorcount and -limitdescendantcount into a nMaxChainLength, this commit uses each one separately in the coin eligibility filters.
TransactionWithinChainLimits would take a 'limit' and check it against ascendants and descendants. This is changed to take an explicit
max ancestors and max descendants value, and to test the corresponding value against its corresponding max.
6aa33feadb Drop UpdateTransaction in favor of UpdateInput (Ben Woosley)
Pull request description:
Updating the input explicitly requires the caller to present a mutable
input, which more clearly communicates the effects and intent of the call
(and, often, the enclosing loop).
In most cases, this input is already immediately available and need not be
looked up.
Tree-SHA512: 8c7914a8b7ae975d8ad0e9d760e3c5da65776a5f79d060b8ffb6b3ff7a32235f71ad705f2185b368d9263742d7796bb562395d22b806d90e8502d8c496011e57
6b8b63af14 Generic TransactionSignatureCreator works with both CTransaction and CMutableTransaction (Martin Ankerl)
Pull request description:
Refactored `TransactionSignatureCreator` into a templated `GenericTransactionSignatureCreator` that works with both `CMutableTransaction` and `CTransaction`.
The advantage is that now in `SignSignature`, the `MutableTransactionSignatureCreator` can now operate directly with the `CMutableTransaction` without the need to copy the data into a `CTransaction`.
Running all unit tests brings a very noticable speedup on my machine:
48.4 sec before this change
36.4 sec with this change
--------
12.0 seconds saved
running only `--run_test=transaction_tests/test_big_witness_transaction`:
16.7 sec before this change
5.9 sec with this change
--------
10.8 seconds saved
This relates to my first attempt with the const_cast hack #13202, and to the slow unit test issue #10026.
Also see #13050 which modifies the tests but not the production code (like this PR) to get a speedup.
Tree-SHA512: 2cff0e9699f484f26120a40e431a24c8bc8f9e780fd89cb0ecf20c5be3eab6c43f9c359cde244abd9f3620d06c7c354e3b9dd3da41fa2ca1ac1e09386fea25fb
4b62bdf513 Wallet: Refactor ReserveKeyFromKeyPool for safety (Ben Woosley)
Pull request description:
ReserveKeyFromKeyPool's previous behaviour is to set nIndex to -1 if the keypool is
empty, OR throw an exception for technical failures. Instead, we now return false
if the keypool is empty, true if the operation succeeded.
This is to make failure more easily detectable by calling code.
Tree-SHA512: 753f057ad13bd4c28d121f426bf0967ed72b827d97fb24582f9326ec60072abc5482e3db69ccada7c5fc66de9957fc59098432dd223fc4116991cab44c6d7aef
Templated version so that no copying of CMutableTransaction into a CTransaction is
necessary. This speeds up the test case transaction_tests/test_big_witness_transaction
from 7.9 seconds to 3.1 seconds on my machine.
Many options are extremely technical, and refer internals, making it
difficult to translate usefully. This came up in discussion of e.g.
#10949. If a message is not understood by translators (which are
typically end-users, not developers) they'll either translate it
literally, making it harder to understand instead of easier, with the
added drawback of the user no longer being able to google it.
Also the translation was only working for bitcoin-qt as with
the console programs, there is no translation backend. So it was
injecting never-used translation messages for bitcoin-cli, -tx.
For these reasons, stop translating options help completely. This should
not affect the output **in any way** except for bitcoin-qt when a
non-English language is configured in the locale.
This implements #10962.
c004ffc9b4 Make handling of invalid in IsMine more uniform (Pieter Wuille)
a53f0feff8 Add some checks for invalid recursion in IsMine (Pieter Wuille)
b5802a9f5f Simplify IsMine logic (Pieter Wuille)
4e91820531 Make IsMine stop distinguishing solvable/unsolvable (Pieter Wuille)
6d714c3419 Make coincontrol use IsSolvable to determine solvability (Pieter Wuille)
Pull request description:
Our current `IsMine` logic does several things with outputs:
* Determine "spendability" (roughly corresponding to "could we sign for this")
* Determine "watching" (is this an output directly or indirectly a watched script)
* Determine invalidity (is this output definitely not legally spendable, detecting accidental uncompressed pubkeys in witnesses)
* Determine "solvability" (would we be able to sign for this ignoring the fact that we may be missing some private keys).
The last item (solvability) is mostly unrelated and only rarely needed (there is just one instance, inside the wallet's coin control logic). This PR changes that instance to use the separate `IsSolvable` function, and stop `IsMine` from distinguishing between solvable and unsolvable.
As an extra, this also simplifies the `IsMine` logic and adds some extra checks (which wouldn't be hit unless someone adds already invalid scripts to their wallet).
Tree-SHA512: 95a6ef75fbf2eedc5ed938c48a8e5d77dcf09c933372acdd0333129fb7301994a78498f9aacce2c8db74275e19260549dd67a83738e187d40b5090cc04f33adf
Add a `createwallet` RPC to allow wallets to be created dynamically at
runtime. This functionality is currently only available through RPC and
newly created wallets will not be displayed in the GUI.
80b4910f7d wallet: Use shared pointer to retain wallet instance (João Barbosa)
Pull request description:
Currently there are 3 places where it makes sense to retain a wallet shared pointer:
- `vpwallets`;
- `interfaces::Wallet` interface instance - used by the UI;
- wallet RPC functions - given by `GetWalletForJSONRPCRequest`.
The way it is now it is possible to have, for instance, listunspent RPC and in parallel unload the wallet (once #13111 is merged) without blocking. Once the RPC finishes, the shared pointer will release the wallet.
It is also possible to get all existing wallets without blocking because the caller keeps a local list of shared pointers.
This is mostly relevant for wallet unloading.
This PR replaces #11402.
Tree-SHA512: b7e37c7e1ab56626085afe2d40b1628e8d4f0dbda08df01b7e618ecd2d894ce9b83d4219443f444ba889096286eff002f163cb0a48f37063b62e9ba4ccfa6cce
fac1223a56 Cache witness hash in CTransaction (MarcoFalke)
faab55fbb1 Make CMutableTransaction constructor explicit (MarcoFalke)
Pull request description:
This speeds up:
* compactblocks (v2)
* ATMP
* validation and miner (via `BlockWitnessMerkleRoot`)
* sigcache (see also unrelated #13204)
* rpc and rest (nice, but irrelevant)
This presumably slows down rescan, which uses a `CTransaction` and its `GetHash`, but never uses the `GetWitnessHash`. The slow down is proportional to the number of witness transactions in the rescan window. I.e. early in the chain there should be no measurable slow down. Later in the chain, there should be a slow down, but acceptable given the speedups in the modules mentioned above.
Tree-SHA512: 443e86acfcceb5af2163e68840c581d44159af3fd1fce266cab3504b29fcd74c50812b69a00d41582e7e1c5ea292f420ce5e892cdfab691da9c24ed1c44536c7
Updating the input explicitly requires the caller to present a mutable
input, which more clearly communicates the effects and intent of the method.
In most cases, this input is already immediately available and need not be
looked up.
ReserveKeyFromKeyPool's previous behaviour is to set nIndex to -1 if the keypool is
empty, OR throw an exception for technical failures. Instead, we now return false
if the keypool is empty, true if the operation succeeded.
This is to make failure more easily detectable by calling code.
labels are associated with addresses (rather than addresses being
associated with labels, as was the case with accounts). The
getlabeladdress does not make sense in this model, so remove it.
getaccountaddress is still supported for one release as the accounts
API is deprecated.
The new `loadwallet` RPC method allows an existing wallet to be loaded
dynamically at runtime.
`unloadwallet` and `createwallet` are not implemented. Notably,
`loadwallet` can only be used to load existing wallets, not to create a
new wallet.
Pass an error message back from CWallet::Verify(), and call
InitError/InitWarning from WalletInit::Verify().
This means that we can call CWallet::Verify() independently from
WalletInit and not have InitErrors printed to stdout. It also means that
the error can be reported to the user if dynamic wallet load fails.
This allows a single wallet to be verified. Prior to this commit, all
wallets were verified together by the WalletInit::Verify() function at
start-up.
Individual wallet verification will be done when loading wallets
dynamically at runtime.
WalletInit::Start calls postInitProcess() for each wallet. Previously
each call to postInitProcess() would attempt to schedule wallet
background flushing.
Just start wallet background flushing once from WalletInit::Start().
159c32d1f1 Add assertion to guide static analyzers. Clang Static Analyzer needs this guidance. (practicalswift)
fd447a6efe Fix dead stores. Values were stored but never read. Limit scope. (practicalswift)
Pull request description:
Fix Clang Static Analyzer warnings reported by @kallewoof in #12961:
* Fix dead stores. Values were stored but never read.
* Add assertion to guide static analyzers. See #12961 for details.
Tree-SHA512: 83dbec821f45217637316bee978e7543f2d2caeb7f7b0b3aec107fede0fff8baa756da8f6b761ae0d38537740839ac9752f6689109c38a4b05c0c041aaa3a1fb
66b0b1b2a6 Add compile time checking for all cs_wallet runtime locking assertions (practicalswift)
Pull request description:
Add compile time checking for `cs_wallet` runtime locking assertions.
This PR is a subset of #12665. The PR was broken up to make reviewing easier.
The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo.
Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without
first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`):
* It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation.
* It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation.
Tree-SHA512: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd
b6f0b4d wallet: Improve logging when BerkeleyDB environment fails to close (Tim Ruffing)
264c643 wallet: Reset BerkeleyDB handle after connection fails (Tim Ruffing)
Pull request description:
According to the BerkeleyDB docs, the DbEnv handle may not be accessed
after close() has been called. This change ensures that we create a new
handle after close() is called. This avoids a segfault when the first
connection attempt fails and then a second connection attempt tries to
call open() on the already closed DbEnv handle.
Without the patch, bitcoindd reliably crashes in the second call to `set_lg_dir()` after `close()` if
there is an issue with the database:
```
2018-05-03T13:27:21Z Bitcoin Core version v0.16.99.0-a024a1841-dirty (debug build)
[...]
2018-05-03T13:27:21Z Using wallet directory /home/tim/.bitcoin
2018-05-03T13:27:21Z init message: Verifying wallet(s)...
2018-05-03T13:27:21Z Using BerkeleyDB version Berkeley DB 4.8.30: (April 9, 2010)
2018-05-03T13:27:21Z Using wallet wallet.dat
2018-05-03T13:27:21Z BerkeleyEnvironment::Open: LogDir=/home/tim/.bitcoin/database
2018-05-03T13:27:21Z BerkeleyEnvironment::Open: Error -30974 opening database environment: DB_RUNRECOVERY: Fatal error, run database recovery
2018-05-03T13:27:21Z Moved old /home/tim/.bitcoin/database to /home/tim/.bitcoin/database.1525354041.bak. Retrying.
2018-05-03T13:27:21Z BerkeleyEnvironment::Open: LogDir=/home/tim/.bitcoin/database ErrorFile=/home/tim/.bitcoin/db.log
[1] 14533 segmentation fault (core dumped) ./src/bitcoind
```
After the fix:
```
2018-05-03T17:19:32Z Bitcoin Core version v0.16.99.0-cc09e3bd0-dirty (release build)
[...]
2018-05-03T17:19:32Z Using wallet directory /home/tim/.bitcoin
2018-05-03T17:19:32Z init message: Verifying wallet(s)...
2018-05-03T17:19:32Z Using BerkeleyDB version Berkeley DB 4.8.30: (April 9, 2010)
2018-05-03T17:19:32Z Using wallet wallet.dat
2018-05-03T17:19:32Z BerkeleyEnvironment::Open: LogDir=/home/tim/.bitcoin/database ErrorFile=/home/tim/.bitcoin/db.log
2018-05-03T17:19:32Z scheduler thread start
2018-05-03T17:19:32Z BerkeleyEnvironment::Open: Error -30974 opening database environment: DB_RUNRECOVERY: Fatal error, run database recovery
2018-05-03T17:19:32Z Moved old /home/tim/.bitcoin/database to /home/tim/.bitcoin/database.1525367972.bak. Retrying.
2018-05-03T17:19:32Z BerkeleyEnvironment::Open: LogDir=/home/tim/.bitcoin/database ErrorFile=/home/tim/.bitcoin/db.log
2018-05-03T17:19:32Z Cache configuration:
2018-05-03T17:19:32Z * Using 2.0MiB for block index database
2018-05-03T17:19:32Z * Using 8.0MiB for chain state database
2018-05-03T17:19:32Z * Using 440.0MiB for in-memory UTXO set (plus up to 286.1MiB of unused mempool space)
2018-05-03T17:19:32Z init message: Loading block index..
[...]
```
Tree-SHA512: b809b318e5014ec47d023dc3dc40826b9706bfb211fa08bc2d29f36971b96caa10ad48d9a3f96c03933be46fa4ff7e00e952ac77bfffb6563767fb08aa4f23d6
a8da482 Bump wallet version for pre split keypool (Andrew Chow)
dfcd9f3 Use a keypool of presplit keys after upgrading to hd chain split (Andrew Chow)
5c50e93 Allow -upgradewallet to upgradewallets to HD (Andrew Chow)
2bcf2b5 Test sethdseed (Andrew Chow)
b5ba01a Add 'sethdseed' RPC to initialize or replace HD seed (Chris Moore)
dd3c07a Separate HaveKey function that checks whether a key is in a keystore (Andrew Chow)
Pull request description:
Revival/rebase of #11085
Adds a new command `sethdseed` which allows you to either set or generate a new HD seed to be used. A new keypool can be generated or the original one kept and new keys added to the keypool will come from the new HD seed.
Wallets that are not HD will be upgraded to be version FEATURE_HD_SPLIT when the `sethdseed` RPC command is used.
I have also add some tests for this.
Additionally `-upgradewallet` can now be used to upgrade a wallet from non-HD to HD. When it is used for such an upgrade, the keypool will be regenerated.
Tree-SHA512: e56c792e150590429ac4a1061e8d6f7b20cca06366e184eb9bbade4cd6ae82699a28fe84f87031eadba97ad2c1606517a105f00fb7b45779c979243020071adb
Bump the wallet version to indicate support for the pre split keypool.
Also prevents any wallets from upgrading to versions between HD_SPLIT
and PRE_SPLIT_KEYPOOL.
After upgrading to HD chain split, we want to continue to use keys
from the old keypool. To do this, before we generate any new keys after
upgrading, we mark all of the keypool entries as being pre-chain
split and move them to a separate pre chain split keypool. Keys are
fetched from that keypool until it is emptied. Only then are the new
internal and external keypools used.
Changes the maximum upgradewallet version to the latest wallet version
number, 159900. Non-HD wallets will be upgraded to use HD derivation.
Non HD chain split wallets will be upgraded to HD chain split.
If a non-HD wallet is upgraded to HD, the keypool will be entirely
regenerated.
Since upgradewallet is effectively run during a first run, all of the
first run initial setup stuff is combined with the upgrade to HD
16be13345 Fix rescanblockchain rpc to property report progress (Ben Woosley)
Pull request description:
Previously it assumed tip in all cases. This also extracts a RescanVerificationProgress helper object whose role is to manage reporting, in order to simplify ScanForWalletTransactions - more lines in total, but much simpler to follow the core logic.
Tree-SHA512: 5ebed0c56fae4ccfe613ff1d7082cb6da5a86635a8993ed3af70b500a4ea43074121aea9219b2f0321fbfeb7efcb964bdc2199297a64ca0fa85d9d07aa637d40
CWallet::ScanForWalletTransactions did not previously take into account
pindexStop when calculating progress.
Renamed progress vars to progress_*.
rescanblockchain is the only rpc that uses this parameter.
According to the BerkeleyDB docs, the DbEnv handle may not be accessed
after close() has been called. This change ensures that we create a new
handle after close() is called. This avoids a segfault when the first
connection attempt fails and then a second connection attempt tries to
call open() on the already closed DbEnv handle.
1e46d8a Get rid of ambiguous OutputType::NONE value (Russell Yanofsky)
Pull request description:
Based on suggestion by @sipa https://github.com/bitcoin/bitcoin/pull/12119#issuecomment-357982763
After #12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool to indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type.
This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
Follows up #12408 by @MarcoFalke
Followups for future PRs:
- [ ] Add explicit support for specifying "auto" in `ParseOutputType` as suggested by promag and sipa: https://github.com/bitcoin/bitcoin/pull/12729#issuecomment-374799567 and https://github.com/bitcoin/bitcoin/pull/12729#discussion_r175969481
- [ ] Add wallet `AddressChangeType` method to complement `TransactionChangeType`: https://github.com/bitcoin/bitcoin/pull/12729#discussion_r175969618.
Tree-SHA512: 8b08b272bcb177a0a9e556dcd965840a7fe601ef83ca97938b879c9b1a33b5b3f96939e1bceef11ba7c644ac21bfd6c1dbc6ca715cd1da4ace50475240e4ee48
9cb6cdc Simplify semantics of ChainStateFlushed callback (Matt Corallo)
50b6533 scripted-diff: Rename SetBestChain callback ChainStateFlushed (Matt Corallo)
Pull request description:
Previously, ChainStateFlushed would fire either if a full flush
completed (which can happen due to memory limits, forced flush, or
on its own DATABASE_WRITE_INTERVAL timer) *or* on a
ChainStateFlushed-specific DATABASE_WRITE_INTERVAL timer. This is
both less clear for clients (as there are no guarantees about a
flush having actually happened prior to the call), and reults in
extra flushes not clearly intended by the code. We drop the second
case, providing a strong guarantee without removing the periodit
timer-based flushing.
This is a follow-up to discussion in #11857.
Tree-SHA512: 22ba3a0954d265d28413dbf87040790ca5b439820ee7bbadab14028295ec190de82ce5cd664426c82e58b706dc84278868026fa8d066702eb6e6962c9ace1f8e
e2f58f4 wallet: Make vpwallets usage thread safe (João Barbosa)
Pull request description:
This PR turns the functions introduced in #13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in #10740.
Tree-SHA512: efaa09e501636cf957aa33de83719ce09dc0c2a19daff741a94ef10d6b7ba5dee538355b80c96ead995140f99f5df0c92fb0e22ae1adb8f397eb478280c8d8c7
This much more accurately captures the meaning of the callback.
-BEGIN VERIFY SCRIPT-
sed -i 's/SetBestChain/ChainStateFlushed/g' src/validationinterface.h src/validationinterface.cpp src/wallet/wallet.h src/wallet/wallet.cpp src/validation.cpp src/index/txindex.h src/index/txindex.cpp
-END VERIFY SCRIPT-
d8e9a2a Remove "rpc" category from GetWarnings (Wladimir J. van der Laan)
7da3b0a rpc: Move RPC_FORBIDDEN_BY_SAFE_MODE code to reserved section (Wladimir J. van der Laan)
2ae705d Remove Safe mode (Andrew Chow)
Pull request description:
Rebase of #10563. Safe mode was [disabled by default and deprecated in 0.16](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.16.0.md#safe-mode-disabled-by-default), so probably should be removed for 0.17.
> Rationale:
>
> Safe mode is useless. It only disables some RPC commands when large work forks are detected. Nothing else is affected by safe mode. It seems that very few people would be affected by safe mode. The people who use Core as a wallet are primarily using it through the GUI, which safe mode does not effect. In the GUI, transactions will still be made as normal; only a warning is displayed.
>
> I also don't think that we should be disabling RPC commands or any functionality in general. If we do, it should be done consistently, which safe mode is not. If we want to keep the idea of a safe mode around, I think that the current system needs to go first before a new system can be implemented.
Tree-SHA512: 067938f47ca6e879fb6c3c4e21f9946fd7c5da3cde67ef436f1666798c78d049225b9111dc97064f42b3bc549d3915229fa19ad5a634588f381e34fc65d64044
7d0f80b Use anonymous namespace instead of static functions (Pieter Wuille)
b61fb71 Mention removal of bare multisig IsMine in release notes (Pieter Wuille)
9c2a8b8 Do not treat bare multisig as IsMine (Pieter Wuille)
08f3228 Optimization: only test for witness scripts at top level (Pieter Wuille)
3619735 Track difference between scriptPubKey and P2SH execution in IsMine (Pieter Wuille)
ac6ec62 Switch to a private version of SigVersion inside IsMine (Pieter Wuille)
19fc973 Do not expose SigVersion argument to IsMine (Pieter Wuille)
fb1dfbb Remove unused IsMine overload (Pieter Wuille)
952d821 Make CScript -> CScriptID conversion explicit (Pieter Wuille)
Pull request description:
Currently our wallet code will treat bare multisig outputs (meaning scriptPubKeys with multiple public keys + `OP_CHECKMULTISIG` operator in it) as ours without the user asking for it, as long as all private keys in it are in our wallet.
This is a pointless feature. As it only works when all private keys are in one place, it's useless compared to single key outputs (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH), and worse in terms of space, cost, UTXO size, and ability to test (due to lack of address format for them).
Furthermore, they are problematic in that producing a list of all `scriptPubKeys` we accept is not tractable (it involves all combinations of all public keys that are ours). In further wallet changes I'd like to move to a model where all scriptPubKeys that are treated as ours are explicit, rather than defined by whatever keys we have. The current behavior of the wallet is very hard to model in such a design, so I'd like to get rid of it.
I think there are two options:
* Remove it entirely (do not ever accept bare multisig outputs as ours, unless watched)
* Only accept bare multisig outputs in situations where the P2SH version of that output would also be acceptable
This PR implements the first option. The second option was explored in #12874.
Tree-SHA512: 917ed45b3cac864cee53e27f9a3e900390c576277fbd6751b1250becea04d692b3b426fa09065a3399931013bd579c4f3dbeeb29d51d19ed0c64da75d430ad9a
fac0db0 wallet: Make fee settings non-static members (MarcoFalke)
Pull request description:
The wallet header defined some globals (they were called "settings"), that should be class members instead.
This commit is hopefully only refactoring, apart from a multiwallet bugfix: Calling the rpc `settxfee` for one wallet, would set (and change) the fee rate for all loaded wallets. (See added test case)
Tree-SHA512: 4ab6ec2f5c714742396ded5e451ec3b1ceb771e3696492de29889d866de4365b3fbe4a2784d085c8b8bd11b1ebb8a1fec99ab2c62eee716791cfc67c0cf29e1b
cead28b [docs] Add release notes for deprecated 'account' API (John Newbery)
72c9575 [wallet] [tests] Add tests for accounts/labels APIs (John Newbery)
109e05d [wallet] [rpc] Deprecate wallet 'account' API (John Newbery)
3576ab1 [wallet] [rpc] Deprecate account RPC methods (John Newbery)
3db1ba0 [tests] Set -deprecatedrpc=accounts in tests (John Newbery)
4e671f0 [tests] Rename rpc_listtransactions.py to wallet_listtransactions.py (John Newbery)
a28b907 [wallet] [rpc] Remove duplicate entries in rpcwallet.cpp's CRPCCommand table (John Newbery)
Pull request description:
Deprecate all accounts functionality and make it only accessible by using `-deprecatedrpc=accounts`.
Accounts specific RPCs, account arguments, and account related results all require the `-deprecatedrpc=accunts` startup option now in order to see account things.
Several wallet functional tests use the accounts system. Those tests are unchanged, except to start the nodes with `-deprecatedrpc=accounts`. We can slowly migrate those tests to use the 'label' API instead of the 'account' API before accounts are fully removed.
Tree-SHA512: 89f4ae2fe6de4a1422f1817b0997ae22d63ab5a1a558362ce923a3871f3e42963405d6573c69c27f1764679cdee5b51bf52202cc407f1361bfd8066d652f3f37
1f83839 [wallet] [tests] Test disallowed multiwallet params (John Newbery)
3476e3c [wallet] Fix zapwallettxes/multiwallet interaction. (John Newbery)
Pull request description:
`-zapwallettxes` should be disallowed when starting bitcoin in multiwallet mode.
There's code in `WalletInit::ParameterInteraction()` to disallow `-zapwallettxes` when running in multiwallet mode. This code functioned as expected when passing the parameter `-zapwallettxes=1`, but not when passing the parameter `-zapwallettxes` (ie without the value specified). Fix that and add a test.
The new test in the
_[wallet] [tests] Test disallowed multiwallet params_ commit reproduces the bug and should fail against master.
Fixes#12505
Tree-SHA512: 6cd921717e9c7d2773ca84c946c310c2adec8430e37cbecdb33a620f510db3058a72bd328411812ba415111bc52a3367b332c9d15a37a92ccfd7ae785d2f32ab
fae58eca93 tests: Avoid copies of CTransaction (MarcoFalke)
Pull request description:
Avoid the copy (or move) constructor of `CTransaction` in test code, whereever a simple reference can be used instead.
Tree-SHA512: 8ef2077a277d6182996f4671722fdc01a90909ae7431c1e52604aab8ed028910615028caf9b4cb07a9b15fdc04939dea2209cc3189dde7d38271256d9fe1076c
This commit finalizes the deprecation of the wallet 'accounts' API by
removing all account arguments and return values.
RPC behaviour is slightly different if the 'accounts' or 'labels' API is
being used. Those behaviour changes are fully documented in the RPC help
text.
All account RPC methods are now deprecated and can only be called if
bitcoind has been started with the -deprecatedrpc=accounts switch.
Affected RPC methods are:
- getaccount
- getaccountaddress
- getaddressesbyaccount
- getreceivedbyaccount
- listaccouts
- listreceivedbyaccount
- move
- setaccount
ae1d2b030 Give an error when rescan is aborted by the user (Andrew Chow)
69b01e6f8 Add cancel button to rescan progress dialog (Andrew Chow)
Pull request description:
A cancel button is added to the `showProgress` dialog that is used only for rescans. When clicked, `AbortRescan` is called directly to cancel the rescan.
Rescans triggered from the debug console will now be cancelable by clicking the cancel button.
Rescans triggered by a command (e.g. `importmulti`) will now give an error indicating that the rescan was aborted by the user (either by the `abortrescan` command or by clicking cancel).
Tree-SHA512: 4bb14998766de686e2318fbc9805758eccf5dbe628a7257d072c9ae2fb4f61303a0876f49988d6e5eddb261969b8a307c81c0c2df0a42ae909a43d738af3dc1b
be67831 Make DummySignatureCreator a singleton (Pieter Wuille)
190b8d2 Make BaseSignatureCreator a pure interface (Pieter Wuille)
Pull request description:
* Removes the `m_provider` field from `BaseSignatureCreator`. Instead both a `SigningProvider` (which provides keys and scripts) and a `BaseSignatureCreator` (which implements the transaction-specific (or other) signing logic) are passed into and down in `ProduceSignature`, making the two concepts orthogonal.
* Makes `BaseSignatureCreator` a pure interface without constructor, making it easier to implement new derivations of it (for example for message signing).
* As `DummySignatureCreator` now becomes a stateless object, turn it into a singleton `DUMMY_SIGNATURE_CREATOR`.
Tree-SHA512: 5f1f4512e4ea7d02a31df7b9ede55008efa716c5b74a2630ca1c2fc6599584d8bf5f5641487266127f4b3788033803539fbd22b03ef1219c83c10da2d3da3dcd
72ec5b7 debug log number of unknown wallet records on load (Gregory Sanders)
Pull request description:
This would have saved me some time during wallet debugging, with minimal logging clutter.
Tree-SHA512: e11a4d73a5b1d2bd73fe7b75b62fdfa127e21b8641c5b0c76f14ecd292ab374c0d4749f6bd99919b2b3e9cb00c3b5e8179386eb39ac656698306b3b545ee79f1
05c03d1249 rpc: fix type mistmatch in listreceivedbyaddress (joemphilips)
Pull request description:
`txids` filed in return value is supposed be `string` but it was `numeric` in the help message
Tree-SHA512: 7d860994c2d1d9149b41fd7afefc1a44460eede5a023070fcc18b0a4a19a26c5eec5abd157038c15fe7d50a3390bdaf7a4823279129eb1458b0d3c6141a533ee
cab0824 Logprint the start of a rescan (Jonas Schnelli)
Pull request description:
Right now, there is no log entry when a rescan starts which is confusing especially when a "still rescanning" log entry appears after the log-update timeout of 60s or when user manually aborts the rescan.
This PR adds a log entry when a rescan starts.
Tree-SHA512: 8712605af6fd60950bf3904cfb586da6022e44b3da6f3155fe4f02aae16df6044bc504b3d48945ea6d7fe768f0c6cb3282a2e2251d14bf3b7f1dcbd12568b05e
c55aa4f test: Fix sign for expected values (Karl-Johan Alm)
Pull request description:
A number of `BOOST_CHECK_EQUAL` calls would result in warnings about signs.
This PR fixes signedness for all expectation values, sometimes resulting in `int` → `unsigned int`. No other code changes besides adding/removing `U` to/from values.
Running `make &> make_output_...` on master versus on this PR:
```
$ wc make_output_*
1464 5925 90357 make_output_master
613 1469 28370 make_output_signfixed
```
More than halves the output lines from compiling.
Tree-SHA512: b06c9fb81704fd32a6a61fe7b2ceb5f1bb381e9873d79e13d7e4d26bbd9b67c9725a84e6fb2903bcda775aea2a792e544b0799d36735c19f5d1c7225e8c6d14e
41ba061 [docs] Add release notes for wallet 'label' API. (John Newbery)
189e0ef [wallet] [rpc] introduce 'label' API for wallet (Wladimir J. van der Laan)
Pull request description:
Add label API to wallet RPC.
This is one step towards #3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.
These initially mirror the account functions, with the following differences:
- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
associated with addresses, instead of addresses associated with labels. (unlike
with accounts.)
- Labels have no balance
- No balances in `listlabels`
- `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
Being unable to delete them is a common annoyance (see #1231).
Currently only by reassigning all addresses using `setlabel`, but an explicit
call `deletelabel` which assigns all address to the default label may make
sense.
Tree-SHA512: 45cc313c68ad529ce3a15c02181d2ab0083a7e14fe824e2cde34972713fecce512e3d4b9aa46db5355f2baa857c44b234d4fe9709225bc23c7ebbc0e03febbf5
Add label API to wallet RPC.
This is one step towards #3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.
These initially mirror the account functions, with the following differences:
- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
associated with addresses, instead of addresses associated with labels. (unlike
with accounts.)
- Labels have no balance
- No balances in `listlabels`
- `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
Being unable to delete them is a common annoyance (see #1231).
Currently only by reassigning all addresses using `setlabel`, but an explicit
call `deletelabel` which assigns all address to the default label may make
sense.
Thanks to Pierre Rochard for test fixes.
818dc74 Support serialization as another type without casting (Pieter Wuille)
Pull request description:
This adds a `READWRITEAS(type, obj)` macro which serializes `obj` as if it were converted to `const type&` when `const`, and to `type&` when non-`const`. No actual cast is involved, so this only works when this conversion can be done automatically.
This makes it usable in serialization code that uses a single implementation for both serialization and deserializing, which doesn't know the constness of the object involved.
This is a redo of #12712, using a slightly different interface.
Tree-SHA512: 262f0257284ff99b5ffaec9b997c194e221522ba35c3ac8eaa9bb344449d7ea0a314de254dc77449fa7aaa600f8cd9a24da65aade8c1ec6aa80c6e9a7bba5ca7
f526046 adapt bumpfee change discard test to be more strict and add note on p2sh discrep (Gregory Sanders)
5805d6f feebumper: discard change outputs below discard rate (Gregory Sanders)
Pull request description:
The "discard rate" is the concept we use to ensure the wallet isnt creating not so useful just-above-relay dust.
Outside of bumpfee previous to this PR, and manually creating such an output, the wallet will never make change outputs of that size, preferring to send them to fees instead.
"Worst case" for the user is that users pay a slightly higher feerate than they were expecting, which is already a possibility with relay dust.
Tree-SHA512: dd69351810dc1709437602e7db1be46e4e905ccd8e16d03952de8b4c1fdbf9cb7e6c99968930896baf6b5c7cb005a03ec0506a2669d22e21e32982e60329606b
2b2b96cd45 Use std::bind instead of boost::bind to re-lock the wallet (Suhas Daftuar)
662d19ff72 [rpcwallet] Clamp walletpassphrase value at 100M seconds (Suhas Daftuar)
Pull request description:
Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping).
Tree-SHA512: 890f3b641f6c586e2f8f629a9d23bca6ceb8b237b285561aad488cb7adf941a21177d3129d0c2b8293c0a673cd8e401957dbe2b6b3b7c8c4e991bb411d260102
d207207 [logging] add lint-logs.sh to check for newline termination. (John Newbery)
5c21e6c [logging] Comment all continuing logs. (John Newbery)
Pull request description:
Check that all calls to LogPrintf() are terminated by a newline,
except those that are explicitly marked as 'continued' logs.
Tree-SHA512: fe5162b2b2df1e8a4c807da87584fa9af97a6b8377e4090fe0caa136d90bf29a487a123cde94569bdce7101fee3478196d99aa13f1212e24bfe5f41c773604fc
d1b622b tests: Add check for test suite name uniqueness in lint-tests.sh (practicalswift)
dc8067b tests: Add note about uniqueness requirement for test suite names (practicalswift)
3ebfb2d tests: Avoid test suite name collision in wallet crypto_tests (MarcoFalke)
Pull request description:
* Add documentation: Add note about test suite name uniqueness requirement in developer notes
* Add regression test: Update `lint-tests.sh` to make it check also for test suite name uniqueness
Context: #12894 (`tests: Avoid test suite name collision in wallet crypto_tests`)
Tree-SHA512: 3c8502db069ef3d753f534976a86a997b12bac539e808a7285193bf81c9dd8c1b06821c3dd1bdf870ab87722b02c8aa9574c62ace70c2a1b8091785cb8c9aace
Most logs should terminated with a '\n'. Some logs
are built up over multiple calls to logPrintf(), so
do not need a newline terminater. Comment all of
these 'continued' logs as a linter hing.
f63bc5e wallet: Initialize m_last_block_processed to nullptr. Initialize fields where defined. (practicalswift)
Pull request description:
Initialize `m_last_block_processed` to `nullptr`.
`m_last_block_processed` was introduced in 5ee3172636.
Tree-SHA512: 6e4a807e5b02115cbd80460761056f2eb22043203212d88dd0cd44c28dc0abce30ab29b078ca2c612232e76af4886f4fdbf2b0ff75e2df19b4d1a801b236cc13
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at
https://github.com/bitcoin/bitcoin/pull/12119#issuecomment-357982763
After #12119, the NONE output type was overloaded to refer to either an output
type that couldn't be parsed, or to an automatic change output mode. This
change drops the NONE enum and uses a simple bool indicate parse failure, and a
new CHANGE_AUTO enum to refer the change output type.
This change is almost a pure refactoring except it makes RPCs reject empty
string ("") address types instead of treating them like they were unset. This
simplifies the parsing code a little bit and could prevent RPC usage mistakes.
It's noted in the release notes.
a5bca13 Bugfix: Include <memory> for std::unique_ptr (Luke Dashjr)
Pull request description:
Not sure why all these includes were missing, but it's breaking builds for some users:
https://bugs.gentoo.org/show_bug.cgi?id=652142
(Added to all files with a reference to `std::unique_ptr`)
Tree-SHA512: 8a2c67513ca07b9bb52c34e8a20b15e56f8af2530310d9ee9b0a69694dd05e02e7a3683f14101a2685d457672b56addec591a0bb83900a0eb8e2a43d43200509
5b10ab0 [trivial] Add newlines to end of log messages. (John Newbery)
Pull request description:
Log messages should terminate with a '\n', or the following log will be
written to the same line without a timestamp. Fix a couple of cases
where the message is not terminated with a \n.
Tree-SHA512: 88677afe85c88ce9f58312430e8881916bd76bbc8cd353ff81c97b3de8356680503160992c0ef3ea192b4694e848e9ca2480dbc38fea1776903b3784497f1af6
Log messages should terminate with a '\n', or the following log will be
written to the same line without a timestamp. Fix a couple of cases
where the message is not terminated with a \n.
db983beba6 tests: Add lint-tests.sh which checks the test suite naming convention (practicalswift)
5fd864fe8a tests: Rename test suits not following the test suite naming convention (practicalswift)
7b4a296a71 tests: Add note about test suite naming convention (practicalswift)
Pull request description:
Changes:
* Add note about test suite naming convention
* Fix exceptions
* Add regression test
Rationale:
* Consistent naming of test suites makes programmatic test running of specific tests/subsets of tests easier
* Explicit is better than implicit
Before this commit:
```
$ contrib/devtools/lint-tests.sh
The test suite in file src/test/foo_tests.cpp should be named
"foo_tests". Please make sure the following test suites follow
that convention:
src/test/blockchain_tests.cpp:BOOST_FIXTURE_TEST_SUITE(blockchain_difficulty_tests, BasicTestingSetup)
src/test/prevector_tests.cpp:BOOST_FIXTURE_TEST_SUITE(PrevectorTests, TestingSetup)
src/wallet/test/coinselector_tests.cpp:BOOST_FIXTURE_TEST_SUITE(coin_selection_tests, WalletTestingSetup)
src/wallet/test/crypto_tests.cpp:BOOST_FIXTURE_TEST_SUITE(wallet_crypto, BasicTestingSetup)
$
```
After this commit:
```
$ contrib/devtools/lint-tests.sh
$
```
Tree-SHA512: 7258ab9a6b9b8fc1939efadc619e2f2f02cfce8034c7f2e5dc5ecc769aa12e17f6fb8e363817feaf15c026c5b958b2574525b8d2d3f6be69658679bf8ceea9e9
c7ec524 [wallet] Add dummy wallet init class (John Newbery)
49baa4a [wallet] Use global g_wallet_init_interface to init/destroy the wallet. (John Newbery)
caaf972 [wallet] Create wallet init interface. (John Newbery)
5fb5421 [wallet] Move wallet init functions into WalletInit class. (John Newbery)
Pull request description:
This continues the work of #7965. This PR, along with several others, would remove the remaining dependencies from libbitcoin_server.a on libbitcoin_wallet.a.
To create the interface, I've just translated all the old init.cpp wallet function calls into an interface class. I've not done any thinking about whether it makes sense to change that interface by combining/splitting those calls. This is a purely internal interface, so there's no problem in changing it later.
Tree-SHA512: 32ea57615229c33fd1a7f2f29ebc11bf30337685f7211baffa899823ef74b65dcbf068289c557a161c5afffb51fdc38a2ee8180720371f64d433b12b0615cf3f
This commit creates a global g_wallet_init_interface, which is created
in bitcoind and bitcoin-qt. g_wallet_init_interface is used to init
and destroy the wallet.
This removes the dependency from init.cpp on the wallet library.
1f45e21 scripted-diff: Convert 11 enums into scoped enums (C++11) (practicalswift)
Pull request description:
Rationale (from Bjarne Stroustrup's ["C++11 FAQ"](http://www.stroustrup.com/C++11FAQ.html#enum)):
>
> The enum classes ("new enums", "strong enums") address three problems with traditional C++ enumerations:
>
> * conventional enums implicitly convert to int, causing errors when someone does not want an enumeration to act as an integer.
> * conventional enums export their enumerators to the surrounding scope, causing name clashes.
> * the underlying type of an enum cannot be specified, causing confusion, compatibility problems, and makes forward declaration impossible.
>
> The new enums are "enum class" because they combine aspects of traditional enumerations (names values) with aspects of classes (scoped members and absence of conversions).
Tree-SHA512: 9656e1cf4c3cabd4378c7a38d0c2eaf79e4a54d204a3c5762330840e55ee7e141e188a3efb2b4daf0ef3110bbaff80d8b9253abf2a9b015cdc4d60b49ac2b914
2fb9c1e shuffle selected coins before transaction finalization (Gregory Sanders)
Pull request description:
Currently inputs are ordered based on COutPoint ordering, which while doesn't leak additional internal wallet state, likely further fingerprints the wallet as a Core wallet to observers.
Note: This slightly changed behavior of `fundrawtransaction` in that the newly-appended inputs will now be shuffled rather than in outpoint-order. This does not break API compatibility.
Simple shuffling of the coins being returned will hopefully allow the wallet to blend in a bit more, in lieu of additional data to find what other wallets are doing, or another standard, ala @gmaxwell's suggested of ordering via scriptPubKey.
Tree-SHA512: 70689a6eccf9fa7fc6e3d884f2eba4b482446a1e6128beff7a98f446d0c60f7966c5a6c55e9b0b3d73a9b539ce54889a26c7efe78ab7f34af386d5e4f3fa6df2
d2527bd Rename wallet_accounts.py test (Russell Yanofsky)
045eeb8 Rename account to label where appropriate (Russell Yanofsky)
Pull request description:
Rename account to label where appropriate
This change only updates strings and adds RPC aliases, but should simplify the implementation of address labels in https://github.com/bitcoin/bitcoin/pull/7729, by getting renaming out of the way and letting that change focus on semantics.
The difference between accounts and labels is that labels apply only to addresses, while accounts apply to both addresses and transactions (transactions have "from" and "to" accounts). The code associating accounts with transactions is clumsy and unreliable so we would like get rid of it.
---
There is a rebased version of #7729 atop this PR at https://github.com/ryanofsky/bitcoin/commits/pr/label, see https://github.com/bitcoin/bitcoin/pull/7729#issuecomment-338417139.
Tree-SHA512: b3f934e612922d6290f50137f8ba71ddfaea4485713c7d97e89400a8b73b09b254f9186dffa462c77f5847721f5af9852b5572ade5443d8ee95dd150b3edb7ff
081bf54 Test that BnB is not used when there are preset inputs (Andrew Chow)
6ef9982 Actually disable BnB when there are preset inputs (Andrew Chow)
Pull request description:
We don't want to use BnB when there are preset inputs because there
is some weirdness with making that work with using the KnapsackSolver
as the fallback. Currently we say that we haven't used bnb when
there are preset inputs, but we don't actually disable BnB. This fixes
that.
I thought this was done originally. I guess it got lost in a rebase somewhere.
Tree-SHA512: 9792c0cdd0736866bddbed20f10b8050104955dc589fba49a0bd61a582ba491c921af2cdcc2269678b7b69275dad5fcf89c71b75c28733c7bacbe52e55891b9c
4d9b4256d8 Fix typos (Dimitris Apostolou)
Pull request description:
Unfortunately I messed up my repo while trying to squash #12593 so I created a PR with just the correct fixes.
Tree-SHA512: 295d77b51bd2a9381f1802c263de7ffb2edd670d9647391e32f9a414705b3c8b483bb0e469a9b85ab6a70919ea13397fa8dfda2aea7a398b64b187f178fe6a06
This adds a READWRITEAS(type, obj) macro which serializes obj as if it
were casted to (const type&) when const, and to (type&) when non-const.
This makes it usable in serialization code that uses a single
implementation for both serialization and deserializing, which doesn't
know the constness of the object involved.
e5468a19d1 Remove unreachable help conditions (lutangar)
Pull request description:
These conditions on `request.fHelp`, which appears in the body of the following functions are never reached:
* `walletpassphrase`
* `walletpassphrasechange`
* `encryptwallet`
```
...
if (request.fHelp || request.params.size() != 0) {
throw std::runtime_error("");
}
...
if (request.fHelp)
return true;
...
```
The first condition would throw if `request.fHelp` evaluates to `true`.
Tree-SHA512: 1aa41ed233c6bebae27151ab5cc67144d2a408335a3acef3c103e144d6343685f360b1146e14bc8dc1d53d00fcfc6ff1ab6a0eeb0805191172a23b306ab50b79
This change only updates strings and adds RPC aliases, but should simplify the
implementation of address labels in
https://github.com/bitcoin/bitcoin/pull/7729, by getting renaming out of the
way and letting it focus on semantics.
The difference between accounts and labels is that labels apply only to
addresses, while accounts apply to both addresses and transactions
(transactions have "from" and "to" accounts). The code associating accounts
with transactions is clumsy and unreliable so we would like get rid of it.
We don't want to use BnB when there are preset inputs because there
is some weirdness with making that work with using the KnapsackSolver
as the fallback. Currently we say that we haven't used bnb when
there are preset inputs, but we don't actually disable BnB. This fixes
that.
73b5bf2cb Add a test to make sure that negative effective values are filtered (Andrew Chow)
76d2f068a Benchmark BnB in the worst case where it exhausts (Andrew Chow)
6a34ff533 Have SelectCoinsMinConf and SelectCoins use BnB or Knapsack and use it (Andrew Chow)
fab04887c Add a GetMinimumFeeRate function which is wrapped by GetMinimumFee (Andrew Chow)
cd927ff32 Move original knapsack solver tests to coinselector_tests.cpp (Andrew Chow)
fb716f7b2 Move current coin selection algorithm to coinselection.{cpp,h} (Andrew Chow)
4566ab75f Add tests for the Branch and Bound algorithm (Andrew Chow)
4b2716da4 Remove coinselection.h -> wallet.h circular dependency (Andrew Chow)
7d77eb1a5 Use a struct for output eligibility (Andrew Chow)
ce7435cf1 Move output eligibility to a separate function (Andrew Chow)
0185939be Implement Branch and Bound coin selection in a new file (Andrew Chow)
f84fed8eb Store effective value, fee, and long term fee in CInputCoin (Andrew Chow)
12ec29d3b Calculate and store the number of bytes required to spend an input (Andrew Chow)
Pull request description:
This is an implementation of the [Branch and Bound coin selection algorithm written by Murch](http://murch.one/wp-content/uploads/2016/11/erhardt2016coinselection.pdf) (@xekyo). I have it set so this algorithm will run first and if it fails, it will fall back to the current coin selection algorithm. The coin selection algorithms and tests have been refactored to separate files instead of having them all in wallet.cpp.
I have added some tests for the new algorithm and a test for all of coin selection in general. However, more tests may be needed, but I will need help with coming up with more test cases.
This PR uses some code borrowed from #10360 to use effective values when selecting coins.
Tree-SHA512: b0500f406bf671e74984fae78e2d0fbc5e321ddf4f06182c5855e9d1984c4ef2764c7586d03e16fa4b578c340b21710324926f9ca472d5447a0d1ed43eb4357e
b4bc32a451 [wallet] Get rid of CWalletTx default constructor (Russell Yanofsky)
a128bdc9e1 [wallet] Construct CWalletTx objects in CommitTransaction (Russell Yanofsky)
Pull request description:
Two commits:
- `Construct CWalletTx objects in CommitTransaction` moves a bunch of CWalletTx initialization into CWallet::CommitTransaction to dedup some code and avoid future inconsistencies in how wallet transactions are created.
- `Get rid of CWalletTx default constructor` does what is described and eliminates the possibility of empty transaction entries being inadvertently created by mapWallet[hash] accesses.
Both of these changes were originally part of #9381
Tree-SHA512: af3841c4f0539e0662d81b33c5369fc70aa06ddde1c59cb00fb21c9e4c7d9ff47f1edc5040cb463af1333838802c56b3ef875b939e2b804ee45b8e0294a4371c
92fabcd44 Add LookupBlockIndex function (João Barbosa)
43a32b739 Add missing cs_lock in CreateWalletFromFile (João Barbosa)
f814a3e8f Fix cs_main lock in LoadExternalBlockFile (João Barbosa)
c651df8b3 Lock cs_main while loading block index in AppInitMain (João Barbosa)
02de6a6bc Assert cs_main is held when accessing mapBlockIndex (João Barbosa)
Pull request description:
Replace all `mapBlockIndex` lookups with the new `LookupBlockIndex()`. In some cases it avoids a second lookup.
Tree-SHA512: ca31118f028a19721f2191d86f2dd398144d04df345694575a64aeb293be2f85785201480c3c578a0ec99690516205708558c0fd4168b09313378fd4e60a8412
42343c748 Split up and sanitize CAccountingEntry serialization (Pieter Wuille)
029ecac1b Split up and sanitize CWalletTx serialization (Pieter Wuille)
Pull request description:
This is a small subset of changes taken from #10785, fixing a few of the craziest constness violations in the serialization code.
`CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again.
Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`.
Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
Allows SelectCoinsMinConf and SelectCoins be able to switch between
using BnB or Knapsack for choosing coins.
Has SelectCoinsMinConf do the preprocessing necessary to support either
BnB or Knapsack. This includes calculating the filtering the effective
values for each input.
Uses BnB in CreateTransaction to find an exact match for the output.
If BnB fails, it will fallback to the Knapsack solver.
Moves the current coin selection algorithm out of SelectCoinsMinConf
and puts it in coinselection.{cpp,h}. The new function, KnapsackSolver,
instead of taking a vector of COutputs, will take a vector of CInputCoins
that is prepared by SelectCoinsMinConf.
Changes CInputCoin to coinselection and to use CTransactionRef in
order to avoid a circular dependency. Also moves other coin selection
specific variables out of wallet.h to coinselectoin.h
* Z is the zone designator for the zero UTC offset.
* T is the delimiter used to separate date and time.
This makes it clear for the end-user that the date/time logged is
specified in UTC and not in the local time zone.
No change in behavior in the normal case. But buggy mapWallet lookups with
invalid txids will now throw exceptions instead of inserting dummy entries into
the map, and potentially causing segfaults and other failures.
This also makes it a compiler error to use the mapWallet[hash] syntax which
could create dummy entries.
Construct CWalletTx objects in CWallet::CommitTransaction, instead of having
callers do it. This ensures CWalletTx objects are constructed in a uniform way
and all fields are set.
This also makes it possible to avoid confusing and wasteful CWalletTx copies in
https://github.com/bitcoin/bitcoin/pull/9381
There is no change in behavior.
be8ab7d08 Create new wallet databases as directories rather than files (Russell Yanofsky)
26c06f24e Allow wallet files not in -walletdir directory (Russell Yanofsky)
d8a99f65e Allow wallet files in multiple directories (Russell Yanofsky)
Pull request description:
This change consists of three commits:
* The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory.
* The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory.
* The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up.
All three commits should be straightforward:
* The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper).
* The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test.
* The third commit just changes the mapping from specified wallet paths to bdb environment & data paths.
---
**Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at https://github.com/bitcoin/bitcoin/pull/11687#issuecomment-345625565. Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit.
Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
f08761371 Add tests of listreceivedbyaddress address filtering (Jeremy Rubin)
8ee08120d Add address filtering to listreceivedbyaddress (Jeremy Rubin)
Pull request description:
Supersede https://github.com/bitcoin/bitcoin/pull/9503 created by @JeremyRubin , I will maintain it.
Tree-SHA512: 2accaed493b7e1c2eb5cb5270180f100f8c718b6585b9574f294191c318dc622a79e42ac185300f291f82d3b2a6f1c00850b6b17e4ff2dbab94d71df695acbfe
92f1f8b31 Split off key_io_tests from base58_tests (Pieter Wuille)
119b0f85e Split key_io (address/key encodings) off from base58 (Pieter Wuille)
ebfe217b1 Stop using CBase58Data for ext keys (Pieter Wuille)
32e69fa0d Replace CBitcoinSecret with {Encode,Decode}Secret (Pieter Wuille)
Pull request description:
This PR contains some of the changes left as TODO in #11167 (and built on top of that PR). They are not intended for backporting.
This removes the `CBase58`, `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey` classes, in favor of simple `Encode`/`Decode` functions. Furthermore, all Bitcoin-specific logic (addresses, WIF, BIP32) is moved to `key_io.{h,cpp}`, leaving `base58.{h,cpp}` as a pure utility that implements the base58 encoding/decoding logic.
Tree-SHA512: a5962c0ed27ad53cbe00f22af432cf11aa530e3efc9798e25c004bc9ed1b5673db5df3956e398ee2c085e3a136ac8da69fe7a7d97a05fb2eb3be0b60d0479655
2736c9e05 Avoid unintentional unsigned integer wraparounds in tests (practicalswift)
Pull request description:
Avoid unintentional unsigned integer wraparounds in tests.
This is a subset of #11535 as suggested by @MarcoFalke :-)
Tree-SHA512: 4f4ee8a08870101a3f7451aefa77ae06aaf44e3c3b2f7555faa2b8a8503f97f34e34dffcf65154278f15767dc9823955f52d1aa7b39930b390e57cdf2b65e0f3
This change should make it easier for users to make complete backups of wallets
because they can now just back up the specified `-wallet=<path>` path directly,
instead of having to back up the specified path as well as the transaction log
directory (for incompletely flushed wallets).
Another advantage of this change is that if two wallets are located in the same
directory, they will now use their own BerkeleyDB environments instead using a
shared environment. Using a shared environment makes it difficult to manage and
back up wallets separately because transaction log files will contain a mix of
data from all wallets in the environment.
Remove requirement that two wallet files can only be opened at the same time if
they are contained in the same directory.
This change mostly consists of updates to function signatures (updating
functions to take fs::path arguments, instead of combinations of strings,
fs::path, and CDBEnv / CWalletDBWrapper arguments).
New global variables were introduced in #11882 and not setting them causes:
wallet/test/wallet_tests.cpp(638): error in "ListCoins": check wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy) failed
wallet/test/wallet_tests.cpp(679): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2]
wallet/test/wallet_tests.cpp(686): error in "ListCoins": check available.size() == 2 failed [1 != 2]
wallet/test/wallet_tests.cpp(705): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2]
It's possible to reproduce the failure reliably by running:
src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins
Failures happen nondeterministically because boost test framework doesn't run
tests in a specified order, and tests that run previously can set the global
variables and mask the bug.
3f592b8 [QA] add wallet-rbf test (Jonas Schnelli)
8222e05 Disable wallet fallbackfee by default on mainnet (Jonas Schnelli)
Pull request description:
Removes the default fallback fee on mainnet (but keeps it on testnet/regtest).
Transactions using the fallbackfee in case the fallback fee has not been set are getting rejected.
Tree-SHA512: e54d2594b7f954e640cc513a18b0bfbe189f15e15bdeed4fe02b7677f939bca1731fef781b073127ffd4ce08a595fb118259b8826cdaa077ff7d5ae9495810db
90ba2df11 Fix missing cs_main lock for GuessVerificationProgress() (Jonas Schnelli)
Pull request description:
`GuessVerificationProgress()` needs `cs_main` due to accessing the `pindex->nChainTx`.
This adds a `AssertLockHeld` in `GuessVerificationProgress()` and adds the missing locks in...
* `LoadChainTip()`
* `ScanForWalletTransactions()` (got missed in #11281)
* GUI, `ClientModel::getVerificationProgress()` <--- **this may have GUI performance impacts**, but could be relaxed later with a cache or something more efficient.
Tree-SHA512: 13302946571422375f32af8e396b9d2c1180f2693ea363aeba9e98c8266ddec64fe7862bfdcbb5a93a4b12165a61eec1e51e4e7d7a8515fa50879095dc163412
Allow importmulti RPC to apply address labels when importing standard
scriptPubKeys. This makes the importmulti RPC less finnicky about import
formats and also simpler internally.
Splits signrwatransaction into a wallet version (signrawtransactionwithwallet) and
non-wallet version (signrawtransactionwithkey). signrawtransaction is marked as DEPRECATED
and will call the right signrawtransaction* command as per the parameters in order to
maintain compatibility.
Updated signrawtransactions test to use new RPCs
Moves the parts of validateaddress which require the wallet into getaddressinfo
which is part of the wallet RPCs. Mark those parts of validateaddress which
require the wallet as deprecated.
Validateaddress will call getaddressinfo
for the data that both share for right now.
Moves IsMine functions to libbitcoin_common and then links libbitcoin_wallet
before libbitcoin_common in order to prevent linker errors since IsMine is no
longer used in libbitcoin_server.
bb00c95 Consistently use FormatStateMessage in RPC error output (Ben Woosley)
8b8a1c4 Add test for 'mempool min fee not met' rpc error (Ben Woosley)
c04e0f6 Fix 'mempool min fee not met' debug output (Ben Woosley)
Pull request description:
Output the value that is tested, rather than the unmodified fee value.
Prompted by looking into: #11955
Tree-SHA512: fc0bad47d4af375d208f657a6ccbad6ef7f4e2989ae2ce1171226c22fa92847494a2c55cca687bd5a1548663ed3313569bcc31c00d53c0c193a1b865dd8a7657
6ef86c9 Do not un-mark fInMempool on wallet txn if ATMP fails. (Matt Corallo)
Pull request description:
Irrespective of the failure reason, un-marking fInMempool
out-of-order is incorrect - it should be unmarked when
TransactionRemovedFromMempool fires.
Clean up of #11839, which I think was the wrong fix.
Tree-SHA512: 580731297eeac4c4c99ec695e15b09febf62249237bc367fcd1830fc811d3166f9336e7aba7f2f6f8601960984ae22cebed781200db0f04e7cd2008db1a83f64
New global variables were introduced in #11403 and not setting them causes:
test_bitcoin: wallet/wallet.cpp:4259: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed.
unknown location(0): fatal error in "importwallet_rescan": signal: SIGABRT (application abort requested)
It's possible to reproduce the failure reliably by running:
src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/importwallet_rescan
Failures happen nondeterministically because boost test framework doesn't run
tests in a specified order, and tests that run previously can set the global
variables and mask the bug.
91986ed206 scripted-diff: Use UniValue.pushKV instead of push_back(Pair()) (Karel Bilek)
a570098021 Squashed 'src/univalue/' changes from 07947ff2da..51d3ab34ba (MarcoFalke)
Pull request description:
Rebased version of #11386 by karel-3d.
Closes: #11386
Tree-SHA512: f3a81447e573c17e75813f4d41ceb34b9980eac81efdd98ddb149d7c51f792be7e2b32239b6ea7e6da68af23897afa6b4ce3f4e8070f9c4adf5105bf6075f2a0
faca18dcf feebumper: Use PreconditionChecks to determine bump eligibility (MarcoFalke)
718f05cab move more bumpfee prechecks to feebumper::PreconditionChecks (Gregory Sanders)
Pull request description:
This only affects the gui.
Fee-bumping of transactions that are already confirmed or are already conflicted by other transactions should not be offered by the gui.
Tree-SHA512: 4acf8087c69fbe5bd67be0485cdb4055e985bbf84acc420aa786ad31e2dc6c2572baaac1d359af10a6907790f626edca690285d9a46ae5440900ea12624c634f
004f999 boost: drop boost threads for [alert|block|wallet]notify (Cory Fields)
0827267 boost: drop boost threads from torcontrol (Cory Fields)
ba91724 boost: remove useless threadGroup parameter from Discover (Cory Fields)
f26866b boost: drop boost threads for upnp (Cory Fields)
Pull request description:
This doesn't completely get rid of boost::thread, but this batch should be easy to review, and leaves us with only threadGroup (scheduler + scriptcheck) remaining.
Note to reviewers: The upnp diff changes a bunch of whitespace, it's much more clear with 'git diff -w'
Tree-SHA512: 5a356798d0785f93ed143d1f0afafe890bc82f0d470bc969473da2d2aa78bcb9b096f7ba11b92564d546fb447d4bd0d347e7842994ea0170aafd53fda7e0a66e
2f960b5 [wallet] Indent only change of CWallet::AvailableCoins (João Barbosa)
1beea7a [wallet] Make CWallet::ListCoins atomic (João Barbosa)
Pull request description:
Fix a potencial race in `CWallet::ListCoins`.
Replaces `cs_main` and `cs_wallet` locks by assertions in `CWallet::AvailableCoins`.
Tree-SHA512: 09109f44a08b4b53f7605d950ab506d3f748490ab9aed474aa200e93f7b0b9f96f9bf60abe1c5f658240fd13d9e3267c0dd43fd3c1695d82384198ce1da8109f
fa795cf wallet: Disallow abandon of conflicted txes (MarcoFalke)
Pull request description:
Abandon transactions that are already conflicted is a noop, so don't try and return false/throw instead.
Tree-SHA512: fd2af4149bd2323f7f31fe18685c763790b8589319b4e467b464ab456d5e8971501ab16d124e57a22693666b06ae433ac3e59f0fd6dfbd2be2c6cae8be5bcbd8
db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery)
cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery)
ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery)
d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery)
c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery)
a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery)
a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery)
d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery)
Pull request description:
There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched.
- `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (#11031)
- the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (#10858)
- providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (#11415)
- The return format from `addmultisigaddress` has changed (#11415)
`getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after #11739 and #11398 have been merged and the segwit test code tidied up)
Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124
45eea40 Bech32 addresses in dumpwallet (fivepiece)
Pull request description:
Output bech32 addresses in dumpwallet if address type is not as legacy
Tree-SHA512: f6b6f788293779fe6339b94d9b792180e1d1dcb9c8e826caef8693557e1710213ba57891981c17505ace8d67b407eeca6fd9a8825757dd292cca2aa12575d15c
9ad6746ccd Use static_cast instead of C-style casts for non-fundamental types (practicalswift)
Pull request description:
A C-style cast is equivalent to try casting in the following order:
1. `const_cast(...)`
2. `static_cast(...)`
3. `const_cast(static_cast(...))`
4. `reinterpret_cast(...)`
5. `const_cast(reinterpret_cast(...))`
By using `static_cast<T>(...)` explicitly we avoid the possibility of an unintentional and dangerous `reinterpret_cast`. Furthermore `static_cast<T>(...)` allows for easier grepping of casts.
For a more thorough discussion, see ["ES.49: If you must use a cast, use a named cast"](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast) in the C++ Core Guidelines (Stroustrup & Sutter).
Tree-SHA512: bd6349b7ea157da93a47b8cf238932af5dff84731374ccfd69b9f732fabdad1f9b1cdfca67497040f14eaa85346391404f4c0495e22c467f26ca883cd2de4d3c
c409b1adac [rpc] Reduce scope of cs_main and cs_wallet locks in listtransactions (João Barbosa)
Pull request description:
Trivial change, no behaviour change.
Benchmark done as follow:
- run with `-regtest`
- wallet with 5000 transactions
- measured the time spent with the lock and the total time
- times are an average of 100 `listtransactions --count=...` calls
| `--count` | lock (ms) | total (ms) | saving |
|--:|--:|--:|--:|
| 10 | 0.2230 | 0.2510 | 11% |
| 100 | 2.5150 | 2.8690 | 12% |
| 1000 | 20.0320 | 23.3490 | 14% |
| 10000 | 105.2070 | 125.5310 | 16% |
Tree-SHA512: ebedfeeb4c8ad75c89128e53cae976a82967dbb5ffd129da0f7204ccf9c3c15070b3d509f3767bebd745512e410200cc546147c836e82409f95fc9b8d14fc3ed
d3e467520f Properly alphabetize output of CLI --help option. (murrayn)
Pull request description:
The --help output of bitcoind, bitcoin-cli, bitcoin-tx, qt/bitcoin-qt, et al. is only about 90% alphabetized by option, which is kind of sloppy and occasionally misleading. This change (mostly) organizes the output alphabetically.
Tree-SHA512: 3029900dbe99f03397c1cbdb5e4ac09a13bc99bafe73c6855097206e4cdd9ad70d0b5cedb5e1e520005c3e9ef1c4cd32bb9d8c98ce6918d8434fec8bf06e56c8
16f6f59dc [qa] Test fundrawtransaction with change_type option (João Barbosa)
536ddeb17 [rpc] Add change_type option to fundrawtransaction (João Barbosa)
31dbd5af4 [wallet] Add change type to CCoinControl (João Barbosa)
Pull request description:
Adds a new option `change_type` to `fundrawtransaction` RPC. This is useful to override the node `-changetype` argument.
The new option is exclusive to `changeAddress` option, setting both raises a RPC error.
See also #11403, #12119.
Tree-SHA512: 654686444f6125e37015a62f167064d54ec335701534988447be4687fa5ef9c7980a8a07cc0a03fff6ea6c4c1abf0f77a8843d535c4f3fe0bf93f968a4e676e6
f523c6bec [qa] Use address type in addmultisigaddress to avoid addwitnessaddress (João Barbosa)
886a92f25 [rpc] Add address type option to addmultisigaddress (João Barbosa)
Pull request description:
Adds the option `address_type` to `addmultisigaddress` and `createmultisg` RPC. This also allows to avoid `addwitnessaddress` to obtain an `p2sh-segwit` or `bech32` multsig address.
Related to #12210 as this reduces `addwitnessaddress` usage.
Tree-SHA512: 8f8f85dfcff66bb6c7e1e9865e37c285dead1d6dadb9672a89b92fa209d03cc35817ca1d656588c6c2146b728daaf7540b851929b640294653c62836cbefe7ee
596c446 [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH (Sjors Provoost)
Pull request description:
If `-changetype` is not explicitly set, then regardless of `-addresstype`, the wallet will use a ~`bech32` change address~ `P2WPKH` change output if any destination is `P2WPKH` or `P2WSH`.
This seems more intuitive to me and more in line with the spirit of [BIP-69](https://github.com/bitcoin/bips/blob/master/bip-0069.mediawiki).
When combined with #11991 a QT user could opt to use `bech32` exclusively without having to figure out how to launch with `-changetype=bech32`, although so would #11937.
Tree-SHA512: 9238d3ccd1f3be8dfdd43444ccf45d6bdc6584ced3172a3045f3ecfec4a7cc8999db0cdb76ae49236492a84e6dbf3a1fdf18544d3eaf6d518e1f8bd241db33e7
1df206f Disallow using addresses in createmultisig (Andrew Chow)
Pull request description:
This PR should be the last part of #7965.
This PR makes createmultisig only accept public keys and marks the old functionality of accepting addresses as deprecated.
It also splits `_createmultisig_redeemscript` into two functions, `_createmultisig_getpubkeys` and `_createmultisig_getaddr_pubkeys`. `_createmultisig_getpubkeys` retrieves public keys from the RPC parameters and `_createmultisig_getaddr_pubkeys` retrieves addresses' public keys from the wallet. `_createmultisig_getaddr_pubkeys` requires the wallet and is only used by `addwitnessaddress` (except when `createmultisig` is used in deprecated mode).
`addwitnessaddress`'s API is also changed. Instead of returning just an address, it now returns the same thing as `createmultisig`: a JSON object with two fields, address and redeemscript.
Tree-SHA512: a5796e41935ad5e47d8165ff996a8b20d5112b5fc1a06a6d3c7f5513c13e7628a4fd37ec30fde05d8b15abfed51bc250710140f6834b13f64d0a0e47a3817969
7f81250 Mention that other RPC calls report keys as "imported" while txns are still missing (Jonas Schnelli)
ccd8ef6 Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock (Jonas Schnelli)
bc356b4 Make sure WalletRescanReserver has successfully reserved the rescan (Jonas Schnelli)
dbf8556 Add RAII wallet rescan reserver (Jonas Schnelli)
8d0b610 Avoid pemanent cs_main/cs_wallet lock during wallet rescans (Jonas Schnelli)
Pull request description:
Right now, we are holding `cs_main`/`cs_wallet` during the whole rescan process (which can take a couple of hours).
This was probably only done because of laziness and it is an important show-stopper for #11200 (GUI rescan abort).
Tree-SHA512: 0fc3f82d0ee9b2f013e6bacba8d59f7334306660cd676cd64c47bb305c4cb7c7a36219d6a6f76023b74e5fe87f3ab9fc7fd2439e939f71aef653fddb0a1e23b1
Also warn if bitcoind is configured to use a relative -datadir path.
Specifying paths relative to the current working directory in a daemon process
can be dangerous, because files can fail to be located even if the
configuration doesn't change, but the daemon is started up differently.
Specifying a relative -datadir now adds a warning to the debug log. It would
not be backwards-compatible to forbid relative -datadir paths entirely, and it
could also be also inconvenient for command line testing.
Specifying a relative -walletdir now results in a startup error. But since the
-walletdir option is new in 0.16.0, there should be no compatibility issues.
Another reason not to use working directory paths for -walletdir specifically
is that the default -walletdir is a "wallets" subdirectory inside the datadir,
so it could be surprising that setting -walletdir manually would choose a
directory rooted in a completely different location.
134cdc7 Test walletpassphrase timeout bounds and clamping (Andrew Chow)
0b63e3c Clamp walletpassphrase timeout to 2^(30) seconds and check its bounds (Andrew Chow)
Pull request description:
Fixes#12100
Makes the timeout be clamped to 2^30 seconds to avoid the issue with sign flipping with large timeout values and thus relocking the wallet instantly. Unlocking for at most ~34 years should be sufficient.
Also checks that the timeout is not negative to avoid instant relocks.
Tree-SHA512: 426922f08c54e323d259e25dcdbebc2cd560708a65111ce6051493a7e7c61e79d9da1ea4026cc0d68807d728f5d7c0d7c58168c6ef4167b94cf6c2877af88794
63ac8907c [qt] receive tab: bech32 address opt-in checkbox (Sjors Provoost)
Pull request description:
<img width="647" alt="schermafbeelding 2018-01-12 om 18 34 48" src="https://user-images.githubusercontent.com/10217/34887691-a6a796fe-f7c7-11e7-8b89-87ce07c61ce3.png">
Checkbox does what you would expect. Press tab from the amount field to get there.
It's unchecked by default.
When launched with `-addresstype=bech32` it's checked by default. When launched with `-addresstype=legacy` it unchecked and disabled.
The change in `receivecoinsdialog.ui` is smaller than it looks, due to the way git handles XML diffs. I had to add a horizontal spacer to make it look decent, see https://github.com/bitcoin/bitcoin/issues/11950#issuecomment-352870909. This causes column numbers to change in the rest of the grid.
I recommend testing on at least one other OS than OSX to be on the safe side.
Tree-SHA512: ec4b733b796d9a94278a5d8040a69d9574ef50021e68f94f61f2da75d1bb57f39272cbc9f1f7d34f733a19640daf666a23844fcd132f83bfdaf327d9d1d6f105
5f911c5cc2 trivial: fix address_type help text of getnewaddress and getrawchangeaddress (mruddy)
Pull request description:
"p2sh" in the help messages should have been "p2sh-segwit".
The messages before this patch:
`help getnewaddress`
"address_type" (string, optional) The address type to use. Options are "legacy", "**p2sh**", and "bech32". Default is set by -addresstype.
`help getrawchangeaddress`
"address_type" (string, optional) The address type to use. Options are "legacy", "**p2sh**", and "bech32". Default is set by -changetype.
Tree-SHA512: 6dfc0bebe577995f5521b83a12854045ac3eda4e65c9b92fc581da4ee68ab1218e05af82f2154bb2640a0813c5f79e010cd9e5ada449494c8831b3757bda854c
Clamps the timeout of walletpassphrase to 2^(30) seconds, which is
~34 years. Any number greater than that will be forced to be
2^(30). This avoids the sign flipping problem with large values which
can result in a negative time used.
Also perform bounds checks to ensure that the timeout is positive
to avoid immediate relocking of the wallet.
Make createmultisig only accept public keys with the old functionality
marked as deprecated.
Splits _createmultisig_redeemscript into two functions, one for
getting public keys from UniValue and one for getting addresses
from UniValue and then their respective public keys. The one for
retrieving address's public keys is located in rpcwallet.cpp
Changes addwitnessaddress's output to be a JSON object with
two fields, address and redeemscript.
Adds a test to deprecated_rpc.py for testing the deprecation.
Update the tests to use addwitnessaddress or give only public keys
to createmultisig. Anything that used addwitnessaddress was also
updated to reflect the new API.
New global variables were introduced in #11403 and not setting them causes:
test_bitcoin: wallet/wallet.cpp:4199: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed.
unknown location(0): fatal error in "ListCoins": signal: SIGABRT (application abort requested)
It's possible to reproduce the failure reliably by running:
src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins
Failures happen nondeterministically because boost test framework doesn't run
tests in a specified order, and tests that run previously can set the global
variables and mask the bug.
b224a47a1 Add address_types test (Pieter Wuille)
7ee54fd7c Support downgrading after recovered keypool witness keys (Pieter Wuille)
940a21932 SegWit wallet support (Pieter Wuille)
f37c64e47 Implicitly know about P2WPKH redeemscripts (Pieter Wuille)
57273f2b3 [test] Serialize CTransaction with witness by default (Pieter Wuille)
cf2c0b6f5 Support P2WPKH and P2SH-P2WPKH in dumpprivkey (Pieter Wuille)
37c03d3e0 Support P2WPKH addresses in create/addmultisig (Pieter Wuille)
3eaa003c8 Extend validateaddress information for P2SH-embedded witness (Pieter Wuille)
30a27dc5b Expose method to find key for a single-key destination (Pieter Wuille)
985c79552 Improve witness destination types and use them more (Pieter Wuille)
cbe197470 [refactor] GetAccount{PubKey,Address} -> GetAccountDestination (Pieter Wuille)
0c8ea6380 Abstract out IsSolvable from Witnessifier (Pieter Wuille)
Pull request description:
This implements a minimum viable implementation of SegWit wallet support, based on top of #11389, and includes part of the functionality from #11089.
Two new configuration options are added:
* `-addresstype`, with options `legacy`, `p2sh`, and `bech32`. It controls what kind of addresses are produced by `getnewaddress`, `getaccountaddress`, and `createmultisigaddress`.
* `-changetype`, with the same options, and by default equal to `-addresstype`, that controls what kind of change is used.
All wallet private and public keys can be used for any type of address. Support for address types dependent on different derivation paths will need a major overhaul of how our internal detection of outputs work. I expect that that will happen for a next major version.
The above also applies to imported keys, as having a distinction there but not for normal operations is a disaster for testing, and probably for comprehension of users. This has some ugly effects, like needing to associate the provided label to `importprivkey` with each style address for the corresponding key.
To deal with witness outputs requiring a corresponding redeemscript in wallet, three approaches are used:
* All SegWit addresses created through `getnewaddress` or multisig RPCs explicitly get their redeemscripts added to the wallet file. This means that downgrading after creating a witness address will work, as long as the wallet file is up to date.
* All SegWit keys in the wallet get an _implicit_ redeemscript added, without it being written to the file. This means recovery of an old backup will work, as long as you use new software.
* All keypool keys that are seen used in transactions explicitly get their redeemscripts added to the wallet files. This means that downgrading after recovering from a backup that includes a witness address will work.
These approaches correspond to solutions 3a, 1a, and 5a respectively from https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2. As argued there, there is no full solution for dealing with the case where you both downgrade and restore a backup, so that's also not implemented.
`dumpwallet`, `importwallet`, `importmulti`, `signmessage` and `verifymessage` don't work with SegWit addresses yet. They're remaining TODOs, for this PR or a follow-up. Because of that, several tests unexpectedly run with `-addresstype=legacy` for now.
Tree-SHA512: d425dbe517c0422061ab8dacdc3a6ae47da071450932ed992c79559d922dff7b2574a31a8c94feccd3761c1dffb6422c50055e6dca8e3cf94a169bc95e39e959
This introduces two command line flags (-addresstype and -changetype) which control
the type of addresses/outputs created by the GUI and RPCs. Certain RPCs allow
overriding these (`getnewaddress` and `getrawchangeaddress`). Supported types
are "legacy" (P2PKH and P2SH-multisig), "p2sh-segwit" (P2SH-P2WPKH and P2SH-P2WSH-multisig),
and "bech32" (P2WPKH and P2WSH-multisig).
A few utility functions are added to the wallet to construct different address type
and to add the necessary entries to the wallet file to be compatible with earlier
versions (see `CWallet::LearnRelatedScripts`, `GetDestinationForKey`,
`GetAllDestinationsForKey`, `CWallet::AddAndGetDestinationForScript`).
73041c3c99 RPC Docs: addmultisigaddress is intended for non-watchonly addresses (Gregory Sanders)
Pull request description:
Spent a couple hours debugging why my p2sh watchonly funds were not appearing in various accounting calls when address was imported via `addmultisigaddress`.
Tree-SHA512: 0673e276e5ca8cdc4c9357bd835a29bd5a994520a78179600944932c700917142930288bf179f5e89b0874beaf1a88bd70129f3a297a46df42a10bab847017bb
595a7ba Increment MIT Licence copyright header year on files modified in 2017 (Akira Takizawa)
Pull request description:
Edited via:
$ contrib/devtools/copyright_header.py update .
ps) It is the same commit as #9450
Tree-SHA512: 274bfcd6cf2914315ed52f6db773a68800ce9d6bd225a3142654483f0bbc3fd865009e62f9d954f65765d038c626e55d2a64e37e16843809adc2f67abe659b6d
97d2b09c12 Add helper to wait for validation interface queue to catch up (Matt Corallo)
36137497f1 Block ActivateBestChain to empty validationinterface queue (Matt Corallo)
5a933cefcc Add an interface to get the queue depth out of CValidationInterface (Matt Corallo)
a99b76f269 Require no cs_main lock for ProcessNewBlock/ActivateBestChain (Matt Corallo)
a734896038 Avoid cs_main in net_processing ActivateBestChain calls (Matt Corallo)
66aa1d58a1 Refactor ProcessGetData in anticipation of avoiding cs_main for ABC (Matt Corallo)
818075adac Create new mutex for orphans, no cs_main in PLV::BlockConnected (Matt Corallo)
Pull request description:
This should fix#11822.
It ended up bigger than I hoped for, but its not too gnarly. Note that "
Require no cs_main lock for ProcessNewBlock/ActivateBestChain" is mostly pure code-movement.
Tree-SHA512: 1127688545926f6099449dca6a4e6609eefc3abbd72f1c66e03d32bd8c7b31e82097d8307822cfd1dec0321703579cfdd82069cab6e17b1024e75eac694122cb
This requires the removal of some very liberal (incorrect) cs_mains
sprinkled in some tests. It adds some chainActive.Tip() races, but
the tests are all single-threaded anyway.
GUI wallet uses RBF by default, regardless of -walletrbf.
RPC and debug console in the GUI remain unchanged; they don't
use RBF by default, unless launched with -walletrbf=1.
656fde5 Add script birthtime metadata to dump and import wallet (MeshCollider)
1bab9b2 Add script dump note to RPC help text and release notes (MeshCollider)
68c1e00 Add test for importwallet (MeshCollider)
9e1184d Add dumpwallet scripts test (MeshCollider)
ef0c730 Add scripts to importwallet RPC (MeshCollider)
b702ae8 Add CScripts to dumpwallet RPC (MeshCollider)
cdc260a Add GetCScripts to CBasicKeyStore (MeshCollider)
Pull request description:
As discussed in https://github.com/bitcoin/bitcoin/pull/11289#issuecomment-334600457, adds the CScripts from the wallet to the `dumpwallet` RPC and then allows them to be imported with the `importwallet` RPC. Includes a basic test, and modifies the helptext of the dumpwallet RPC.
Notes:
- Reviewers: use `?w=1` to avoid the indentation-only change in commit `Add scripts to importwallet RPC `
- currently the scripts are followed with `# addr=` comments just as the other keys are, unsure if this might confuse users into thinking all the scripts are for valid P2SH addresses though, but I don't think that should be an issue.
- there are no birthtimes for scripts, so script imports don't affect rescans
- `importwallet` imports the CScripts but I'm not sure how to approach specifying whether scripts are for P2SH addresses, BIP173 addresses, etc. whether that matters or not. Otherwise the RPC helptext might just need modification.
Fixes#11715
Tree-SHA512: 36c55837b3a58b9d3499d4c0c2ae82153d62aa71919e751574651b63a1d2b8ecc83796db4553cc65dad9b5341c3a42ae2fcf4d62598c30af267f8e1461ba8272
aac6b3f067 Update files.md for new wallets/ subdirectory (MeshCollider)
b67342906c Cleanups for walletdir PR (MeshCollider)
Pull request description:
This addresses the remaining nits from https://github.com/bitcoin/bitcoin/pull/11466
- Updates `doc/files.md` with respect to the new default wallet directory
- Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit
- ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~
- Changes the #includes from "" to <> style after #11651
Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa
ecf9b25 remove unused fNoncriticalErrors variable from CWalletDB::FindWalletTx (Pierre Rochard)
Pull request description:
The `CWalletDB::FindWalletTx` method was patterned after `CWalletDB::LoadWallet`, where `fNoncriticalErrors` is used when a tx check fails in `ReadKeyValue`.
Since `FindWalletTx` is only used by methods which are zapping txs, it makes sense that `ReadKeyValue` is not called and the tx is not checked, so I think that deleting the unused `fNoncriticalErrors` boolean variable and its conditional statement is appropriate.
Tree-SHA512: 0976eae97522719fdaeca1fb3f4a080561e46c06d0b8dc75e14262c6bc242998db3f7057183a230a1d7e4ac5fc348e9059f545b7d718ebbcdf6dcdfc63bcc286
6f39ac0 Add test for decoderawtransaction bool (MeshCollider)
bbdbe80 Add iswitness parameter to decode- and fundrawtransaction RPCs (MeshCollider)
Pull request description:
Suggested in https://github.com/bitcoin/bitcoin/pull/10481#issuecomment-325244946, this adds the option to explicitly choose whether a serialized transaction should be decoded as a witness or non-witness transaction rather than relying on the heuristic checks in #10481. The parameter defaults to relying on #10481 if not included, but it overrides that if included.
Tree-SHA512: d4846a5bb7d64dc19c516445488b00af329fc1f4181d9dfdf9f2382a086568edc98250a4ac7594e24a1bc231dfdee53c699b12c8380c355b920a67cc6770b7a9
c79d73d Clarify getbalance meaning a tiny bit in response to questions. (Matt Corallo)
Pull request description:
Someone was asking why getbalance "*" was more "correct" than getbalance, which should rarely be true...spendzeroconfchange was the issue.
Tree-SHA512: 90201cad1acec5161aee469fb4c6d737a0eb90f8380ac93abf0e41e0f02d120afcc3e2e873e5096d3655bb63bbd16fe99e72452f308d72e69139c7f6bb2d745e
99ba0c3 Don't use pass by reference to const for cheaply-copied types (bool, char, etc.). (practicalswift)
Pull request description:
Don't use pass by reference to const for cheaply-copied types (`bool`, `char`, etc.).
Tree-SHA512: ccad5e2695dff0b3d6de3e713ff3448f2981168cdac72d73bee10ad346b9919d8d4d588933369e54657a244b8b222fa0bef919bc56d983e1fa64b2004e51b225
03a5dc9 [wallet] Make CWallet::FundTransaction atomic (João Barbosa)
95d4450 [wallet] Tidy up CWallet::FundTransaction (João Barbosa)
Pull request description:
This PR fixes a race for `setLockedCoins` when `lockUnspents` is true. For instance, it should not be possible to use the same unspent in concurrent `fundrawtransaction` calls.
Now the `cs_main` and `cs_wallet` locks are held during `CreateTransaction` and `LockCoin`(s). Also added some style nits around the change.
Tree-SHA512: ccf383c0c5f6db775655a3e9ccd200c3bd831a83afae2b7c389564c74f7227f5bea86a4775727de2c3603b188f383f8a12d3f9d6d94f7887865c31c94ce95ef6
9c8eca7 Split up key and script metadata for better type safety (Russell Yanofsky)
Pull request description:
Suggested by @TheBlueMatt
https://github.com/bitcoin/bitcoin/pull/11403#discussion_r155599383
Combining the maps was probably never a good arrangement but is more
problematic now in presence of WitnessV0ScriptHash and WitnessV0KeyHash types.
Tree-SHA512: 9263e9c01090fb49221e91d88a88241a9691dda3e92d86041c8e284306a64d3af5e2438249f9dcc3e6e4a5c11c1a89f975a86d55690adf95bf2636f15f99f92a
a720b92 Remove includes in .cpp files for things the corresponding .h file already included (practicalswift)
Pull request description:
Remove includes in .cpp files for things the corresponding .h file already included.
Example case:
* `addrdb.cpp` includes `addrdb.h` and `fs.h`
* `addrdb.h` includes `fs.h`
Then remove the direct inclusion of `fs.h` in `addrman.cpp` and rely on the indirect inclusion of `fs.h` via the included `addrdb.h`.
In line with the header include guideline (see #10575).
Tree-SHA512: 8704b9de3011a4c234db336a39f7d2c139e741cf0f7aef08a5d3e05197e1e18286b863fdab25ae9638af4ff86b3d52e5cab9eed66bfa2476063aa5c79f9b0346
680bc2cbb Use range-based for loops (C++11) when looping over map elements (practicalswift)
Pull request description:
Before this commit:
```c++
for (std::map<T1, T2>::iterator x = y.begin(); x != y.end(); ++x) {
T1 z = (*x).first;
…
}
```
After this commit:
```c++
for (auto& x : y) {
T1 z = x.first;
…
}
```
Tree-SHA512: 954b136b7f5e6df09f39248a6b530fd9baa9ab59d7c2c7eb369fd4afbb591b7a52c92ee25f87f1745f47b41d6828b7abfd395b43daf84a55b4e6a3d45015e3a0
49bd659 tests: move pwalletMain to wallet test fixture (Wladimir J. van der Laan)
Pull request description:
Scope the variable instead of using an external global; this is how test fixtures are intended to be used.
Followup to #11713.
Tree-SHA512: 7d5bda93cdfe1329c8fe39bd72965906e36dad72fbb5d344ebedf26e66b1857510d01a3c2872d7f718fdeb23365e6ba71991aafe68e82781c6767a086b6d1590
c1e5d40 Make debugging test crash easier (MeshCollider)
8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider)
9587a9c Default walletdir is wallets/ if it exists (MeshCollider)
d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider)
80c5cbc Add test for -walletdir (MeshCollider)
0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider)
Pull request description:
Closes#11348
Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists.
Includes tests and release notes. Things which might need to be considered more:
- there is no 'lock' on the wallets directory, which might be needed?
- because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir)
- jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in https://github.com/bitcoin/bitcoin/pull/11687
- doc/files.md needs updating (will do soon)
I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. #3073), but decided it wasn't super relevant here will just complicate review.
Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d
28f8b66 Diagnose unsuitable outputs in lockunspent(). (Eelis)
Pull request description:
Fixes#2667.
This is a simplified version of pull request #3574, which was abandoned by its author.
I added some tests as well.
Tree-SHA512: e63e00dec8b1b232079380183805cb0b0b18c78ea6bea769837949aab984689d7f68b2ccfe66b1873517b040b9e616ce0eb058575c3d4382aa8c26eebcf1f14e
aed1d90ac [wallet] Change feebumper from class to functions (Russell Yanofsky)
37bdcca3c [refactor] Make feebumper namespace (Russell Yanofsky)
7c4f00919 [trivial] Rename feebumper variables according to project code style (Russell Yanofsky)
Pull request description:
Make feebumper methods static and remove stored state in the class.
Having the results of feebumper calls persist in an object makes process
separation between Qt and wallet awkward, because it means the feebumper object
either has to be serialized back and forth between Qt and wallet processes
between fee bump calls, or that the feebumper object needs to stay alive in the
wallet process with an object reference passed back to Qt. It's simpler just to
have fee bumper calls return their results immediately instead of storing them
in an object with an extended lifetime.
In addition to making feebumper methods static, also:
- Move LOCK calls from Qt code to feebumper
- Move TransactionCanBeBumped implementation from Qt code to feebumper
- Rename CFeeBumper class to FeeBumper (every CFeeBumper reference had to be
updated in this PR anyway so this doesn't increase the size of the diff)
This change was originally part of https://github.com/bitcoin/bitcoin/pull/10244
Tree-SHA512: bf75e0c741b4e9c8912e66cc1dedf0ff715f77ea65fc33f7020d97d9099b0f6448f5852236dac63eea649de7d6fc03b0b21492e2c5140fb7560a39cf085506fd
89f0312 Remove redundant pwallet nullptr check (Matt Corallo)
c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo)
3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo)
cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo)
e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo)
17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo)
5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo)
5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo)
0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo)
2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo)
0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo)
a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo)
Pull request description:
Based on #10179, this effectively reverts #9583, regaining most of the original speedups of #7946.
This concludes the work of #9725, #10178, and #10179.
See individual commit messages for more information.
Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9
Change feebumper from a stateful class into a namespace of stateless
functions.
Having the results of feebumper calls persist in an object makes process
separation between Qt and wallet awkward, because it means the feebumper object
either has to be serialized back and forth between Qt and wallet processes
between fee bump calls, or that the feebumper object needs to stay alive in the
wallet process with an object reference passed back to Qt. It's simpler just to
have fee bumper calls return their results immediately instead of storing them
in an object with an extended lifetime.
In addition to making feebumper stateless, also:
- Move LOCK calls from Qt code to feebumper
- Move TransactionCanBeBumped implementation from Qt code to feebumper
Future commit will remove the FeeBumper class. This commit simply places
everything into a feebumper namespace, and changes the enum class name
from BumpeFeeResult to feebumper::Result.
Future PRs will completely refactor this translation unit and touch all
this code so we rename the variables to follow project stlye guidelines
in this preparation commit.
Don't use m_ prefixes for member variables since we're going to remove
the class entirely in the next commits.
a357293 Use MakeUnique<Db>(...) (practicalswift)
3e09b39 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (practicalswift)
8617989 Add MakeUnique (substitute for C++14 std::make_unique) (practicalswift)
d223bc9 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (practicalswift)
b45c597 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift)
29ab96d Use unique_ptr for dbenv (DbEnv) (practicalswift)
f72cbf9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift)
8ccf1bb Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift)
73db063 Use unique_ptr for upnp_thread (boost::thread) (practicalswift)
0024531 Use unique_ptr for dbw (CDBWrapper) (practicalswift)
fa6d122 Use unique_ptr:s for {fee,short,long}Stats (TxConfirmStats) (practicalswift)
5a6f768 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift)
860e912 Use unique_ptr for pwalletMain (CWallet) (practicalswift)
Pull request description:
Use `std::unique_ptr` (C++11) where possible.
Rationale:
1. Avoid resource leaks (specifically: forgetting to `delete` an object created using `new`)
2. Avoid undefined behaviour (specifically: double `delete`:s)
**Note to reviewers:** Please let me know if I've missed any obvious `std::unique_ptr` candidates. Hopefully this PR should cover all the trivial cases.
Tree-SHA512: 9fbeb47b800ab8ff4e0be9f2a22ab63c23d5c613a0c6716d9183db8d22ddbbce592fb8384a8b7874bf7375c8161efb13ca2197ad6f24b75967148037f0f7b20c
dd9bb25 Fix code style in keystore.cpp/crypter.cpp (Jonas Schnelli)
208fda6 CCrypter: move relevant implementation out of the header (Jonas Schnelli)
3155fd2 CKeystore: move relevant implementation out of the header (Jonas Schnelli)
Pull request description:
Tree-SHA512: 4ce73cca5609199b74b8ff2614ee2b6af949545a1332a3a0135c6453c98665d2b0da171c1e390c9a2aec6b12b7fad931ec90084bb7c2defe243786bfc70daf60
6c4042a Assert that CWallet::SyncMetaData finds oldest transaction. (Eelis)
Pull request description:
Without this assert, the Clang static analyzer warns about subsequent dereferencing of copyFrom, because it can't be sure that it's not nullptr. See #9573.
Tree-SHA512: 83cbcb32c52c94fcfefbc90ec7de2011dacd6bdb0da35adc401b8d8dda6a86de2fa0403e2158592268c2cf15eef4f3d887d98c90f1031d4735d5f4bf9dbc1d23
5a5e4e9 [wallet] Remove CTransaction&() helper conversion operator from wallet implementation. (Karl-Johan Alm)
Pull request description:
The `CTransaction&()` operator in `CMerkleTx` makes conversion into `CTransaction`s transparent, but was marked as to-be-removed in favor of explicitly getting the `tx` ivar, presumably as the operator can lead to ambiguous behavior and makes the code harder to follow.
This PR removes the operator and adapts callers. This includes some cases of `static_cast<CTransaction>(wtx)` → `*wtx.tx`, which is definitely an improvement.
Tree-SHA512: 95856fec7194d6a79615ea1c322abfcd6bcedf6ffd0cfa89bbdd332ce13035fa52dd4b828d20df673072dde1be64b79c513529a6f422dd5f0961ce722a32d56a
7963335 Fix -disablewallet default value (João Barbosa)
b411c2a Improve -disablewallet parameter interaction (João Barbosa)
Pull request description:
The first commit logs a message for each configured wallet if `-disablewallet` is set:
```
bitcoind -printtoconsole -regtest -disablewallet -wallet=foo -wallet=bar
...
WalletParameterInteraction: parameter interaction: -disablewallet -> ignoring -wallet=foo
WalletParameterInteraction: parameter interaction: -disablewallet -> ignoring -wallet=bar
```
It also moves up the `-disablewallet` check which avoids the unnecessary `-wallet` soft set.
The second commit fixes the default value of `-disablewallet`, currently the value is correct, but it should use `DEFAULT_DISABLE_WALLET`.
The third commit can be dropped or squashed, just took the opportunity to fix the coding style there.
Tree-SHA512: bec13d2b2be5adf4680c77212020ed27dd05f15c4c73542d2005d91108bf704e2df1707ed2bec696e584ecd40eff7a63e25201fd70400222aa5a8da6aed6afeb
c098c58 Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider)
a38bfbc Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/11243
Adds "Requires a new wallet backup" text to `addwitnessaddress`, `importprivkey`, `importmulti`, `importaddress`, `importpubkey`, and `addmultisigaddress`. Also adds a warning to `dumpwallet` that backing up the seed alone is not sufficient to back up non-HD addresses
Tree-SHA512: 76d7cdca54d5b458acf479154620322391b889922525fddd6153f4164cfee393ad743757400cb8f6b1b30f24947df68ea9043b4e509f7df77a8fa05dda370933
720d9e8fa [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli)
Pull request description:
We do currently show/hide the wallet encryption RPC calls from the help if the current wallet.
In case of an encrypted wallet, `encryptwallet` is hidden and `walletpassphrasechange`, `walletpassphrasechange` and `walletlock` do appear in the help.
This is no longer ideal in case of multiwallet due to the fact that one may want help infos in order to target a specific wallet.
IMO its preferable to have a static help screen (show everything always). The currently show/hidden calls do handle the possible invalid encryption-state fine.
Fixes#11588
Tree-SHA512: 513fecd15248a31361f5143685e8cdeb63dfd3fa7120828917e1db54d936dc3db60d48ce46efa5c3a563a48157fe962689879856eeeed53f904686b12aec204e
5d465e396 Ensure backupwallet fails when attempting to backup to source file (Tomas van der Wansem)
Pull request description:
Previous behaviour was to destroy the wallet (to zero-length)
This fixes#11375
Tree-SHA512: bfd1738659b15e3f23b6bbdf55ec12269c62c820bf701daec19500b52bd5845bb5516733c6f76f36197eb155182a8a35dc239ad4de2ef1e59bbb0f124a455759
Change suggested by Cory Fields <cory-nospam-@coryfields.com> who noticed
listsinceblock would ignore invalid block hashes causing it to return a
completely unfiltered list of transactions.
Make sure wallet databases have unique fileids. If they don't, throw an error.
BDB caches do not work properly when more than one open database has the same
fileid, because values written to one database may show up in reads to other
databases.
Bitcoin will never create different databases with the same fileid, but users
can create them by manually copying database files.
BDB caching bug was reported by Chris Moore <dooglus@gmail.com>
https://github.com/bitcoin/bitcoin/issues/11429Fixes#11429
0aacfa4 Remove accidental stray semicolon (practicalswift)
68feb49 Use nullptr instead of NULL (practicalswift)
c6b07fd Fix a vs. an typo (practicalswift)
Pull request description:
Minor cleanups:
* Typo: Fix a vs. an typo
* Typo: Remove accidental stray semicolon (only remaining instance in repo)
* Correctness/consistency: Use `nullptr` instead of `NULL` (only remaining instance in repo)
Tree-SHA512: 47142e557da9d3fa0b532c46edeb7f356a1f6dc5973e60b0e496badff3581ff696eade542d49da777ac7f2e895129cc8487ccdb1984ff828434fa86f9a56dad0
7104de8 [wallet] Fix leak in CDB constructor (João Barbosa)
Pull request description:
First commit fixes a minor leak.
Second commit improves the constructor in the failure cases.
Tree-SHA512: 5165413d60ed9fc28203c9fe128adbba03a9ea9e9aa3734d9ea2522dafd815ba0fb8b90fd0809dbc06eb3ad360e7764de01dadf653ade3350fe86f6b8f04bc90
Now using a std::unique_ptr, the Db instance is correctly released
when CDB initialization fails.
The internal CDB state and mapFileUseCount are only mutated when
the CDB initialization succeeds.
This avoid calling out to mempool state during coin selection,
balance calculation, etc. In the next commit we ensure all wallet
callbacks from CValidationInterface happen in the same queue,
serialized with each other. This helps to avoid re-introducing one
of the issues described in #9584 [1] by further disconnecting
wallet from current chain/mempool state.
Thanks to @morcos for the suggestion to do this.
Note that there are several race conditions introduced here:
* If a user calls sendrawtransaction from RPC, adding a
transaction which is "trusted" (ie from them) and pays them
change, it may not be immediately used by coin selection until
the notification callbacks finish running. No such race is
introduced in normal transaction-sending RPCs as this case is
explicitly handled.
* Until Block{Connected,Disconnected} and
TransactionAddedToMempool calls also run in the CSceduler
background thread, there is a race where
TransactionAddedToMempool might be called after a
Block{Connected,Disconnected} call happens.
* Wallet will write a new best chain from the SetBestChain
callback prior to having processed the transaction from that
block.
[1] "you could go to select coins, need to use 0-conf change, but
such 0-conf change may have been included in a block who's
callbacks have not yet been processed - resulting in thinking they
are not in mempool and, thus, not selectable."
This prevents the wallet-RPCs-return-stale-info issue from being
re-introduced when new-block callbacks no longer happen in the
block-connection cs_main lock
7a91ceb5e [QA] Add RPC based rescan test (Jonas Schnelli)
c77170fbd [Wallet] add rescanblockchain <start_height> <stop_height> RPC command (Jonas Schnelli)
Pull request description:
A RPC rescan command is much more flexible for the following reasons:
* You can define the start and end-height
* It can be called during runtime
* It can work in multiwallet environment
Tree-SHA512: df67177bad6ad1d08e5a621f095564524fa3eb87204c2048ef7265e77013e4b1b29f991708f807002329a507a254f35e79a4ed28a2d18d4b3da7a75d57ce0ea5
55509f1 Document assumptions that are being made to avoid division by zero (practicalswift)
Pull request description:
Document assumptions (via `assert(…)`:s) that are being made to avoid division by zero.
Rationale:
* Make it clear to human reviewers and non-human static analyzers that what might look like potential division by zero cases are written the way they are intentionally (these cases are currently flagged by various static analyzers).
Tree-SHA512: bbb67b1370afd8f39bda35f9e3a20f4325f017d94cc1bfac3b0d36c9f34c2d95a9efe11efe44db29fb4aadd25d8276d8f0e03c8806ac64f0d21d821912e13b8e
Fixes#11462. Updated documentation for importprivkey function to use the correct name for the first argument.
Also updates a call to importprivkey to use named args in functional test.
c626dcb50 Make fUseCrypto atomic (MeshCollider)
731065b11 Consistent parameter names in txdb.h (MeshCollider)
35aeabec6 Make fReindex atomic to avoid race (MeshCollider)
58d91af59 Fix race for mapBlockIndex in AppInitMain (MeshCollider)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/11106
Also makes fReindex atomic as suggested in @TheBlueMatt comment below, and makes fUseCrypto atomic as suggested in 10916
d291e7635b just renames the parameters in the txdb header file to make them consistent with those used in the cpp file, noticed it when looking for uses of fReindex
Tree-SHA512: b378aa7289fd505b76565cd4d48dcdc04ac5540283ea1c80442170b0f13cb6df771b1a94dd54b7fec3478a7b4668c224ec9d795f16937782724c5d020edd3a42
0cd9273 rpc: Prevent `dumpwallet` from overwriting files (Wladimir J. van der Laan)
Pull request description:
Prevent arbitrary files from being overwritten by `dumpwallet`. There have been reports that users have overwritten wallet files this way. It may also avoid other security issues.
Fixes#9934. Adds mention to release notes and adds a test.
Tree-SHA512: 268c98636d40924d793b55a685a0b419bafd834ad369edaec08227ebe26ed4470ddea73008d1c4beb10ea445db1b0bb8e3546ba8fc2d1a411ebd4a0de8ce9120
cffe85f Skip sys::system(...) call in case of empty command (practicalswift)
6fb8f5f Check that -blocknotify command is non-empty before executing (practicalswift)
Pull request description:
Check that `-blocknotify` command is non-empty before executing.
To make the `BlockNotifyCallback(...)` (`-blocknotify`) behaviour consistent with that of:
* `AlertNotify(...)` (`-alertnotify`)
* `AddToWallet(...)` (`-walletnotify`)
Tree-SHA512: 18272166793a5a8b9cc2a727bfbcea53d38c329a55bc975c02db601329d608a61c20e026ce4b616193ecd3810dca4d3e2cb3bf773898a51872008a8dba96763e
1789e4675 Force explicit double -> int conversion for CFeeRate constructor (Matt Corallo)
53a6590f4 Make float <-> int casts explicit outside of test, qt, CFeeRate (Matt Corallo)
0b1b9148c Remove countMaskInv caching in bench framework (Matt Corallo)
Pull request description:
This fixes an issue where estimatesmartfee which matches at the min relay fee will return 999 sat/byte instead of 1000 sat/byte due to a float rounding issue. I went ahead and made all float <-> int conversion outside of test/qt explicit (test only had one or two more, Qt had quite a few, including many in the Qt headers themselves) and added overloads to CFeeRate to force callers to do an explicit round themselves. Easy to test with -Wfloat-conversion.
Tree-SHA512: 66087b08e5dfca67506da54ae057c2f9d86184415e8fa4fa0199e38839e06a3ce96c836fcb7593b7d960065f5240c594ff3a0cfa14333ac528421f5aeac835c9
bf64c3cb3 Ignore transactions added to mempool during a reorg for fee estimation purposes. (Alex Morcos)
04f78ab5b Do not reject based on mempool min fee when bypass_limits is set. (Alex Morcos)
fd849e1b0 Change AcceptToMemoryPool function signature (Alex Morcos)
Pull request description:
First commit just removes default arguments from `AcceptToMemoryPool` and consolidates two arguments, it does not change behavior.
Second commit finally fixes the fact that we're not meant to reject based on mempool min fee when adding a transaction from a disconnected block during a reorg as mentioned [here](https://github.com/bitcoin/bitcoin/pull/9602#issue-202197849)
Third commit makes fee estimation ignore transactions added from a disconnected block during a reorg. I think this was another source of fee estimates returning estimates below 1000 sat/kB as in #11303.
Tree-SHA512: 30925ca8b341915bb214f1d2590b36b7931f2e125b7660150e38ae70338f00db5aa7f1608546dddb181446924177eb7cf62ea8bd2583068acc074d6c3f86bc0c
8213838 [Qt] tolerate BIP173/bech32 addresses during input validation (Jonas Schnelli)
06eaca6 [RPC] Wallet: test importing of native witness scripts (NicolasDorier)
fd0041a Use BIP173 addresses in segwit.py test (Pieter Wuille)
e278f12 Support BIP173 in addwitnessaddress (Pieter Wuille)
c091b99 Implement BIP173 addresses and tests (Pieter Wuille)
bd355b8 Add regtest testing to base58_tests (Pieter Wuille)
6565c55 Convert base58_tests from type/payload to scriptPubKey comparison (Pieter Wuille)
8fd2267 Import Bech32 C++ reference code & tests (Pieter Wuille)
1e46ebd Implement {Encode,Decode}Destination without CBitcoinAddress (Pieter Wuille)
Pull request description:
Builds on top of #11117.
This adds support for:
* Creating BIP173 addresses for testing (through `addwitnessaddress`, though by default it still produces P2SH versions)
* Sending to BIP173 addresses (including non-v0 ones)
* Analysing BIP173 addresses (through `validateaddress`)
It includes a reformatted version of the [C++ Bech32 reference code](https://github.com/sipa/bech32/tree/master/ref/c%2B%2B) and an independent implementation of the address encoding/decoding logic (integrated with CTxDestination). All BIP173 test vectors are included.
Not included (and intended for other PRs):
* Full wallet support for SegWit (which would include automatically adding witness scripts to the wallet during automatic keypool topup, SegWit change outputs, ...) [see #11403]
* Splitting base58.cpp and tests/base58_tests.cpp up into base58-specific code, and "address encoding"-code [see #11372]
* Error locating in UI for BIP173 addresses.
Tree-SHA512: 238031185fd07f3ac873c586043970cc2db91bf7735c3c168cb33a3db39a7bda81d4891b649685bb17ef90dc63af0328e7705d8cd3e8dafd6c4d3c08fb230341
603efe9fc Fix parameter name typo in ErasePurpose walletdb method. (Pierre Rochard)
Pull request description:
The header file has the correct method signature and the one usage in CWallet::DelAddressBook is correctly passing in EncodeDestination(address)
Tree-SHA512: ee0808a74111fd23a1c47ba5ab51de151fdd33a01d92895671e562ac184cbcb33180a3ff26c22e5717595592097b9fa33deca9878d89ce8d34687f09cfadfcf0
d01a968 wallet: update stored witness in AddToWallet (Suhas Daftuar)
Pull request description:
Replace witness-stripped wallet transactions with full transactions;
this can happen when upgrading from a pre-segwit wallet to a segwit-
aware wallet.
Tree-SHA512: a348b16b38ae738fa75cf7d3ff50ebd0d0071d5d6061c9a10dc3325fc34f6bc96a67aea21fde460ca20f6178768ee0af04d6d8785b35647f436a9083c4270b07
Prevent arbitrary files from being overwritten. There have been reports
that users have overwritten wallet files this way. It may also avoid
other security issues.
Fixes#9934. Adds mention to release notes and adds a test.
A C-style cast is equivalent to try casting in the following order:
1. const_cast(...)
2. static_cast(...)
3. const_cast(static_cast(...))
4. reinterpret_cast(...)
5. const_cast(reinterpret_cast(...))
By using static_cast<T>(...) explicitly we avoid the possibility
of an unintentional and dangerous reinterpret_cast. Furthermore
static_cast<T>(...) allows for easier grepping of casts.
2a07f878a Refactor: Modernize disallowed copy constructors/assignment (Dan Raviv)
Pull request description:
Use C++11's better capability of expressing an interface of a non-copyable class by publicly deleting its copy ctor and assignment operator instead of just declaring them private.
Tree-SHA512: 878f446be5a136bb2a90643aaeaca62948b575e6ef71ccc5b4b8f373e66f36ced00665128f36504e0ccfee639863d969329c4276154ef9f2a9de9137f0801e01
fdc3293 Document assumptions that are being made to avoid NULL pointer dereferences (practicalswift)
Pull request description:
Document assumptions (via `assert(…)`:s) that are being made avoid `NULL` pointer dereferences.
Rationale:
* Make it clear to human reviewers and non-human static analyzers that what might look like potential `NULL` pointer dereferences are written the way they are intentionally (these cases are currently flagged by various static analyzers).
Tree-SHA512: b424328195e2680e1e4ec546298f718c49e5ad182147dc004de580693db1b50eec4065e1c4f232bdb302baa12954265a50ba21cb5ba4ff30248535b2de778672
Use C++11's better capability of expressing an interface of a non-copyable class by publicly deleting its copy ctor and assignment operator instead of just declaring them private.
Combine fLimitFree and fOverrideMempoolLimit into a single boolean:
bypass_limits. This is used to indicate that mempool limiting based on feerate
should be bypassed. It is used when readding transactions from a reorg and then
the mempool is trimmed to size after all transactions are added and they can be
evaluated in the context of their descendants. No changes to behavior.
713a92073 Remove usehd option and warn when it is used (Andrew Chow)
d4c18f733 Bump wallet version number to 159900 (Andrew Chow)
Pull request description:
Bump the wallet version number to 159900 so that new wallets made without a default key will no longer work on previous versions at all. Also remove the `usehd` option to avoid weird interaction with wallet version numbers and HD-ness of wallets.
Tree-SHA512: dd7965505bfad6a926c79afd423236f509229a398a8398076f8d57d90a5974243f9459a61225c4daee560c796f427445c9e55a3ad528a3a97a9123ca6a1269ab
5d2a3995e [trivial] fixup comment for VerifyWallets() (John Newbery)
43b0e81d0 [wallet] Add StartWallets() function to wallet/init.cpp (John Newbery)
290f3c56d [wallet] Add RegisterWalletRPC() function to wallet/init.cpp (John Newbery)
062d63102 [wallet] Add CloseWallets() function to wallet/init.cpp (John Newbery)
77fe07c15 [wallet] Add StopWallets() function to wallet/init.cpp (John Newbery)
2da5eafa4 [wallet] Add FlushWallets() function to wallet/init.cpp (John Newbery)
1b9cee66e [wallet] Rename WalletVerify() to VerifyWallets() (John Newbery)
9c76ba18c [wallet] Rename InitLoadWallet() to OpenWallets() (John Newbery)
Pull request description:
Apologies for the mostly code move only PR. This is a pre-req for both #10740 and #10762
All wallet component initialization/destruction functions are now in their own `wallet/init.cpp` translation unit and are no longer static functions on the CWallet class. The bitcoin_server also no longer has any knowledge that there are multiple wallets in vpwallet.
There should be no changes in behavior from this PR.
Tree-SHA512: 7c260eb094f2fa1a88d803769ba60935810968a7309f731135e4b17623b97f18c03bbcd293c942093d1efce62c6c978f9ff484d54dc9a60bc2fcb5af2d160fcd
Rationale:
- this init function can now open multiple wallets (hence
Wallet->Wallets)
- This is named as the antonym to CloseWallets(), which carries out the
opposite action.
592404f03 Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider)
Pull request description:
This just continues the work of https://github.com/bitcoin/bitcoin/pull/9804
Modifies a lot of `&vector[]`'s to `vector.data()`'s across all the files including tests, just the stuff that 9804 missed
Tree-SHA512: dd1a9dffb999dea4fba78dcc91fe02f90250db86f5c74948e1ff3e8b4036b2154b600555eaa04dece5368920aae3513bc36425dc96e4319ca1041b0928a6b656
fe09b0197 add missing lock to crypter GetKeys() (Marko Bencun)
5cb3da04b keystore GetKeys(): return result instead of writing to reference (Marko Bencun)
Pull request description:
Issue: #10905
First commit makes GetKeys() return the result instead of writing to a reference to remove some useless lines.
Tree-SHA512: bb51255b5a6cf5488c3d5dee89f539d41f0717f018441d120047f877e0a705a133fb3b7a97d1cf8f73b5d2ed93dd2dbdfcd6f394e40105af2a12e01d397cb402
864cd2787 Move CBitcoinAddress to base58.cpp (Pieter Wuille)
5c8ff0d44 Introduce wrappers around CBitcoinAddress (Pieter Wuille)
Pull request description:
This patch removes the need for the intermediary Base58 type `CBitcoinAddress`, by providing {`Encode`,`Decode`,`IsValid`}`Destination` functions that directly operate on the conversion between `std::string`s and `CTxDestination`.
As a side, it also fixes a number of indentation issues, and removes probably several unnecessary implicit `CTxDestination`<->`CBitcoinAddress` conversions.
This change is far from complete. In follow-ups I'd like to:
* Split off the specific address and key encoding logic from base58.h, and move it to a address.h or so.
* Replace `CTxDestination` with a non-`boost::variant` version (which can be more efficient as `boost::variant` allocates everything on the heap, and remove the need for `boost::get<...>` and `IsValidDestination` calls everywhere).
* Do the same for `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey`.
However, I've tried to keep this patch to be minimally invasive, but still enough to support non-Base58 addresses. Perhaps a smaller patch is possible to hack Bech32 support into `CBitcoinAddress`, but I would consider that a move in the wrong direction.
Tree-SHA512: c2c77ffb57caeadf2429b1c2562ce60e8c7be8aa9f8e51b591f354b6b441162625b2efe14c023a1ae485cf2ed417263afa35c892891dfaa7844e7fbabccab85e
This patch removes the need for the intermediary Base58 type
CBitcoinAddress, by providing {Encode,Decode,IsValid}Destination
function that directly operate on the conversion between strings
and CTxDestination.
47ba2c312 Fix currency/fee-rate unit string in the help text (Akio Nakamura)
Pull request description:
1. The RPC help text should use the constant `CURRENCY_UNIT` defined in `policy/feerate.cpp` instead of the literal `'BTC'`.
In the following 2 RPC commands, `'BTC'` is written directly in the help text.
This commit changes them to use that constant.
1) `estimatesmartfee`
2) `estimaterawfee`
2. Some RPC command use `'satoshis'` as the unit.
It should be written as `'satoshis'` instead of `'Satoshis'` in the RPC help text.
So, this commit fixes this typo in `getblocktemplate`.
Tree-SHA512: d0bd1cd90560e59bf456b076b958a2a1c998f85a7e65aeb6b2abcaba18919a3ae62f7c3909210461084c1a3275a35b6ba3ea3ec8f5cce33702ffe383c9e84bce
1. The RPC help text should use the constant CURRENCY_UNIT defined in
policy/feerate.cpp instead of the literal 'BTC'. In the following
2 RPC commands, 'BTC' is written directly in the help text.
1) estimatesmartfee
2) estimaterawfee
And also, for these help strings, the notation
'fee-per-kilobyte (in BTC)' is somewhat ambiguous.
To write more precisely, this commit changes to 'fee rate in BTC/kB'
with using the constant CURRENCY_UNIT.
2. Some RPC command use 'satoshis' as the unit. It should be written
as 'satoshis' instead of 'Satoshis' in the RPC help text.
So, this commit fixes this typo in getblocktemplate.
3. The phrase that '... feerate (BTC per KB) ...' is used to explain
the fee rate in the help text of following 2 RPC commands.
1) getmempoolinfo
2) fundrawtransaction
But they are different from other similar help text of the RPCs.
And also, 'KB' implies Kibibyte (2^10 byte).
To unify and to clarify, this commit changes these phrase to
'... fee rate in BTC/kB ...'.
(BTC references the constant 'CURRENCY_UNIT')
Replace witness-stripped wallet transactions with full transactions;
this can happen when upgrading from a pre-segwit wallet to a segwit-
aware wallet.
This contains most of the changes of 10563 "remove safe mode", but doesn't
remove the safe mode yet, but put an `ObserveSafeMode()` check in
individual calls with okSafeMode=false.
This cleans up the ugly "okSafeMode" flag from the dispatch tables,
which is not a concern for the RPC server.
Extra-author: Wladimir J. van der Laan <laanwj@gmail.com>
de9a1db Acquire cs_main lock before cs_wallet during wallet initialization (Russell Yanofsky)
Pull request description:
`CWallet::MarkConflicted` may acquire the `cs_main` lock after `CWalletDB::LoadWallet` acquires the `cs_wallet` lock during wallet initialization. (`CWalletDB::LoadWallet` calls `ReadKeyValue` which calls `CWallet::LoadToWallet` which calls `CWallet::MarkConflicted`). This is the opposite order that `cs_main` and `cs_wallet` locks are acquired in the rest of the code, and so leads to `POTENTIAL DEADLOCK DETECTED` errors if bitcoin is built with `-DDEBUG_LOCKORDER`.
This commit changes `CWallet::LoadWallet` (which calls `CWalletDB::LoadWallet`) to acquire both locks in the standard order.
Error was reported by @luke-jr in https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/
Tree-SHA512: 353fe21bc0a4a2828b41876897001a3c414d4b115ee7430925bd391d8bc396fca81661145d00996c1ba1a01516d9acf8b89fb5c3da27092f5f3aa7e37ef26ffa
6af49dd Output a bit more information for fee calculation report. (Alex Morcos)
a54c7b9 Fix rounding errors in calculation of minimum change size (Alex Morcos)
Pull request description:
Thanks to @juscamarena for reporting this.
Please backport to 0.15.
There was a potential rounding error where the fee for the change added to the fee for the original tx could be less than the fee for the tx including change.
This is fixed in the first commit. The second commit adds one more snippet of information in the fee calculation report. I actually realized that there is more information that would be nice to report, but we can add that post 0.15.
An open question is whether we should be returning failure if the test in line 2885 is hit or just resetting pick_new_inputs and continuing. Originally I made it a failure to avoid any possible infinite loops. But the case hit here is an example of where that logic possibly backfired.
Tree-SHA512: efe049781acc1f6a8ad429a689359ac6f7b7c44cdfc9578a866dff4a2f6596e8de474a89d25c704f31ef4f8c89af770e98b75ef06c25419d5a6dfc87247bf274
f01103c MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (Russell Yanofsky)
e7fe320 MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (Russell Yanofsky)
d97fe20 Move some static functions out of wallet.h/cpp (Russell Yanofsky)
Pull request description:
This just moves some static wallet fee and init functions out of `wallet/wallet.cpp` and into new `wallet/fees.cpp` and `wallet/init.cpp` source files. There is one commit updating declarations and callers, followed by two MOVEONLY commits actually moving the function bodies.
This change is desirable because wallet.h/cpp are monolithic and hard to navigate, so pulling things out and grouping together pieces of related functionality should improve the organization.
Another motivation is the wallet process separation work in https://github.com/bitcoin/bitcoin/pull/10973, where (at least initially) parameter parsing and fee estimation are still done in the main process rather than the wallet process, and having functions that run in different processes scrambled up throughout wallet.cpp is unnecessarily confusing.
Tree-SHA512: 6e6982ff82b2ab4e681c043907e2b1801ceb9513394730070f16c46ad338278a863f5b3759aa13db76a259b268b1c919c81f4e339f0796a3cfb990161e8c316d
CWallet::MarkConflicted may acquire the cs_main lock after
CWalletDB::LoadWallet acquires the cs_wallet lock during wallet initialization.
(CWalletDB::LoadWallet calls ReadKeyValue which calls CWallet::LoadToWallet
which calls CWallet::MarkConflicted). This is the opposite order that cs_main
and cs_wallet locks are acquired in the rest of the code, and so leads to
POTENTIAL DEADLOCK DETECTED errors if bitcoin is built with -DDEBUG_LOCKORDER.
This commit changes CWallet::LoadWallet (which calls CWalletDB::LoadWallet) to
acquire both locks in the standard order. It also fixes some tests that were
acquiring wallet and main locks out of order and failed with the new locking in
CWallet::LoadWallet.
Error was reported by Luke Dashjr <luke-jr@utopios.org> in
https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/
a897d0e tests: Remove OldSetKeyFromPassphrase/OldEncrypt/OldDecrypt (practicalswift)
Pull request description:
Reduces the number of non-free:d allocs with four (Δ in use at exit = -928 bytes).
With this patch applied:
```
$ valgrind --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto
…
==20243== HEAP SUMMARY:
==20243== in use at exit: 72,704 bytes in 1 blocks
==20243== total heap usage: 53,138 allocs, 53,137 frees, 49,600,420 bytes allocated
==20243==
==20243== 72,704 bytes in 1 blocks are still reachable in loss record 1 of 1
==20243== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==20243== by 0x6AA5EFF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==20243== by 0x40106B9: call_init.part.0 (dl-init.c:72)
==20243== by 0x40107CA: call_init (dl-init.c:30)
==20243== by 0x40107CA: _dl_init (dl-init.c:120)
==20243== by 0x4000C69: ??? (in /lib/x86_64-linux-gnu/ld-2.23.so)
==20243== by 0x2: ???
==20243== by 0xFFF0006A2: ???
==20243== by 0xFFF0006B8: ???
==20243== by 0xFFF0006CF: ???
==20243==
==20243== LEAK SUMMARY:
==20243== definitely lost: 0 bytes in 0 blocks
==20243== indirectly lost: 0 bytes in 0 blocks
==20243== possibly lost: 0 bytes in 0 blocks
==20243== still reachable: 72,704 bytes in 1 blocks
==20243== suppressed: 0 bytes in 0 blocks
```
Without this patch applied:
```
$ valgrind --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto
…
==19023== HEAP SUMMARY:
==19023== in use at exit: 73,632 bytes in 5 blocks
==19023== total heap usage: 52,718 allocs, 52,713 frees, 49,502,962 bytes allocated
==19023==
==19023== 24 bytes in 1 blocks are still reachable in loss record 1 of 5
==19023== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19023== by 0x642DE77: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E5665: lh_insert (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E7BB3: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E87AD: ERR_get_state (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E883D: ERR_put_error (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64EAAE4: EVP_DecryptFinal_ex (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x3AD150: wallet_crypto::OldDecrypt(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, secure_allocator<unsigned char> >&, unsigned char const*, unsigned char const*) (crypto_tests.cpp:81)
==19023== by 0x3AF892: wallet_crypto::TestCrypter::TestDecrypt(CCrypter const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) (crypto_tests.cpp:137)
==19023== by 0x3AD5E9: wallet_crypto::decrypt::test_method() (crypto_tests.cpp:223)
==19023== by 0x3ADC11: wallet_crypto::decrypt_invoker() (crypto_tests.cpp:216)
==19023== by 0x182596: invoke<void (*)()> (callback.hpp:56)
==19023== by 0x182596: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
==19023==
==19023== 128 bytes in 1 blocks are still reachable in loss record 2 of 5
==19023== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19023== by 0x642DE77: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E5331: lh_new (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E7862: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E7B7F: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E87AD: ERR_get_state (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E883D: ERR_put_error (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64EAAE4: EVP_DecryptFinal_ex (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x3AD150: wallet_crypto::OldDecrypt(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, secure_allocator<unsigned char> >&, unsigned char const*, unsigned char const*) (crypto_tests.cpp:81)
==19023== by 0x3AF892: wallet_crypto::TestCrypter::TestDecrypt(CCrypter const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) (crypto_tests.cpp:137)
==19023== by 0x3AD5E9: wallet_crypto::decrypt::test_method() (crypto_tests.cpp:223)
==19023== by 0x3ADC11: wallet_crypto::decrypt_invoker() (crypto_tests.cpp:216)
==19023==
==19023== 176 bytes in 1 blocks are still reachable in loss record 3 of 5
==19023== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19023== by 0x642DE77: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E530F: lh_new (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E7862: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E7B7F: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E87AD: ERR_get_state (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E883D: ERR_put_error (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64EAAE4: EVP_DecryptFinal_ex (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x3AD150: wallet_crypto::OldDecrypt(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, secure_allocator<unsigned char> >&, unsigned char const*, unsigned char const*) (crypto_tests.cpp:81)
==19023== by 0x3AF892: wallet_crypto::TestCrypter::TestDecrypt(CCrypter const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) (crypto_tests.cpp:137)
==19023== by 0x3AD5E9: wallet_crypto::decrypt::test_method() (crypto_tests.cpp:223)
==19023== by 0x3ADC11: wallet_crypto::decrypt_invoker() (crypto_tests.cpp:216)
==19023==
==19023== 600 bytes in 1 blocks are still reachable in loss record 4 of 5
==19023== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19023== by 0x642DE77: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E8745: ERR_get_state (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64E883D: ERR_put_error (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x64EAAE4: EVP_DecryptFinal_ex (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==19023== by 0x3AD150: wallet_crypto::OldDecrypt(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, secure_allocator<unsigned char> >&, unsigned char const*, unsigned char const*) (crypto_tests.cpp:81)
==19023== by 0x3AF892: wallet_crypto::TestCrypter::TestDecrypt(CCrypter const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) (crypto_tests.cpp:137)
==19023== by 0x3AD5E9: wallet_crypto::decrypt::test_method() (crypto_tests.cpp:223)
==19023== by 0x3ADC11: wallet_crypto::decrypt_invoker() (crypto_tests.cpp:216)
==19023== by 0x182596: invoke<void (*)()> (callback.hpp:56)
==19023== by 0x182596: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
==19023== by 0x596CCB0: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==19023== by 0x594C995: boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==19023==
==19023== 72,704 bytes in 1 blocks are still reachable in loss record 5 of 5
==19023== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19023== by 0x6AA5EFF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==19023== by 0x40106B9: call_init.part.0 (dl-init.c:72)
==19023== by 0x40107CA: call_init (dl-init.c:30)
==19023== by 0x40107CA: _dl_init (dl-init.c:120)
==19023== by 0x4000C69: ??? (in /lib/x86_64-linux-gnu/ld-2.23.so)
==19023== by 0x2: ???
==19023== by 0xFFF0006A2: ???
==19023== by 0xFFF0006B8: ???
==19023== by 0xFFF0006CF: ???
==19023==
==19023== LEAK SUMMARY:
==19023== definitely lost: 0 bytes in 0 blocks
==19023== indirectly lost: 0 bytes in 0 blocks
==19023== possibly lost: 0 bytes in 0 blocks
==19023== still reachable: 73,632 bytes in 5 blocks
==19023== suppressed: 0 bytes in 0 blocks
==19023==
==19023== For counts of detected and suppressed errors, rerun with: -v
==19023== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
```
Tree-SHA512: 38b6552736a5710a42dbad770c490583cfc762acbec716f5db4cf38314f494ea99430713ea407c73b49d867676ced221a282437f3fcfd8346f8f68386f4fc74d
745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky)
fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky)
e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky)
e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky)
Pull request description:
This is a followup to #10783.
- The first commit doesn't change behavior at all, just simplifies code.
- The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors.
- The third commit updates developer notes after the cleanup.
- The forth commit does some additional code cleanup in `getbalance`.
Followup changes that should happen in future PRs:
- [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133850525
- [ ] Add braces around if statements. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133851133
- [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133829303
Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9
40a0f9f Enable devirtualization opportunities by using the final specifier (C++11) (practicalswift)
9a1675e optim: mark a few classes final (Cory Fields)
Pull request description:
Using gcc's ```-Wsuggest-final-types``` and lto, I identified a few easy devirtualization wins:
> wallet/wallet.h:651:7: warning: Declaring type 'struct CWallet' final would enable devirtualization of 26 calls [-Wsuggest-final-types]
>coins.h:201:7: warning: Declaring type 'struct CCoinsViewCache' final would enable devirtualization of 13 calls [-Wsuggest-final-types]
>txdb.h:67:7: warning: Declaring type 'struct CCoinsViewDB' final would enable devirtualization of 5 calls [-Wsuggest-final-types]
>zmq/zmqnotificationinterface.h:16:7: warning: Declaring type 'struct CZMQNotificationInterface' final would enable devirtualization of 4 calls [-Wsuggest-final-types]
>httpserver.cpp:42:7: warning: Declaring type 'struct HTTPWorkItem' final would enable devirtualization of 2 calls [-Wsuggest-final-types]
Tree-SHA512: 2a825fd27121ccabaacff5cde2fc8a50d1b4cc846374606caa2a71b0cd8fcb0d3c9b5b3fd342d944998610e2168048601278f8a3709cc515191a0bb2d98ba782
e53615b Remove vchDefaultKey and have better first run detection (Andrew Chow)
Pull request description:
Removes vchDefaultKey which was only used for first run detection. Improves wallet first run detection by checking to see if any keys were read from the database.
This also fixes a (rather contrived) case where an encrypted non-HD wallet has corruption such that the default key is no longer valid and is loaded into a Core version that supports HD wallets. This causes a runtime exception since a new hd master key is generated as the software believes the wallet file is newly created but cannot add the generated key to the wallet since it is encrypted. I was only able to replicate this error by creating a non-hd wallet, encrypting it, then editing the wallet using `db_dump` and `db_load` before loading the wallet with hd enabled. This problem has been reported by [two](https://bitcointalk.org/index.php?topic=1993244.0) [users](https://bitcointalk.org/index.php?topic=1746976.msg17511261#msg17511261) so it is something that can happen, although that raises the question of "what corrupted the default key".
~P.S. I don't know what's up with the whitespace changes. I think my text editor is doing something stupid but I don't think those are important enough to attempt undoing them.~ Undid those
Tree-SHA512: 63b485f356566e8ffa033ad9b7101f7f6b56372b29ec2a43b947b0eeb1ada4c2cfe24740515d013aedd5f51aa1890dfbe499d2c5c062fc1b5d272324728a7d55
67ceff4 [wallet] Add logging to MarkReserveKeysAsUsed (John Newbery)
1221f60 [wallet] Remove keypool_topup_cleanups (John Newbery)
Pull request description:
A couple of minor cleanups suggested by @ryanofsky here: https://github.com/bitcoin/bitcoin/pull/11022#pullrequestreview-55598940
Does not affect functionality. Not required for v0.15.
Tree-SHA512: d8d0698fd26ea49a4157e68669d5511095760c3a1ecfa3f917e3f273efbafb55c51a202d677614216eae3f796b6e8d17506b2ec2d4799a94f18981b396e65eec
8f2f1e0 wallet: Avoid second mapWallet lookup (João Barbosa)
Pull request description:
All calls to `mapWallet.count()` have the intent to detect if a `txid` exists and most are followed by a second lookup to retrieve the `CWalletTx`.
This PR replaces all `mapWallet.count()` calls with `mapWallet.find()` to avoid the second lookup.
Tree-SHA512: 96b7de7f5520ebf789a1aec1949a4e9c74e13683869cee012f717e5be8e51097d068e2347a36e89097c9a89f1ed1a1529db71760dac9b572e36a3e9ac1155f29
6e8c48dc5 Add const to methods that do not modify the object for which it is called (practicalswift)
Pull request description:
Tree-SHA512: a6888111ba16fb796e320e60806e1a77d36f545989b5405dc7319992291800109eab0b8e8c286b784778f41f1ff5289e7cb6b4afd7aec77f385fbcafc02cffc1
Removes vchDefaultKey which was only used for first run detection.
Improves wallet first run detection by checking to see if any keys
were read from the database.
This will now also check for a valid defaultkey for backwards
compatibility reasons and to check for any corruption.
Keys will stil be generated on the first one, but there won't be
any shown in the address book as was previously done.
Only change in behavior is that unsupported combinations of parameters now
trigger more specific error messages instead of the vague "JSON value is not a
string as expected" error.
03bc719a8 [wallet] Close DB on error. (Karl-Johan Alm)
Pull request description:
This PR intends to plug some leaks. It specifically implements adherence to the requirement in BDB to close a handle which failed to open (https://docs.oracle.com/cd/E17276_01/html/api_reference/C/dbopen.html):
> The `DB->open()` method returns a non-zero error value on failure and 0 on success. If `DB->open()` fails, the `DB->close()` method must be called to discard the DB handle.
Tree-SHA512: cc1f2b925ef3fd6de785f62108fbc79454443397f80707762acbc56757841d2c32b69c0234f87805571aa40c486da31f315ca4c607a2c7d1c97c82a01301e2a6
This changes RPC methods to treat null arguments the same as missing arguments,
instead of throwing type errors. Specifically:
- `getbalance` method now returns the wallet balance when the `account` param
is null instead of throwing a type error (same as when parameter is missing).
It is still an error to supply `minconf` or `watchonly` options when the
account is null.
- `addnode` and `setban` methods now return help text instead of type errors if
`command` params are null (same as when params are missing).
- `sendrawtransaction`, `setaccount`, `movecmd`, `sendfrom`,
`addmultisigaddress`, `listaccounts`, `lockunspent` methods accept null
default values where missing values were previously allowed, and treat them
the same.
This commit just moves a few function declarations and updates callers.
Function bodies are moved in two followup MOVEONLY commits.
This change is desirable because wallet.h/cpp are monolithic and hard to
navigate, so pulling things out and grouping together pieces of related
functionality should improve the organization.
Another proximate motivation is the wallet process separation work in
https://github.com/bitcoin/bitcoin/pull/10973, where (at least initially)
parameter parsing and fee estimation are still done in the main process rather
than the wallet process, and having functions that run in different processes
scrambled up throughout wallet.cpp is unnecessarily confusing.
90d4d89 scripted-diff: Use the C++11 keyword nullptr to denote the pointer literal instead of the macro NULL (practicalswift)
Pull request description:
Since C++11 the macro `NULL` may be:
* an integer literal with value zero, or
* a prvalue of type `std::nullptr_t`
By using the C++11 keyword `nullptr` we are guaranteed a prvalue of type `std::nullptr_t`.
For a more thorough discussion, see "A name for the null pointer: nullptr" (Sutter &
Stroustrup), http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2431.pdf
With this patch applied there are no `NULL` macro usages left in the repo:
```
$ git grep NULL -- "*.cpp" "*.h" | egrep -v '(/univalue/|/secp256k1/|/leveldb/|_NULL|NULLDUMMY|torcontrol.*NULL|NULL cert)' | wc -l
0
```
The road towards `nullptr` (C++11) is split into two PRs:
* `NULL` → `nullptr` is handled in PR #10483 (scripted, this PR)
* `0` → `nullptr` is handled in PR #10645 (manual)
Tree-SHA512: 3c395d66f2ad724a8e6fed74b93634de8bfc0c0eafac94e64e5194c939499fefd6e68f047de3083ad0b4eff37df9a8a3a76349aa17d55eabbd8e0412f140a297
d34957e [wallet] [tests] Add keypool topup functional test (Jonas Schnelli)
095142d [wallet] keypool mark-used and topup (John Newbery)
c25d90f [wallet] Add HasUnusedKeys() helper (John Newbery)
f2123e3 [wallet] Cache keyid -> keypool id mappings (John Newbery)
83f1ec3 [wallet] Don't hold cs_LastBlockFile while calling setBestChain (John Newbery)
2376bfc [wallet] [moveonly] Move LoadKeyPool to cpp (Matt Corallo)
cab8557 [wallet] [moveonly] Move CAffectedKeysVisitor (Jonas Schnelli)
Pull request description:
This PR contains the first part of #10882 :
- if a key from the keypool is used, mark all keys up to that key as used, and then try to top up the keypool
- top up the keypool on startup
Notably, it does not stop the node or prevent the best block from advancing if the keypool drops below a threshold (which means that transactions may be missed and funds lost if restoring from an old HD wallet backup).
Tree-SHA512: ac681fefeaf7ec2aab2fa1da93d12273ea80bd05eb48d7b3b551ea6e5d975dd97ba7de52b7fba52993823280ac4079cc36cf78a27dac708107ebf8fb6326142b
This commit adds basic keypool mark-used and topup:
- try to topup the keypool on initial load
- if a key in the keypool is used, mark all keys before that as used and
try to top up
49d903e Eliminate fee overpaying edge case when subtracting fee from recipients (Alex Morcos)
Pull request description:
I'm not sure if this is the cause of the issue in #10034 , but this was a known edge case. I just didn't realize how simple the fix is.
Could use a couple more eyes to make sure nothing silly can go wrong here, but if we all agree it's this simple, we can add this as another 0.15 bug fix.
Tree-SHA512: db1dd1e83363a3c231267b626d3a388893ee70ba1972056fe2c339c5c9e4fbfd30f7fe837c30cc7be884d454797fd4c619b9d631a8d5eeb55cdb07402a83acb3
e222dc2 Replace ismine with producesignature check in witnessifier (Andrew Chow)
Pull request description:
Instead of using ismine to check whether an address can be spent by us, make the witness version of the script or address first and then use ProduceSignature with the DummySignatureCreator to check if we can
solve for the script.
This is to fix cases where we don't have all of the private keys (for something like a multisig address) but have the redeemscript so we can witnessify it.
Tree-SHA512: 371777aee839cceb41f099109a13689120d35cf3880cde39216596cc2aac5cc1096af7d9cf07ad9306c3b05c073897f4518a7e97f0b88642f1e3b80b799f481e
Instead of using ismine to check whether an address can be spent by us,
make the witness version of the script or address first and then use
ProduceSignature with the DummySignatureCreator to check if we can
solve for the script.
Also fixes test cases to reflect this change.
df389bc Change wallet method disabled error text (Russell Yanofsky)
e526b3d Fix misleading "Method not found" multiwallet errors (Russell Yanofsky)
Pull request description:
Raise RPC_WALLET_NOT_SPECIFIED instead of RPC_METHOD_NOT_FOUND when a required
wallet filename was not specified in an RPC call.
Also raise more specific RPC_WALLET_NOT_FOUND error instead of
RPC_INVALID_PARAMETER in case an invalid wallet was specified, for consistency.
Tree-SHA512: 6a8d885283f69bcfc28f2e08ac03eff02f9f8160a312ce2a90d868aa52533434fc0b4c4ab86547c2f09392338956df915637eaf7136a4fc105e6c8179f2d0ac8
065039d [tests] Fix incorrect memory_cleanse(…) call in crypto_tests.cpp (practicalswift)
Pull request description:
`chKey` and `chIV` are pointers, not arrays :-)
Probably the result of copy-pasting of old code where the code was operating on arrays instead of pointers.
If I'm reading the code correctly the absence/presence of these `memory_cleanse(…)` calls won't alter the outcome of the test in question (`TestPassphraseSingle`) even if fixed. Therefore removing.
Tree-SHA512: a053b2817bedf6ef889744e546ce9a0f165dee94aef6850d9d6a6bb05b0018789597371ecf154a4aec8588c0ef5626ef08c23c35e35927f6b0497b5f086146fe
Raise RPC_WALLET_NOT_SPECIFIED instead of RPC_METHOD_NOT_FOUND when a required
wallet filename was not specified in an RPC call.
Also raise more specific RPC_WALLET_NOT_FOUND error instead of
RPC_INVALID_PARAMETER in case an invalid wallet was specified, for consistency.
095b917 Avoid using sizes on non-fixed-width types to derive protocol constants. (Gregory Maxwell)
Pull request description:
Thanks to awemany for pointing this out.
This replaces #10172 which appears to be abandoned, but uses the constants as requested on that PR.
Tree-SHA512: 032c0d75b3aaf807a7d0c7fb8ff5515acc45ad58bd00fe81413f900fe02bad900534a970403b9bb568e132c9eddea6043e958daf625e8acc84375bd41ee2e2ef
9f8a46f Properly document target_confirmations in listsinceblock (Ryan Havar)
Pull request description:
There seems to be some misunderstandings about this, but it's a heavily used function so I'd like to make sure the docs are clear about how it works.
For a later issue:
* Change the default of target_confirmations to 6 (1 is a pretty silly default)
* Change the name of target_confirmations (it's really a horrible name)
Tree-SHA512: a2fba2fab30019cea9db56cd7e31de95ba31090617ab336bdf130f9591bfcf3fc5fbd9e7e1e40b6c7bd2f74b9b4658afb1fdc7fc44e1f79520d1319758982a1c
99c7fc3 Prevent user from specifying conflicting parameters to fundrawtx (Matt Corallo)
Pull request description:
estimate_mode/conf_target both are overridden by feeRate, so should
not be specified together with feeRate.
Based on #10706
Tree-SHA512: 8ccd08575fd1f2a0d45112538ffbbc73983ee172963230b0cc7ac41d13c6f3c740917f82b212c41ded3a64d873452e7f2c7af49f3b47cab897f8e85117f21333
This assures that we don't overwrite a random file called
`wallet.backup` that happens to be in the current directory. It also
assures that the temporary file will be cleaned up.
Noticed by Evan Klitzke, came up in discussion here:
https://github.com/bitcoin/bitcoin/pull/10880#discussion_r128460722
a5ecaf1 Fix misspellings and remove safety verbiage (Steven D. Lander)
Pull request description:
Standardizing punctuation on CLI output and also including a few fixes for grammer. This PR is for text only changes and includes no code edits.
Tree-SHA512: afde551bf1212838822188b6723f2bf1b7222decfa1cd7aa6b04967489108a29f80833af6059252af028c53437755f258275af0614e0d4d0311e09421cd8e131
876e92b Testing: listsinceblock should display all transactions that were affected since the given block, including transactions that were removed due to a reorg. (Karl-Johan Alm)
f999c46 listsinceblock: optionally find and list any transactions that were undone due to reorg when requesting a non-main chain block in a new 'removed' array. (Karl-Johan Alm)
Pull request description:
The following scenario will not notify the caller of the fact `tx0` has been dropped:
1. User 1 receives BTC in tx0 from utxo1 in block aa1.
2. User 2 receives BTC in tx1 from utxo1 (same) in block bb1
3. User 1 sees 2 confirmations at block aa3.
4. Reorg into bb chain.
5. User 1 asks `listsinceblock aa3` and does not see that tx0 is now invalidated.
See `listsinceblock.py` commit for related test.
The proposed fix is to iterate from the given block down to the fork point, and to check each transaction in the blocks against the wallet, in addition to including all transactions from the fork point to the active chain tip (the current behavior). Any transactions that were present will now also be listed in the `listsinceblock` output in a new `replaced` array. This operation may be a bit heavy but the circumstances (and perceived frequency of occurrence) warrant it, I believe.
Example output:
```Python
{
'transactions': [],
'replaced': [
{
'walletconflicts': [],
'vout': 1,
'account': '',
'timereceived': 1485234857,
'time': 1485234857,
'amount': '1.00000000',
'bip125-replaceable': 'unknown',
'trusted': False,
'category': 'receive',
'txid': 'ce673859a30dee1d2ebdb3c05f2eea7b1da54baf68f93bb8bfe37c5f09ed22ff',
'address': 'miqEt4kWp9zSizwGGuUWLAmxEcTW9bFUnQ',
'label': '',
'confirmations': -7
}
],
'lastblock': '7a388f27d09e3699102a4ebf81597d974fc4c72093eeaa02adffbbf7527f6715'
}
```
I believe this addresses the comment by @luke-jr in https://github.com/bitcoin/bitcoin/pull/9516#issuecomment-274190081 but I could be wrong..
Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35
This commit adds a listwallets RPC, which lists the names of the
currently loaded wallets. This command intentionally shows no
information about the wallet other then the name. Information on
individual wallets can be obtained using the getwalletinfo RPC.
4dc1915 check for null values in rpc args and handle appropriately (Gregory Sanders)
999ef20 importmulti options are optional (Gregory Sanders)
a70d025 fixup some rpc param counting for rpc help (Gregory Sanders)
Pull request description:
Audited where named args will fail to use correct default values or may fail when additional optional arguments are added.
Previously for these parameters, it was fine to omit them as positional arguments, but it would trigger UniValue runtime errors to set them to null, or to omit them while passing named parameters with greater positions (which would internally set earlier missing arguments to null). Now null values are treated the same as missing values so these errors do not occur.
Included a few other small fixes while working on it.
I didn't bother fixing account-based rpc calls.
Tree-SHA512: 8baf781a35bd48de7878d4726850a580dab80323d3416c1c146b4fa9062f8a233c03f37e8ae3f3159e9d04a8f39c326627ca64c14e1cb7ce72538f934ab2ae1e
