Commit graph

21025 commits

Author SHA1 Message Date
Wladimir J. van der Laan
5859b7dc6f
Merge #16338: test: Disable other targets when enable-fuzz is set
84edfc72e5 Update doc and CI config (qmma)
48bcb2ac24 Disable other targets when enable-fuzz is set (qmma)

Pull request description:

  This is to fix https://github.com/bitcoin/bitcoin/issues/16094

  When the `enable-fuzz` flag is set, disable all other binary targets.

ACKs for top commit:
  MarcoFalke:
    ACK 84edfc72e5 (only checked that travis compiled this)

Tree-SHA512: f4ac80526388a67709986b22de88b00bf93ab44ae31a20bd4d8923a4982ab97e015a9f13010081d6ecf6c23ae8afeac7ca9d849d198ce6ebe239aa3127151efc
2019-07-10 12:23:35 +02:00
Wladimir J. van der Laan
8d1286014c
Merge #16237: Have the wallet give out destinations instead of keys
8e7f930828 Add GetNewChangeDestination for getting new change Destinations (Andrew Chow)
33d13edd2b Replace CReserveKey with ReserveDestinatoin (Andrew Chow)
172213be5b Add GetNewDestination to CWallet to fetch new destinations (Andrew Chow)

Pull request description:

  The wallet should give out destinations instead of keys. It should be the one that handles the conversion from key to destination and the setting of the label, not the caller. In order to do this, two new member functions are introduced `GetNewDestination()` and `GetNewChangeDestination()`. Additionally, `CReserveKey` is changed to be `ReserveDestination` and represents destinations whose keys can be returned to the keypool.

ACKs for top commit:
  instagibbs:
    re-utACK 8e7f930828
  sipa:
    ACK 8e7f930828. Concept ACK as this gives a much cleaner abstraction to work with, and light code review ACK.
  laanwj:
    ACK 8e7f930828

Tree-SHA512: 5be7051409232b71e0ef2c1fd1a3e76964ed2f5b14d47d06edc2ad3b3687abd0be2803a1adc45c0433aa2c3bed172e14f8a7e9f4a23bff70f86260b5a0497500
2019-07-10 11:45:55 +02:00
Andrew Chow
1aecdf2063 Move wallet creation out of the createwallet rpc into its own function 2019-07-09 19:50:16 -04:00
MarcoFalke
357488f660
Merge #16240: JSONRPCRequest-aware RPCHelpMan
b6fb617aaa rpc: switch to using RPCHelpMan.Check() (Karl-Johan Alm)
c7a9fc234f Make the RPCHelpMan aware of JSONRPCRequest and add Check() helper (Karl-Johan Alm)
5c5e32bbe3 rpc: migrate JSONRPCRequest functionality into request.cpp (Karl-Johan Alm)
0ab8ba1ac6 rpc: fix RPC help requirements for getblocktemplate (Karl-Johan Alm)

Pull request description:

  Every single RPC call has a helper-section at the start, which throws a help string if the user asks for help or if the user provided too few/many arguments.

  ```C++
  const RPCHelpMan help{...};
  if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
      throw std::runtime_error(help.ToString());
  }
  ```

  or (older version)

  ```C++
  if (request.fHelp || request.params.size() < min || request.params.size() > max)
      throw std::runtime_error(
          RPCHelpMan{...}.ToString()
      );
  ```

  It seems like an obvious improvement, and less copy-pasting, to make `RPCHelpMan` aware of `JSONRPCRequest`, and to let it handle the checks instead. Both of the above become

  ```C++
  RPCHelpMan{...}.Check(request);
  ```

  which means we save roughly 3 lines per RPC command, and the `RPCHelpMan` instance is never referenced afterwards, so the approach is a tiny fraction cleaner.

  This is a complete update, sans a few special case locations that had special rules. 623 lines turn into 284 (which includes the addition to `RPCHelpMan`).

ACKs for top commit:
  laanwj:
    code rview and lightly tested ACK b6fb617aaa
  MarcoFalke:
    ACK b6fb617aaa, looked at the diff, verified move-only where applicable

