This makes calling code less verbose and less fragile. Also, by adding
the CKey::data() member function, it is now possible to call HexStr()
with a CKey object.
c5d7e34bd9 scripted-diff: disable unimplemented ArgsManager BOOL/INT/STRING flags (Russell Yanofsky)
b8c069b7a9 refactor: Add explicit DISALLOW_NEGATION ArgsManager flag to clarify flag usage (Russell Yanofsky)
26a50ab322 refactor: Split InterpretOption into Interpret{Key,Value} functions (Russell Yanofsky)
Pull request description:
This is preparation for #16545 or another PR implementing type validation for ArgsManager settings. It fixes misleading usages of existing flags, prevents flags from being similarly misused in the future, and allows validation logic to be added without breaking backwards compatibility.
---
Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is #16545).
An additional complication is that while these flags don't do any real settings validation, they do affect whether setting negation syntax is allowed.
Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in three commits, with the first commit cleaning up some code, the second commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
None of the changes affect behavior in any way.
ACKs for top commit:
ajtowns:
utACK c5d7e34bd9
promag:
Code review ACK c5d7e34bd9, which as the new argument `-legacy`.
Tree-SHA512: cad0e06361e8cc584eb07b0a1f8b469e3beea18abb458c4e43d9d16e9f301b12ebf1d1d426a407fbd96f99724ad6c0eae5be05c713881da7c55e0e08044674eb
9ba7c44265 refactor: get wallet path relative to wallet_dir (Michael Dietz)
Pull request description:
Now that boost has been updated > 1.60 (see #22320), we can simplify how we get
wallet path relative to wallet_dir by using:
`boost::filesystem::lexically_relative`, removing a TODO.
Test coverage comes from `test/functional/wallet_multiwallet.py`
I first tried this in #20265 which was my first attempted PR, and funny enough exactly 1 year later I'm opening this one to hopefully finally close this.
ACKs for top commit:
ryanofsky:
Code review ACK 9ba7c44265. Basically this same code change is made in #20744 commit b70c84348ac7a8e427a1183f894c73e52c734529, so this PR helps simplify that one
lsilva01:
Code Review ACK 9ba7c44
Tree-SHA512: 6ccb91a18bcb52c3ae0c789a94a18fb5be7db7769fd1121552d63f259fbd32b50c3dcf169cec0b02f978321db3bc60eb4b881b8327e9764f32e700236e0d8a35
Now that boost has been updated > 1.60, we can simplify how we get
wallet path relative to wallet_dir by using:
`boost::filesystem::lexically_relative`
54011e7aa2 refactor: use CWallet const shared pointers when possible (Karl-Johan Alm)
96461989a2 refactor: const shared_ptrs (Karl-Johan Alm)
Pull request description:
```C++
const std::shared_ptr<CWallet> wallet = x;
```
means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like.
This PR
* introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing)
* uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified
In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s.
Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.
ACKs for top commit:
theStack:
re-ACK 54011e7aa2
Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836
This commit does not change behavior in any way. See previous commit for
complete rationale, but these flags are being disabled because they
aren't implemented and will otherwise break backwards compatibility when
they are implemented.
-BEGIN VERIFY SCRIPT-
sed -i 's:\(ALLOW_.*\) \(//!< unimplemented\):// \1\2:' src/util/system.h
sed -i '/DISALLOW_NEGATION.*scripted-diff/d' src/util/system.cpp
git grep -l 'ArgsManager::ALLOW_\(INT\|STRING\)' | xargs sed -i 's/ArgsManager::ALLOW_\(INT\|STRING\)/ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION/g'
git grep -l 'ALLOW_BOOL' -- ':!src/util/system.h' | xargs sed -i 's/ALLOW_BOOL/ALLOW_ANY/g'
-END VERIFY SCRIPT-
While const shared_ptr<X> gives us an immutable shared pointer to a mutable X (we can't set it to some other X later), shared_ptr<const X> gives us a shared pointer to an immutable X. Importantly, we can recast shared_ptr<X> into shared_ptr<const X>, but not the other way around. We do this for two reasons: because it makes the code safer to guarantee the wallet is not modified, and because it further dispells the misconception that const shared_ptr<X> gives immutability to X.
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
Default to SQLiteDatabase instead of BerkeleyDatabase for
CreateDummyWalletDatabase. Most tests already use descriptor wallets and
the mock db doesn't really matter for tests. The tests where it does
matter will make the db directly.
Instead of using AddToWallet so that making a COutput will work,
directly add the transaction into wallet.mapWallet. This bypasses many
checks that AddToWallet will do which are pointless and just slow down
this test.
66f6efc70a rpc: improve TransactionDescriptionString() "generated" help (Jon Atack)
296cfa312f test: add listtransactions/listsinceblock "trusted" coverage (Jon Atack)
d95913fc43 rpc: fix "trusted" description in TransactionDescriptionString (Jon Atack)
Pull request description:
The RPC gettransaction, listtransactions, and listsinceblock helps returned by `TransactionDescriptionString()` inform the user that the `trusted` boolean field is only present if the transaction is trusted and safe to spend from.
The field is in fact returned by `WalletTxToJSON()` when the transaction has 0 confirmations (or negative confirmations, if conflicted), and it can be true or false.
This patch fixes the help, adds test coverage, and touches up the help for the neighboring `generate` field.
ACKs for top commit:
rajarshimaitra:
tACK 66f6efc70a
theStack:
Tested ACK 66f6efc70a
Tree-SHA512: 4c2127765b82780e07bbdbf519d27163d414d9f15598e01e02210f210e6009be344c84951d7274e747b1386991d4c3b082cd25aebe885fb8cf0b92d57178f68e
2d2edc1248 tests: Use Descriptor wallets for generic wallet tests (Andrew Chow)
99516285b7 tests: Use legacy change type in subtract fee from outputs test (Andrew Chow)
dcd6eeb64a tests: Use descriptors in psbt_wallet_tests (Andrew Chow)
4b1588c6bd tests: Use DescriptorScriptPubKeyMan in coinselector_tests (Andrew Chow)
811319fea4 tests, gui: Use DescriptorScriptPubKeyMan in GUI tests (Andrew Chow)
9bf0243872 bench: Use DescriptorScriptPubKeyMan for wallet things (Andrew Chow)
5e54aa9b90 bench: remove global testWallet from CoinSelection benchmark (Andrew Chow)
a5595b1320 tests: Remove global vCoins and testWallet from coinselector_tests (Andrew Chow)
Pull request description:
Currently, various tests use `LegacyScriptPubKeyMan` because it was convenient for the refactor that introduced the `ScriptPubKeyMan` interface. However, with the legacy wallet slated to be removed, these tests should not continue to use `LegacyScriptPubKeyMan` as they are not testing any specific legacy wallet behavior. These tests are changed to use `DescriptorScriptPubKeyMan`s.
Some of the coin selection tests and benchmarks had a global `testWallet`, but this seemed to cause some issues with ensuring that descriptors were set up in that wallet for each test. Those have been restructured to not have any global variables that may be modified between tests.
The tests which test specific legacy wallet behavior remain unchanged.
ACKs for top commit:
laanwj:
Code review ACK 2d2edc1248
brunoerg:
tACK 2d2edc1248
Tree-SHA512: 6d60e5978e822d48e46cfc0dae4635fcb1939f21ea9d84eb72e36112e925554b7ee8f932c7ed0c4881b6566c6c19260bec346abdff1956ca9f300b30fb4e2dd1
6911ab95f1 wallet: fix segfault by avoiding invalid default-ctored `external_spk_managers` entry (Sebastian Falbesoner)
Pull request description:
Fixes#23321 (bug reported by Josef Vondrlik (josef-v)).
In the method `CWallet::LoadActiveScriptPubKeyMan`, the map `external_spk_managers` (or `internal_spk_managers`, if parameter `internal` is false) is accessed via std::map::operator[], which means that a default-ctored entry is created with a null-pointer as value, if the key doesn't exist. As soon as this value is dereferenced, a segmentation fault occurs, e.g. in `CWallet::KeypoolCountExternalKeys`.
The bevaviour can be reproduced by the following steps (starting with empty regtest datadir):
```
$ ./src/bitcoind -regtest -daemon
$ ./src/bitcoin-cli -regtest -named createwallet_name=wallet descriptors=true blank=true
$ cat regtest-descriptors.txt
[
{
"desc": "tr([e4445899/49'/1'/0']tprv8ZgxMBicQKsPd8jCeBWsYLEoWxbVgzJDatJ7XkwQ6G3uF4FsHuaziHQ5JZAW4K515nj6kVVwPaNWZSMEcR7aFCwL4tQqTcaoprMKTTtm6Zg/1/*)#mr3llm7f",
"timestamp": 1634652324,
"active": true,
"internal": true,
"range": [
0,
999
],
"next": 0
}
]
$ ./src/bitcoin-cli -regtest importdescriptors "$(cat regtest-descriptors.txt)"
[
{
"success": true
}
]
$ ./src/bitcoin-cli -regtest getwalletinfo
error: timeout on transient error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
```
ACKs for top commit:
achow101:
Code Review ACK 6911ab95f1
lsilva01:
Tested ACK 6911ab9 on Ubuntu 20.04.
instagibbs:
ACK 6911ab95f1
Tree-SHA512: 76aa96847cf2739413fb68fb902afef0b3ab9381178dd62fb0abac69f853f1f6523d73c60e610375b9a7730f275eda9162503b89f5be6e6e349a8d047b59c8dc
9c1052a521 wallet: Default new wallets to descriptor wallets (Andrew Chow)
f19ad40463 rpc, wallet: Descriptor wallets are no longer experimental (Andrew Chow)
Pull request description:
Changes the default wallet type from legacy to descriptors. Descriptor wallets will now by the default type. Additionally, descriptor wallets will no longer be marked as experimental.
This follows the timeline proposed in #20160
ACKs for top commit:
lsilva01:
Tested ACK 9c1052a521 on Ubuntu 20.04
prayank23:
tACK 9c1052a521
meshcollider:
Code review ACK 9c1052a521
Tree-SHA512: 834e6fec88e0c18673af7ebe135bd5333694d1be502164eb93a90e3e76c27974165aa4e59426945100c88e4eca07356e16886ef5b05cf789683ecb23fc71a12a
Follow-up to:
* commit 700c42b85d, which replaced pIndex
with block_hash in AddToWalletIfInvolvingMe.
* commit 9700fcb47f, which replaced
posInBlock with confirm.nIndex.
In the method `CWallet::LoadActiveScriptPubKeyMan`, the map
`external_spk_managers` (or `internal_spk_managers`, if parameter
`internal` is false) is accessed via std::map::operator[], which means
that a default-ctored entry is created with a null-pointer as value, if
the key doesn't exist. As soon as this value is dereferenced, a
segmentation fault occurs, e.g. in `CWallet::KeypoolCountExternalKeys`.
The bevaviour can be reproduced by the following steps (starting with empty regtest datadir):
$ ./src/bitcoind -regtest -daemon
$ ./src/bitcoin-cli -regtest -named createwallet_name=wallet descriptors=true blank=true
$ cat regtest-descriptors.txt
[
{
"desc": "tr([e4445899/49'/1'/0']tprv8ZgxMBicQKsPd8jCeBWsYLEoWxbVgzJDatJ7XkwQ6G3uF4FsHuaziHQ5JZAW4K515nj6kVVwPaNWZSMEcR7aFCwL4tQqTcaoprMKTTtm6Zg/1/*)#mr3llm7f",
"timestamp": 1634652324,
"active": true,
"internal": true,
"range": [
0,
999
],
"next": 0
}
]
$ ./src/bitcoin-cli -regtest importdescriptors "$(cat regtest-descriptors.txt)"
[
{
"success": true
}
]
$ ./src/bitcoin-cli -regtest getwalletinfo
error: timeout on transient error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
Bug reported by Josef Vondrlik (josef-v).
a0efe529e4 Fix outdated comments referring to ::ChainActive() (Samuel Dobson)
Pull request description:
After #21866 there are a few outdated comments referring to `::ChainActive()`, which should instead refer to `ChainstateManager::ActiveChain()`.
ACKs for top commit:
jamesob:
ACK a0efe529e4
Tree-SHA512: 80da19c105ed29ac247e6df4c8e916c3bf3f37230b63f07302114eef9c115add673e9649f0bbe237295be0c6da7b1030b5b93e14daf6768f17ce5de7cf2c9ff2
For the generic wallet tests, make DescriptorScriptPubKeyMans. There are
still some wallet tests that test legacy wallet things. Those remain
unchanged.
The subtract fee from outputs assumes that the leftover input amount
will be dropped to fees. However this only happens if that amount is
less than the cost of change. In the event that it is higher than the
cost of change, the leftover amount will actually become a change
output. To avoid this scenario, force a change type which has a high
cost of change.
To avoid issues with test data leaking across tests cases, the global
vCoins and testWallet are removed from coinselector_tests and all of the
relevant functions reworked to not need them.
6544ea5035 refactor: Block unsafe fs::path std::string conversion calls (Russell Yanofsky)
b39a477ec6 refactor: Add fs::PathToString, fs::PathFromString, u8string, u8path functions (Russell Yanofsky)
Pull request description:
The `fs::path` class has a `std::string` constructor which will implicitly convert from strings. Implicit conversions like this are not great in general because they can hide complexity and inefficiencies in the code, but this case is especially bad, because after the transition from `boost::filesystem` to `std::filesystem` in #20744 the behavior of this constructor on windows will be more complicated and can mangle path strings. The `fs::path` class also has a `.string()` method which is inverse of the constructor and has the same problems.
Fix this by replacing the unsafe method calls with `PathToString` and `PathFromString` function calls, and by forbidding unsafe method calls in the future.
ACKs for top commit:
kiminuo:
ACK 6544ea5035
laanwj:
Code review ACK 6544ea5035
hebasto:
re-ACK 6544ea5035, only added `fsbridge_stem` test case, updated comment, and rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22937#pullrequestreview-765503126) review. Verified with the following command:
Tree-SHA512: c36324740eb4ee55151146626166c00d5ccc4b6f3df777e75c112bcb4d1db436c1d9cc8c29a1e7fb96051457d317961ab42e6c380c3be2771d135771b2b49fa0
6531599f42 test: Add check that newkeypool flushes change addresses too (Samuel Dobson)
84fa19c77a Add release notes for keypool flush changes (Samuel Dobson)
f9603ee4e0 Add test for flushing keypool with newkeypool (Samuel Dobson)
6f6f7bb36c Make legacy wallet upgrades from non-HD to HD always flush the keypool (Samuel Dobson)
2434b10781 Fix outdated keypool size default (Samuel Dobson)
22cc797ca5 Add newkeypool RPC to flush the keypool (Samuel Dobson)
Pull request description:
This PR makes two main changes:
1) Adds a new RPC `newkeypool` which will entirely flush and refill the keypool.
2) When upgradewallet is called on old, non-HD wallets upgrading them to HD, we now always flush the keypool and generate a new one, to immediately start using the HD generated keys.
This PR is motivated by a number of users with old, pre-compressed-key wallets upgrading them and being confused about why they still can't generate p2sh-segwit or bech32 addresses -- this is due to uncompressed keys remaining in the keypool post-upgrade and being illegal in these newer address formats. There is currently no easy way to flush the keypool other than to call `getnewaddress` a hundred/thousand times or an ugly hack of using a `sethdseed` call.
ACKs for top commit:
laanwj:
re-ACK 6531599f42
meshcollider:
Added new commit 6531599f42 to avoid invalidating previous ACKs.
instagibbs:
ACK 6531599f42
Tree-SHA512: 50c79c5d42dd27ab0ecdbfdc4071fdaa1b2dbb2f9195ed325b007106ff19226419ce57fe5b1539c0c24101b12f5e034bbcfb7bbb0451b766cb1071295383d774
43568782c2 External input fund support cleanups (Gregory Sanders)
Pull request description:
Minor cleanups to https://github.com/bitcoin/bitcoin/pull/17211
ACKs for top commit:
achow101:
ACK 43568782c2
meshcollider:
utACK 43568782c2
benthecarman:
ACK 43568782c2
Tree-SHA512: 865f8a3804f8c0027f5393a0539041158166a919378f2c3bc99b936843eee2329372bcc2af888fa62babfa5f6baf4f13d4cfef7b4e26a7265a82a908f9719ad6
fafff132cf doc: Extract FundTxDoc (MarcoFalke)
Pull request description:
No need to duplicate the documentation for the same field(s) three times.
Fix that by de-duplicating it for the fields: conf_target, estimate_mode, replaceable, and solving_data.
Can be reviewed with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`.
ACKs for top commit:
fanquake:
ACK fafff132cf
Tree-SHA512: 098ddad3904b80b24c9e7b57ca8e807a6ccc3899eac2c9986d71ba3873c2b580bbb95f2fdfbf94b2db02f81c7b0ebf438a90324c23389b7b968ca85ae8475373
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
9d0379cea6 consensus: use <cstdint> over <stdint.h> in amount.h (fanquake)
863e52fe63 consensus: make COIN & MAX_MONEY constexpr (fanquake)
d09071da5b [MOVEONLY] consensus: move amount.h into consensus (fanquake)
Pull request description:
A first step (of a few) towards some source code reorganization, as well as making libbitcoinconsensus slightly more self contained.
Related to #15732.
ACKs for top commit:
MarcoFalke:
concept ACK 9d0379cea6 🏝
Tree-SHA512: 97fc79262dcb8c00996852a288fee69ddf8398ae2c95700bba5b326f1f38ffcfaf8fa66e29d0cb446d9b3f4e608a96525fae0c2ad9cd531ad98ad2a4a687cd6a
928af61cdb allow send rpc take external inputs and solving data (Andrew Chow)
e39b5a5e7a Tests for funding with external inputs (Andrew Chow)
38f5642ccc allow fundtx rpcs to work with external inputs (Andrew Chow)
d5cfb864ae Allow Coin Selection be able to take external inputs (Andrew Chow)
a00eb388e8 Allow CInputCoin to also be constructed with COutPoint and CTxOut (Andrew Chow)
Pull request description:
Currently `fundrawtransaction` and `walletcreatefundedpsbt` both do not allow external inputs as the wallet does not have the information necessary to estimate their fees.
This PR adds an additional argument to both those RPCs which allows the user to specify solving data. This way, the wallet can use that solving data to estimate the size of those inputs. The solving data can be public keys, scripts, or descriptors.
ACKs for top commit:
prayank23:
reACK 928af61cdb
meshcollider:
Re-utACK 928af61cdb
instagibbs:
crACK 928af61cdb
yanmaani:
utACK 928af61.
Tree-SHA512: bc7a6ef8961a7f4971ea5985d75e2d6dc50c2a90b44c664a1c4b0f1be5c1c97823516358fdaab35771a4701dbefc0862127b1d0d4bfd02b4f20d2befa4434700
