ScriptPubKeyMan is only using UnsetWalletFlagWithDB to unset the blank
wallet flag. Just make that it's own function and not expose the flag
writing directly.
This does not change behavior.
d314e8a818d4c162b1c7201533e6b600dcab2d91 refactor: Replace all uses of boost::optional with our own Optional type (Wladimir J. van der Laan)
Pull request description:
Replace all uses of boost::optional with our own Optional type. Luckily, there aren't so many.
After this:
- `boost::optional` is no longer used directly (only through `Optional` which is an alias for it)
- `boost/optional.hpp` is only included in one place
ACKs for top commit:
MarcoFalke:
ACK d314e8a818d4c162b1c7201533e6b600dcab2d91
practicalswift:
ACK d314e8a818d4c162b1c7201533e6b600dcab2d91 -- diff looks correct + satisfying to see incremental progress towards the goal of a Boost free future :)
jtimon:
ACK d314e8a818d4c162b1c7201533e6b600dcab2d91
fanquake:
ACK d314e8a818d4c162b1c7201533e6b600dcab2d91
Tree-SHA512: b43e0017af81b07b5851377cd09624f114510ac5b9018d037664b58ad0fc8e893e30946b61f8f5e21e39125925bf9998a81f2226b468aab2df653ee57ed3213d
After this:
- `boost::optional` is no longer used directly (only through `Optional`
which is an alias for it)
- `boost/optional.hpp` is only included in one place
362ded410b8cb1104b7ef31ff8488fec4824a7d5 Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b751a3e6b3b9f6fb7961c442877fb65a scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5e42feaf7bfa5f41e80292aaa73cd7d Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b71c9d91399c31b043237091be2e5e7 MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e6765b185bd55f4c541f9e27aeea29d scripted-diff: Rename InitInterfaces to NodeContext (Russell Yanofsky)
Pull request description:
This change is mainly a naming / organization change intended to simplify #10102. It:
- Renames struct InitInterfaces to struct NodeContext and moves it from
src/init.h to src/node/context.h. This is a cosmetic change intended to make
the point of the struct more obvious.
- Gets rid of BanMan and ConnMan globals making them NodeContext members
instead. Getting rid of these globals has been talked about in past as a way
to implement testing and simulations. Making them NodeContext members is a
way of keeping them accessible without the globals.
- Splits g_rpc_interfaces global into g_rpc_node and g_rpc_chain globals. This
better separates node and wallet rpc methods. Node RPC methods should have
access NodeContext, while wallet RPC methods should only have indirect access
to node functionality via interfaces::Chain.
- Adds NodeContext& references to interfaces::Chain class and the
interfaces::MakeChain() function. This is needed to access ConnMan and BanMan
instances without the globals.
- Gets rid of redundant Node and Chain instances in Qt tests. This is
needed due to the previous MakeChain change, and also makes test setup a
little more straightforward. More cleanup could be done in the future, but it
will require deduplication of bitcoind, bitcoin-qt, and TestingSetup init
code.
ACKs for top commit:
laanwj:
ACK 362ded410b8cb1104b7ef31ff8488fec4824a7d5
Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2
f3b51eb9352d7a7c5dfa15615efc8bc0a52ffecf Fix occurences of c_str() used with size() to data() (Wladimir J. van der Laan)
Pull request description:
Using `data()` better communicates the intent here.
~~Also, depending on how `c_str()` is implemented, this fixes undefined behavior: The part of the string after the first NULL character might have undefined contents (or even be inaccessible, worst case).~~ Apparently [this is no longer an issue with C++11](https://github.com/bitcoin/bitcoin/pull/17281#discussion_r339742128).
ACKs for top commit:
fjahr:
Code review ACK f3b51eb
practicalswift:
ACK f3b51eb9352d7a7c5dfa15615efc8bc0a52ffecf -- diff looks correct, `data()` more idiomatic
ryanofsky:
Code review ACK f3b51eb9352d7a7c5dfa15615efc8bc0a52ffecf. Most of these calls (including one in crypter.cpp) are passing text strings, not binary strings likely to contain `\0` and were probably safe before, but much better to avoid the possibility of bugs like this.
Tree-SHA512: 842e1bdd37efc4ece2ecb87ca34962aafef0a192180051def630607e349dc9c8b4e562481fff3de474515f493b4ee3ea53b00269a801a66e625326a38dfce5b8
f201ba59ffd2e071a36a688b80d2cff9a9c44bb2 Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes (Andrew Chow)
6702048f91089d7a565e5ca5f7c8dcd2ca405a85 MOVEONLY: Move key handling code out of wallet to keyman file (Andrew Chow)
ab053ec6d1e766402f88947d29cd875a285e7280 Move wallet enums to walletutil.h (Andrew Chow)
Pull request description:
Moves key management functions into a new class LegacyScriptPubKeyMan. First two commits are move-only commits which move stuff out of wallet.{h/cpp} and into newly created scriptpubkeyman.{h/cpp}. Third commit changes several things in CWallet to use LegacyScriptPubKeyMan.
First step in the wallet boxes refactor. Note that LegacyScriptPubKeyMan and ScriptPubKeyMan cannot be used standalone yet and are still very much tied into CWallet with both accessing functions within each other. This PR is to help reduce review burden.
ACKs for top commit:
Sjors:
Code review ACK f201ba5.
promag:
Code review ACK f201ba59ffd2e071a36a688b80d2cff9a9c44bb2.
ryanofsky:
Code review ACK f201ba59ffd2e071a36a688b80d2cff9a9c44bb2
MarcoFalke:
ACK f201ba59ffd2e071a36a688b80d2cff9a9c44bb2
Tree-SHA512: bdc0d8595a06233fe003afcf968a38e0e8cc584a6a89c5bcd05309ac29dca852391802d46763ef81a108d146d0f40c79ea5438e87234ed12b4b8360c9aec94c0
c72906dcc11a73fa06a0adf97557fa756b551bee refactor: Remove redundant c_str() calls in formatting (Wladimir J. van der Laan)
Pull request description:
Our formatter, tinyformat, *never* needs `c_str()` for strings. Still, many places call it redundantly, resulting in longer code and a slight overhead.
Remove redundant `c_str()` calls for:
- `strprintf`
- `LogPrintf`
- `tfm::format`
(also, combined with #17095, I think this improves logging in case of unexpected embedded NULL characters)
ACKs for top commit:
ryanofsky:
Code review ACK c72906dcc11a73fa06a0adf97557fa756b551bee. Easy to review with `git log -p -n1 --word-diff-regex=. -U0 c72906dcc11a73fa06a0adf97557fa756b551bee`
Tree-SHA512: 9e21e7bed8aaff59b8b8aa11571396ddc265fb29608c2545b1fcdbbb36d65b37eb361db6688dd36035eab0c110f8de255375cfda50df3d9d7708bc092f67fefc
Wallet code should use interfaces::Chain and not directly access to node state.
Add a g_rpc_chain replacement global for wallet code to use, and move
g_rpc_node definition to a libbitcoin_server source file so there are link
errors if wallet code tries to access it.
Using `data()` better communicates the intent here.
Also, depending on how `c_str()` is implemented, this fixes undefined
behavior: The part of the string after the first NULL character might
have undefined contents.
This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.
Most of the changes are simple text replacements and variable substitutions
easily verified with:
git log -p -n1 -U0 --word-diff-regex=.
The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:
git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h
or
git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h
This commit does not change behavior.
Start moving wallet and ismine code to scriptpubkeyman.h, scriptpubkeyman.cpp
The easiest way to review this commit is to run:
git log -p -n1 --color-moved=dimmed_zebra
And check that everything is a move (other than includes and copyrights comments).
This commit is move-only and doesn't change code or affect behavior.
9e95931865186d7a9a6dc54b64bd96507e9fea4b [wallet] Remove `state` argument from CWallet::CommitTransaction (John Newbery)
d1734f9a3b138ab046f38ee44a09bc3847bf938a [wallet] Remove return value from CommitTransaction() (John Newbery)
b6f486a02b463ffeaf82ec11fc6f74f439c037ae [wallet] Add doxygen comment to CWallet::CommitTransaction() (John Newbery)
8bba91b22d22a8dfea7c947b542b1022bfc1c0ea [wallet] Fix whitespace in CWallet::CommitTransaction() (John Newbery)
Pull request description:
`CommitTransaction()` returns a bool to indicate success, but since commit
b3a7410 (#9302) it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.
All dead code in `if (!CommitTransaction())` branches has been removed.
Two additional commits fix up the idiosyncratic whitespace in `CommitTransaction` and add a doxygen comment for the function.
ACKs for top commit:
laanwj:
ACK 9e95931865186d7a9a6dc54b64bd96507e9fea4b
Tree-SHA512: a55a2c20369a45222fc0e02d0891495655a926e71c4f52cb72624768dd7b9c1dca716ea67d38420afb90f40c6e0fd448caa60c18fd693bb10ecb110b641820e6
cd68594dcdadc195bd2ea9394fa04edfdbdf1149 Only check the hash of transactions loaded from disk (Andrew Chow)
Pull request description:
It feels unnecessary to do a full `CheckTransaction` for every transaction saved in the wallet. It should not be possible for an invalid transaction to get into the wallet in the first place, and if there is any disk corruption, the hash check will catch it.
ACKs for top commit:
MarcoFalke:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149
laanwj:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149
promag:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149, AFAICT the check is not needed, hash comparison gives data integrity.
Tree-SHA512: 5b2e719f76097cfbf125392db6cc6c764355c81f0b7a5b60aee4b06af1afcca80cfd38a3cf5307fd9e2c1afc405f8321929a4552943099a8161e6762965451fb
facec1c643105d0ae74b5d32cf33d593f9e82a36 wallet: Avoid showing GUI popups on RPC errors (MarcoFalke)
Pull request description:
RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example,
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed.
```
gives me a GUI popup and no reason why loading the wallet failed.
After this pull request:
```
$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet loading failed: Error loading /home/marco/workspace/btc_bitcoin_core/./test/functional/data/wallets/high_minversion/wallet.dat: Wallet requires newer version of Bitcoin Core
ACKs for top commit:
laanwj:
Code review ACK facec1c643105d0ae74b5d32cf33d593f9e82a36
Tree-SHA512: c8274bbb02cfcf71676eeec1e773e51fb3538cf93f82e7cb8536f4716d44ed819cdc162dfc039ac7386a4db381a734cdb27fd32567043a1180c02519fbcba194
CommitTransaction returns a bool to indicate success, but since commit
b3a74100b8 it only returns true, even if the transaction was not
successfully broadcast. This commit changes CommitTransaction() to return
void.
All dead code in `if (!CommitTransaction())` branches has been removed.
b96ed0396294fc4fa89d83ceab6bc169dd09f002 [wallet] Remove pruning check for -rescan option (John Newbery)
eea462de9c652dca556ad241d2126b10790f67f8 [wallet] Remove package limit config access from wallet (John Newbery)
Pull request description:
Removes wallet access to `-limitancestorcount`, `-limitdescendantcount` and `-prune`:
- `-limitancestorcount` and `-limitdescendantcount` are now accessed with a method `getPackageLimits` in the `Chain` interface.
- `-prune` is not required. It was only used in wallet component initiation to prevent running `-rescan` when pruning was enabled. This check is not required.
Partially addresses #17137.
ACKs for top commit:
MarcoFalke:
Tested ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002
ryanofsky:
Code review ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002
promag:
Code review ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002.
ariard:
ACK b96ed03, check there isn't left anymore wallet access to node arguments.
Tree-SHA512: 90c8e3e083acbd37724f1bccf63dab642cf9ae95cc5e684872a67443ae048b4fdbf57b52ea47c5a1da6489fd277278fe2d9bbe95e17f3d4965a1a0fbdeb815bf
66b29848c71c4b3b4dc36ca6d94de829bd533797 change wallet pointers to references in feebumper (Adam Jonas)
9be6666a4e648782b49a6fdc458372ea521b444d typo and unneccessary parentheses (Adam Jonas)
Pull request description:
Picking up some of the suggestions in the comments of #16727 including:
https://github.com/bitcoin/bitcoin/pull/16727#discussion_r330547321https://github.com/bitcoin/bitcoin/pull/16727#discussion_r330549766https://github.com/bitcoin/bitcoin/pull/16727#discussion_r333209674
ACKs for top commit:
promag:
Code review ACK 66b29848c71c4b3b4dc36ca6d94de829bd533797.
MarcoFalke:
ACK 66b29848c71c4b3b4dc36ca6d94de829bd533797 (looked at the diff on GitHub)
fjahr:
ACK 66b2984 reviewed code
Tree-SHA512: d118f7689970fe39d9f5318dc818f13283cce9194370b3ce4758f298172e4681ae119ddc809f5c0b7602677137ac0d38147b915422ff616531a76a570b766fa2
Prior to this PR, the wallet would not allow the `-rescan` option at
startup if pruning was enabled. This is unnecessarily restrictive. It
should be possible to rescan if pruning is enabled, as long as no blocks
have actually been pruned yet.
Remove the pruning check from WalletInit::ParameterInteraction(). If any
blocks have been pruned, that will be caught in CreateWalletFromFile().
The wallet should not be able to directly access global configuration
from the node. Remove access of "-limitancestorcount" and
"-limitdescendantcount".