Tree-SHA512: eb73f47f812512905b852e313281d1c8df803db40a6188aa39d5a7586631664db6764491152a8a96769946c796dc56d38c6e3a66ddd06ba3fb9d20050e6274e1
2019-07-09 19:31:52 -04:00
Andrew Chow
8e7f930828 Add GetNewChangeDestination for getting new change Destinations
Adds a GetNewChangeDestination that has the same objective as GetNewDestination
2019-07-09 16:43:10 -04:00
Andrew Chow
33d13edd2b Replace CReserveKey with ReserveDestinatoin
Instead of reserving keys, reserve destinations which are backed by keys
2019-07-09 16:43:10 -04:00
Andrew Chow
172213be5b Add GetNewDestination to CWallet to fetch new destinations
Instead of having the same multiple lines of code everywhere
that new destinations are fetched, introduce GetNewDestination as
a member function of CWallet which does the key fetching, label
setting, script generation, and destination generation.
2019-07-09 16:43:10 -04:00
Andrew Chow
93ce4a0b6f Move WatchOnly stuff from SigningProvider to CWallet 2019-07-09 16:20:18 -04:00
Andrew Chow
8f5b81e6ed Remove CCryptoKeyStore and move all of it's functionality into CWallet
Instead of having a separate CCryptoKeyStore that handles the encryption
stuff, just roll it all into CWallet.
2019-07-09 16:20:18 -04:00
Andrew Chow
37a79a4fcc Move various SigningProviders to signingprovider.{cpp,h}
Moves all of the various SigningProviders out of sign.{cpp,h} and
keystore.{cpp,h}. As such, keystore.{cpp,h} is also removed.

Includes and the Makefile are updated to reflect this. Includes were largely
changed using:
git grep -l "keystore.h" | xargs sed -i -e 's;keystore.h;script/signingprovider.h;g'
2019-07-09 16:20:18 -04:00
Andrew Chow
16f8096e91 Move KeyOriginInfo to its own header file 2019-07-09 16:20:18 -04:00
Andrew Chow
d9becff4e1 scripted-diff: rename CBasicKeyStore to FillableSigningProvider
-BEGIN VERIFY SCRIPT-
git grep -l "CBasicKeyStore" | xargs sed -i -e 's/CBasicKeyStore/FillableSigningProvider/g'
-END VERIFY SCRIPT-
2019-07-09 16:20:18 -04:00
Andrew Chow
a913e3f2fb Move HaveKey static function from keystore to rpcwallet where it is used 2019-07-09 16:20:12 -04:00
Matt Corallo
50cede3f5a [mempool] Allow one extra single-ancestor transaction per package
This implements the proposed policy change from [1], which allows
certain classes of contract protocols involving revocation
punishments to use CPFP. Note that some such use-cases may still
want some form of one-deep package relay, though even this alone
may greatly simplify some lightning fee negotiation.

[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
2019-07-09 15:46:25 -04:00
Andrew Chow
c7797ec655 Remove CKeyStore and squash into CBasicKeyStore 2019-07-09 15:28:41 -04:00
MarcoFalke
fa8489a155
test: Add test for BIP30 duplicate tx 2019-07-09 14:33:07 -04:00
MarcoFalke
77770d95e2
test: Properly serialize BIP34 coinbase height 2019-07-09 14:12:33 -04:00
Hennadii Stepanov
6285a318d7
Remove redundant WalletController::addWallet slot 2019-07-09 16:30:56 +03:00
Wladimir J. van der Laan
8046a3e0be
Merge #16348: qt: Assert QMetaObject::invokeMethod result
64fee48944 qt: Assert QMetaObject::invokeMethod result (João Barbosa)
f27bd96b5f gui: Fix missing qRegisterMetaType(WalletModel*) (João Barbosa)

Pull request description:

  Invalid/wrong dynamic calls aren't verified by the compiler. This PR asserts those dynamic calls. Once we bump Qt to at least 5.10 these can be refactored to use the `invokeMethod` overload that allows connecting to lambdas or member pointers, which are compile checked.

  For reference, one of the overloaded versions is https://doc.qt.io/qt-5/qmetaobject.html#invokeMethod-5.

ACKs for top commit:
  laanwj:
    ACK 64fee48944

Tree-SHA512: d332e5d7eb2c7be5d3fe90e2e4ff20a67800b9664f6637c122a23647a964f7915703d3f086e2de440f695cfe14de268ff581d0092b7736e911952a4f4d248e25
2019-07-09 11:48:01 +02:00
fanquake
0512f0521a
depends: expat 2.2.7 2019-07-09 08:47:41 +08:00
qmma
84edfc72e5
Update doc and CI config 2019-07-08 20:28:58 -04:00
João Barbosa
64fee48944 qt: Assert QMetaObject::invokeMethod result 2019-07-08 21:30:25 +01:00
João Barbosa
f27bd96b5f gui: Fix missing qRegisterMetaType(WalletModel*) 2019-07-08 21:30:25 +01:00
Carl Dong
e263a343d4
test: rpc_users: Make variable names more clear. 2019-07-08 16:13:35 -04:00
Carl Dong
830dc2dd0f
test: rpc_users: Also test rpcauth.py with specified password. 2019-07-08 16:13:34 -04:00
Carl Dong
c73d871799
test: rpc_users: Add function for testing auth params. 2019-07-08 16:13:33 -04:00
Carl Dong
604e2a997f
test: rpc_users: Add function for auth'd requests. 2019-07-08 16:13:32 -04:00
Wladimir J. van der Laan
c799976c86
Merge #16128: Delete error-prone CScript constructor only used with FindAndDelete
e1a55690e6 Delete error-prone CScript constructor (Gregory Sanders)

Pull request description:

  The behavior of this constructor is not the expected behavior compared to the other constructors which directly interpret the vector as a CScript, rather than serialize it into a new CScript. It has only four uses in the entire codebase. Delete this constructor and replace its four uses with the more clear serialization construction.

ACKs for top commit:
  Empact:
    ACK e1a55690e6
  sipa:
    Concept and code review ACK e1a55690e6, but I'd like to make sure we have tests covering the FindAndDelete usage.

Tree-SHA512: b6721e343c867ca401a80ec87c25939d7f1fc798f3bf7e5feb0ea6f8280eecb6bd65afc8286912c76ff8119ccea50ad7726b1a4137cae70c9d4fed7d960e10d3
2019-07-08 20:45:12 +02:00
Wladimir J. van der Laan
345f42a9e3
Merge #14505: test: Add linter to make sure single parameter constructors are marked explicit
c4606b8432 Add Travis check for single parameter constructors not marked "explicit" (practicalswift)

Pull request description:

  Make single parameter constructors `explicit` (C++11).

  Rationale from the developer notes:

  > - By default, declare single-argument constructors `explicit`.
  >   - *Rationale*: This is a precaution to avoid unintended conversions that might
  >   arise when single-argument constructors are used as implicit conversion
  >   functions.

ACKs for top commit:
  laanwj:
    ACK c4606b8432

Tree-SHA512: 3e6fd51935fd93b2604b2188664692973d0897469f814cd745b5147d71b99ea5d73c1081cfde9f6393f51f56969e412fcda35d2d54e938a3235b8d40945f31fd
2019-07-08 20:29:00 +02:00
Wladimir J. van der Laan
0a6ee9797e
Merge #16267: bench: Benchmark blockToJSON
91509ffe24 bench: Benchmark blockToJSON (Kirill Fomichev)

Pull request description:

  Related:
  - "getblock performance issue on verbosity" https://github.com/bitcoin/bitcoin/issues/15925
  - "refactor: Avoid UniValue copy constructor" #15974

ACKs for top commit:
  laanwj:
    ACK 91509ffe24

Tree-SHA512: e70b12cb31921c7527bde334f52f39776da698b6bbdb196079a8b68478c67585a5bd7bed7403f65166bd604f7ed60778c53dc064d743bb8368318a1283d1073e
2019-07-08 20:14:31 +02:00
MarcoFalke
4882040182
Merge #16291: gui: Stop translating PACKAGE_NAME
fa64b947bb util: No translation of `Bitcoin Core` in the copyright (MarcoFalke)
fab85208f6 qt: Run «make translate» in ./src/ (MarcoFalke)
fabe87d2c9 scripted-diff: Avoid passing PACKAGE_NAME for translation (MarcoFalke)
fa5e9f157e build: Stop translating PACKAGE_NAME (MarcoFalke)

Pull request description:

  Generally the package name is not translated, but the package description is.

  E.g. `GIMP` or `Firefox` are always called that way regardless of the system language. However, "`Firefox` webbrowser" or "`GIMP` image manipulation program" are translated.

ACKs for top commit:
  hebasto:
    ACK fa64b947bb, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 626f811531182d0ba0ef1044930d32726773349bcb49b10261288a86ee6b80a183db30a87d817d5b0d501fad058ac22d6272311716b4f5a154f17c6f391a5a1a
2019-07-08 13:39:59 -04:00
James O'Beirne
682a1d0f20 refactoring: remove mapBlockIndex global
in lieu of ::BlockIndex().
2019-07-08 11:33:13 -04:00
James O'Beirne
55d525ab90 refactoring: make pindexBestInvalid internal to validation.cpp
There's no need to have this member live on CChainState since it's only used
in validation.cpp.
2019-07-08 11:33:13 -04:00
James O'Beirne
4ed55dfcd7 refactoring: add block_index_candidates arg to LoadBlockIndex
Prevents BlockManager from having to reference ChainstateActive()
within one of its methods which improves encapsulation and makes
testing easier.
2019-07-08 11:33:12 -04:00
James O'Beirne
613c46fe9e refactoring: move block metadata structures into BlockManager
Separate out the management of chain-agnostic block metadata from any given
CChainState instance. This allows us to avoid duplicating data like
`mapBlockIndex` unnecessarily for multiple chainstates.

This also adds a CChainState constructor that accepts and sets m_blockman.
Ultimately this reference will point to a BlockMan instance that
is shared across CChainStates.

This commit can be decomposed into smaller commits if necessary.
2019-07-08 11:33:12 -04:00
Wladimir J. van der Laan
2679bb8919
Merge #16106: gui: Sort wallets in open wallet menu
fa90fe6440 refactor: Rename getWallets to getOpenWallets in WalletController (João Barbosa)
224eb9534a gui: Sort wallets in open wallet menu (João Barbosa)

Pull request description:

  Ensure wallets are sorted by name in the open wallet menu. This also improves the change done in #15957, since it avoids a second call to `listWalletDir`.

ACKs for top commit:
  laanwj:
    code review ACK fa90fe6440

Tree-SHA512: 349ea195021e56760dea3551126d1b9dc4821faab44aaf2858229db4fddaf0ce0b5eb0a8fa5025f47c77134b003067a77e8c340f9655a99e1305d430ccecced8
2019-07-08 16:32:01 +02:00
João Barbosa
fa90fe6440 refactor: Rename getWallets to getOpenWallets in WalletController 2019-07-08 15:07:17 +01:00
João Barbosa
224eb9534a gui: Sort wallets in open wallet menu 2019-07-08 15:03:49 +01:00
Wladimir J. van der Laan
b5fa2319d8
Merge #15687: test: tool wallet test coverage for unexpected writes to wallet
7195fa792f test: Tool wallet test coverage for unexpected writes to wallet (Jon Atack)
3bf2b3a37b test: Split tool_wallet.py test into subtests (Jon Atack)
1eb13f09a9 test: Add log messages to test/functional/tool_wallet.py (Jon Atack)

Pull request description:

  This pull request adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in https://github.com/bitcoin/bitcoin/issues/15608 and serve as a benchmark for fixing the issue:

  - Wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write.

  - Wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only.

  Goals:

  1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix. Add debug-level logging for sanity checking and commented-out assertions to be uncommented when fixing the issue. Add the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue.

  2. Add info log messages as there are currently none in the test file.

  3. Split the tests out to separate functions as per review feedback.

  Thanks to Marco Falke for pointing me in the right direction.

ACKs for top commit:
  laanwj:
    code review ACK 7195fa792f

Tree-SHA512: 16a41cce989c8f819cf5b02c6cf8ea84653ede2738fb402f6c36cf4dc075b424dff3e2c73a1cfa1ec9c75f614675baecc71e588845a2596db06ba0957db2df7b
2019-07-08 14:57:06 +02:00
Wladimir J. van der Laan
983c84844c
Merge #16352: build: prune dbus from depends
e8fabd9253 build: prune dbus from depends (fanquake)

Pull request description:

  Since #8210 (59d063d076), we've been passing `-dbus-runtime` when configuring Qt.

  ```
  qtbase-opensource-src-5.9.7 $ ./configure -h | grep -i dbus
    -no-dbus ............. Do not build the Qt D-Bus module
    -dbus-linked ......... Build Qt D-Bus and link to libdbus-1 [auto]
    -dbus-runtime ........ Build Qt D-Bus and dynamically load libdbus-1 [no]
  ```

  This means we don't actually seem to be using the `D-Bus` we build in depends. This was pointed out by theuni at the time, [here](https://github.com/bitcoin/bitcoin/pull/7993#issuecomment-223114395) and [here](https://github.com/bitcoin/bitcoin/pull/8210#issuecomment-226930545), but was never followed up. dongcarl also bought it up as part of #16150.

  I've tested building and running `bitcoin-qt` using depends on Debian. Needs further testing.

ACKs for top commit:
  laanwj:
    code review ACK e8fabd9253

Tree-SHA512: 164e6e52b6f97c04aef42bd185e2a157bc1a42103840f9404c5a795749f45a8c2c35f35873395a3a56398b3cd5955496b90d9c885d929b434c9bc871695abe20
2019-07-08 13:33:07 +02:00
Jon Atack
7195fa792f
test: Tool wallet test coverage for unexpected writes to wallet
This commit adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in https://github.com/bitcoin/bitcoin/issues/15608:

    - wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write.

    - wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only.

1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix in the form of commented-out assertions to be uncommented when testing/fixing.

2. Provisionally extend the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue.

3. Add some logging for sanity checking.

------

Changes after rebase:

5. Make wallet_path an instance method instead of a function in tool_wallet.py as per Marco Falke review suggestion.

6. Assert wallet permissions instead of logging them in tool_wallet.py. This ran into an issue with Appveyor keeping permissions at 666 so allowed for 666 as a workaround.

7. Change the added logging from info to debug level.

8. More helpful assertions order in tool_wallet.py#assert_tool_output. This change makes #assert_tool_output raise "Error loading wallet.dat. Is wallet being used by another process?" rather than a less-helpful message when debugging the read-only wallet permissions issue.
2019-07-08 13:02:28 +02:00
Jon Atack
3bf2b3a37b
test: Split tool_wallet.py test into subtests
as per Marco Falke review suggestion.
2019-07-08 13:02:23 +02:00
Wladimir J. van der Laan
f5a01cf259
Merge #16332: rpc: Add logpath description for getrpcinfo
a30bd09454 Add logpath description for getrpcinfo (Gregory Sanders)

Pull request description:

  Introduced in https://github.com/bitcoin/bitcoin/pull/15483

ACKs for top commit:
  fanquake:
    ACK a30bd09454

Tree-SHA512: f561af675d1184412b9e426debab6269f80a65098fc7226ee93581f4075dfc93846dd4b226bd4842eb43e1649d3291c7d18558bfeb851970728b64b8a0e6df0f
2019-07-08 12:57:10 +02:00
Jon Atack
1eb13f09a9
test: Add log messages to test/functional/tool_wallet.py
and update code comments as per Python PEP 8 style guide.
2019-07-08 12:33:41 +02:00
Wladimir J. van der Laan
e1a8d76aff
Merge #16347: doc: Include static members in Doxygen
84ad4d2b9d doc: Include static members in Doxygen (Carl Dong)

Pull request description:

  This makes our Doxygen output more useful by generating them for static members.

ACKs for top commit:
  practicalswift:
    ACK 84ad4d2b9d
  laanwj:
    ACK 84ad4d2b9d
  fanquake:
    ACK 84ad4d2b9d

Tree-SHA512: f47fe6f36739ba8d7978169b28a29ad3d0796d7535052e447740077f4827c9bf5082d14c9cac2fbaf91f01bb2bffc25d9d7c3f702c0848c79a48a311ebd3344f
2019-07-08 09:29:48 +02:00
Karl-Johan Alm
b6fb617aaa
rpc: switch to using RPCHelpMan.Check() 2019-07-08 09:53:52 +09:00
Karl-Johan Alm
c7a9fc234f
Make the RPCHelpMan aware of JSONRPCRequest and add Check() helper 2019-07-08 09:53:52 +09:00
fanquake
2b465195e0
Merge #16339: doc: add reduce-memory.md
64b27c46e4 docs: add reduce-memory.md (fanquake)

Pull request description:

  Following some discussion in https://github.com/bitcoin-core/docs/issues/50, this adds Wladimir's [reducing bitcoind memory usage gist](https://gist.github.com/laanwj/efe29c7661ce9b6620a7) to `/doc`.

  The conclusion seemed to be that if the main repo already has [reduce-traffic.md](https://github.com/bitcoin/bitcoin/blob/master/doc/reduce-traffic.md), then we could also add `reduce-memory.md`.

ACKs for top commit:
  practicalswift:
    ACK 64b27c46e4
  hebasto:
    ACK 64b27c46e4, I have reviewed the changes and they look OK, I agree they can be merged. Also a link from `/doc/README.md` has been tested.
  jonasschnelli:
    ACK 64b27c46e4

Tree-SHA512: 0ab3035403e5145cfe33c29990a8d082df834ac6602b4ad6bfa821523d57e8451f0cde3017fbf3c2c4e0b34941b6374909d11d27f9598e211bbc14accd487be1
2019-07-08 08:35:18 +08:00
fanquake
05623c0216
Merge #16350: qt: Remove unused guard
d003110351 Remove unused guard (Hennadii Stepanov)

Pull request description:

  `BITCOIN_QT_TEST` is no longer used since switching to autotools build system.

  Some historical refs:
  - #807
  - #4241

ACKs for top commit:
  practicalswift:
    utACK d003110351
  promag:
    ACK d003110351.
  jonasschnelli:
    Verified ACK d003110351

Tree-SHA512: 1242ef7927d2dbd2e47cdb50de6ebb20e4ac427a66a37b4d4de8ca1b50581d34f818cb576fc9fdfb3e7dd7259d11812e3807da33b3357850d67548b837d5549b
2019-07-08 08:22:32 +08:00
fanquake
e8fabd9253
build: prune dbus from depends 2019-07-07 14:26:41 +08:00