8616c81f08 doc: initial RapidCheck property-based testing documentation (Jon Atack)
Pull request description:
This commit proposes documentation that would have been helpful to me when setting up RapidCheck to work correctly with Bitcoin Core. I tested these instructions repeatedly with Linux (Debian 4.19 / GCC 8.3) and macOS (10.14.6 / AppleClang 10).
The markdown version can be seen at https://github.com/jonatack/bitcoin/blob/rapidcheck-documentation/doc/rapidcheck.md.
ACKs for top commit:
fanquake:
ACK 8616c81f08 - Thanks for updating.
Tree-SHA512: 9c2a65f0eb99f59e9adfea82855f217727a8a9d30618c1c0bdd2a73ae9c1ee61bc7efd59fc8703a666d182ad4220e22d6034ac3109311e6573dad00dfa4b06ea
ffea41f530 Enable all tests in feature_config_args.py (Hennadii Stepanov)
66f5c17f8a Use CheckDataDirOption() for code uniformity (Hennadii Stepanov)
7e33a18a34 Fix datadir handling in bitcoin-cli (Hennadii Stepanov)
b28dada374 Fix datadir handling in bitcoin-qt (Hennadii Stepanov)
50824093bb Fix datadir handling in bitcoind (Hennadii Stepanov)
740d41ce9f Add CheckDataDirOption() function (Hennadii Stepanov)
c1f325126c Return absolute path early in AbsPathForConfigVal (Hennadii Stepanov)
Pull request description:
Fix#15240, see: https://github.com/bitcoin/bitcoin/issues/15240#issuecomment-487353760Fix#15745
Fix broken `feature_config_args.py` tests (disabled by MarcoFalke@fabe28a0cdcfa13e0e595a0905e3642a960d3077). All test are enabled now.
This PR is alternative to #13621.
User's `$HOME` directory is not touched unnecessarily now.
~To make reviewing easier only `bitcoind` code is modified (neither `bitcoin-cli` nor `bitcoin-qt`).~
Refs:
- https://github.com/bitcoin/bitcoin/issues/15745#issuecomment-479852569 by **laanwj**
- #16220
Top commit has no ACKs.
Tree-SHA512: 4a4cda10e0b67c8f374da0c9567003d2b566d948e7f8550fe246868b5794c15010e88ea206009480b9cd2f737f310a15e984f920730448f99a895893bed351df
fa21737ba7 ci: Add environment files for all settings (MarcoFalke)
Pull request description:
This moves all environment settings from travis to files in the ci folder. Now, it is possible to easily run each travis configuration with a single command.
Top commit has no ACKs.
Tree-SHA512: 989c6b62eb3839eb1fa5461e986496e9660167e2438a789c7588a6fee4f9c37b332782c010fe5c7de8f606bcf98dffb2481d2777cbce88f87cc9f0c42fb2d7fc
20ea9ef6ce [doc] mention whitelist is inbound, and applies to blocksonly (Sjors Provoost)
Pull request description:
* `-whitelist` only impacts inbound nodes (see #9923). This is obvious in the context of allowing those nodes to connect to you, but there are additional whitelist features where this is less obvious, such as mempool relay behavior.
* `whitelistrelay` (on by default) explains that `-blocksonly` makes an exception for transactions from whitelisted nodes, but it wasn't documented (nor obvious imo) the other way around. See also https://github.com/bitcoin/bitcoin/pull/15984#issuecomment-490645552
Top commit has no ACKs.
Tree-SHA512: 03e363a5da5d81ad147d1c7e38bf11114df8bb89bdd66fb551520b25f810efa886ec6e649d3b435c4935e0ae4f39bb718bc7bb5778b9de6aa0b71e970a431af8
b168dd30cf Bugfix: QA: Run tests with UPnP disabled (Luke Dashjr)
Pull request description:
This replaces #16560 by adding `upnp=0` to `bitcoin.conf` rather than passing it to nodes.
> Needed for builds configured with --enable-upnp-default
You can test this change using:
```bash
./configure --enable-upnp-default && make -j6 && test/functional/test_runner.py feature_config_args.py
```
on master the test will fail without this change.
ACKs for top commit:
practicalswift:
ACK b168dd30cf -- diff looks correct
Tree-SHA512: e639dd480dda2cffa19a679018c4bd7e4bd4d0f5e3001d6b407b833e3c166bde98b201063e267b8e45f8a20b0d53ec8bc028bec806b2357f9a7ba314cc4e2d07
3b05f0f70f Reformat p2p_permissions.py (nicolas.dorier)
ce7eac3cb0 [Fix] The default whitelistrelay should be true (nicolas.dorier)
Pull request description:
I thought `whitelistrelay` default was `false` when it is `true`.
The root of the issue come from the fact that all references to `DEFAULT_` are not in the scope of this file, so hard coding of default values are used everywhere in `net.cpp`. I think that in a separate PR we should fix that more fundamentally everywhere.
ACKs for top commit:
promag:
ACK 3b05f0f70f.
Sjors:
re-ACK 3b05f0f70f
Tree-SHA512: f4a75f986fa2adf1a5f1c91605e0d261f7ac5ac8535fb05437d83b8392dbcf5cc1a47d755adcf8ad8dc67a88de28060187200fd3ce06545261a5c7ec0fea831a
fa0119af22 doc: Refer in rpcbind doc to the manpage (MarcoFalke)
Pull request description:
The help was outdated, so refer to the updated manpage instead.
* closes#14740
* closes#9272
ACKs for top commit:
emilengler:
Concept ACK fa0119a
laanwj:
ACK fa0119af22
fanquake:
ACK fa0119af22
Tree-SHA512: 9836e4f31ece7acae334b47e3104b4595d0d1c1977af386a9463faf49aea9de3ab1c1dc3de248d0ee09e3b13693eaf6902dc82d277062e1bb1980b1117236fbb
dc1bc1c503 doc: Add ZMQ dependencies to Fedora build (Hennadii Stepanov)
Pull request description:
Without `zeromq-devel` package on Fedora 30:
```
$ ./configure | grep zmq
configure: WARNING: libzmq version 4.x or greater not found, disabling
with zmq = no
```
With installed `zeromq-devel`:
```
$ ./configure | grep zmq
with zmq = yes
```
Also this PR improves style in the `miniupnpc` related paragraphs.
ACKs for top commit:
emilengler:
Concept ACK dc1bc1c
practicalswift:
ACK dc1bc1c503 -- diff looks correct
fanquake:
ACK dc1bc1c503 - tested in a Fedora 30 Docker container. Other changes match the Ubuntu & Debian docs above.
Tree-SHA512: 89b083a6256e8d0bcb9f0b193af84bc680843fdbb4a3e8871086477875250ad0ae214ce08797fc70c0cb04a2969632840c805a2a10a433763ee31cd1aa793bac
787c9ec0c3 Additional tests for other failure cases (Andrew Chow)
6e1ae58298 Check error messages in descriptor tests (Andrew Chow)
625534d7b1 Give more errors for specific failure conditions (Andrew Chow)
c325f619dd Return an error from descriptor Parse that gives more information about what failed (Andrew Chow)
Pull request description:
Because it's nigh impossible to figure out what is wrong with a descriptor otherwise.
ACKs for top commit:
Sjors:
ACK 787c9ec
meshcollider:
Code review ACK 787c9ec0c3
Tree-SHA512: 07c5deff127fd65507b96df2e4608b4f918e0cbd20b3b45b9ab5a05c2985a6efa9832079cf1e7f504305bfbb861019e93b96cc1574dd63b2ff0756b5928ece20
a6c1fc3cd9 build: echo prop tests status during build (Jon Atack)
Pull request description:
Enable users to see if the prop tests are enabled during the build. This can be particularly helpful as property-based tests are silently auto-enabled by default if librapidcheck is found.
Sample build output:
```
Options used to compile and link:
with wallet = yes
with gui / qt = yes
with bip70 = yes
with qr = yes
with zmq = yes
with test = yes
with prop = yes <--- new
with fuzz = no
with bench = no
```
ACKs for top commit:
fanquake:
ACK a6c1fc3cd9 - Thanks. Tested `./configure` with and without rapidcheck available.
Tree-SHA512: 460f3b83ee2a03e6dcc53bc326ac210d2484895e11766be117920fbc6fa78cdbcd4267cbceda9447c2f5fa5a7f218865ccc929ac6319308e397c0a5603571ecd
Some failure conditions implicitly fail by failing some other check.
But the error messages are more helpful if they say explicitly what
actually caused the failure, so add those as failure conditions and
errors.
d117f4541d Add test for setban (nicolas.dorier)
dc7529abf0 [Fix] Allow connection of a noban banned peer (nicolas.dorier)
Pull request description:
Reported by @MarcoFalke on https://github.com/bitcoin/bitcoin/pull/16248#discussion_r314026195
The bug would mean that if the peer connecting to you is banned, but whitelisted without specific permissions, it would not be able to connect to the node.
The solution is just to move the same line below.
ACKs for top commit:
Sjors:
Agree inline is more clear. utACK d117f45
MarcoFalke:
ACK d117f4541d
Tree-SHA512: 0fed39acb1e8db67bb0bf4c4de3ad034ae776f38d55bd661f1ae0e1a4c6becaf1824ab46ed8279f2f31df3f4b29ff56461d8b167d3e9cece62cfe58b5a912811
faba46da07 ci: Set --ansi in test_runner (MarcoFalke)
Pull request description:
Fixup to:
* tests: Use colors and dots in test_runner.py output only if standard output is a terminal #16561
ACKs for top commit:
practicalswift:
ACK faba46da07 -- diff looks correct
fanquake:
ACK faba46da07 - assuming Travis is all green.
Tree-SHA512: 50bf6ec8e7a2987d77821816289fd87458e63237d419a149e5e04027387d2c4b3c23db58977585fda30fbc4b13686a7def0d9e00096b9a8469dea2916fd30565
fa27c55b05 util: Move ResolveErrMsg to util/error (MarcoFalke)
Pull request description:
Pull request https://github.com/bitcoin/bitcoin/pull/16248#discussion_r314035862 duplicated the body of this util function. The whole point of the util function is to be shared, so do that here as a fixup to #16248
ACKs for top commit:
Sjors:
utACK fa27c55
ryanofsky:
utACK fa27c55b05
Tree-SHA512: e2b25ae05082fe9d0ee94bdc7d51f801bd9f78e8fc2b141e9a313e008dbb8a77653fe876e111c802c676859c6b76c37a673d1f8cfbe7ad25607a5ffcffde19fd
Enable users to see if the prop tests are enabled during the build. This can be particularly helpful as property-based tests are silently auto-enabled by default if librapidcheck is found.
Minor fixes to the docs and help grammar for this option.
390874c722 qt: Remove menu icons (Wladimir J. van der Laan)
Pull request description:
Remove the icons from the application menu. Why remove?
- They are inconsistently applied, some actions had icons, some newer ones don't. Good luck coming up with a sensible icon for everything
- Menu icons don't seem to have a place in modern UI: for example, GNOME, MacOS have stopped showing these a long time ago (see https://github.com/bitcoin/bitcoin/pull/16584#issuecomment-521195090)
- Less bikeshedding opportunity about "what should the icon for this be"
Removed icons:
```
/icons/quit res/icons/quit.png
/icons/about res/icons/about.png
/icons/about_qt res/icons/about_qt.png
/icons/options res/icons/configure.png
/icons/key res/icons/key.png
/icons/verify res/icons/verify.png (also .svg)
/icons/debugwindow res/icons/debugwindow.png
/icons/open res/icons/open.png
/icons/info res/icons/info.png
/icons/filesave res/icons/filesave.png
```
I checked that these icons are used nowhere else.
Removed from the menu not removed from the repository, because still referenced by other parts of the code:
```
/icons/lock_closed
/icons/edit
/icons/address-book
/icons/send
```
ACKs for top commit:
practicalswift:
ACK 390874c722 -- diff looks correct
l2a5b1:
ACK 390874c722 - Bitcoin Core has a very simple application menu. As long as the menu items describe their actions clearly and unambiguously then the icons alongside the label are redundant and offer very little value, if anything at all.
kallewoof:
ACK 390874c722
jonasschnelli:
utACK 390874c722
Tree-SHA512: dd1c52bed3bc6fb9359d5ea1b229a023dafaf813ae640775cbb433b9886bbc11a7d6a4306bac350b26d45fca9b495e4468630f2a32e185570e05f16a3ce45b47
72eaab073b tests: functional watch-only wallet tests (William Casarin)
72ffbdc579 doc: add release note for include_watchonly default changes (William Casarin)
003a3c73c0 rpcwallet: document include_watchonly default for watchonly wallets (William Casarin)
a50d9e6c0b rpcwallet: default include_watchonly to true for watchonly wallets (William Casarin)
Pull request description:
Right now it's a bit annoying to deal with watchonly wallets, many rpc commands have an `include_watchonly` argument that needs to be explicitly set.
Wallets created with `createwallet` can have a `disable_private_keys` parameter, for those wallets we already know that they are watchonly, so there's no reason to have to explicitly ask for it for every command. Instead we check this wallet flag when the `include_watchonly` parameter isn't set.
ACKs for top commit:
achow101:
Code review ACK 72eaab073b
Sjors:
ACK 72eaab073b
promag:
ACK 72eaab073b, code review only, didn't look closely to the test.
kallewoof:
ACK 72eaab073b
fanquake:
ACK 72eaab073b - I've looked over the changes, they make sense to me. Compiled and ran the tests etc.
Tree-SHA512: d3646b55e97f386594d7efc994f0712f3888475c6a5dc7f131ac9f8c49bf5d4677182b88f42b34152abe1ad101ecadd152b4c20e9d3c1267190db36f77ab8bd7
6576a8765f doc: Improve versionbits.h documentation (Antoine Riard)
Pull request description:
While reviewing burying of BIP 9 deployments, seen that versionbits.h wasn't that much documented. This is an attempt to improve it. It can be useful, given after burying this code isn't going to be used anymore and isn't straightforward at first sight.
ACKs for top commit:
jnewbery:
ACK 6576a8765f
ajtowns:
ACK 6576a8765f
fanquake:
ACK 6576a8765f
Tree-SHA512: 906463e0b22b988f89d77f798bf94d294f70467d29975088b87384764fb5d0dd1350be67562cc264656f61f1eada2cba20f99c0d797d1d7f90203c269e34c714
e78aaf41f4 [docs] Add release notes for burying bip 9 soft fork deployments (John Newbery)
8319e738f9 [tests] Add coverage for the content of getblockchaininfo.softforks (James O'Beirne)
0328dcdcfc [Consensus] Bury segwit deployment (John Newbery)
1c93b9b31c [Consensus] Bury CSV deployment height (John Newbery)
3862e473f0 [rpc] Tidy up reporting of buried and ongoing softforks (John Newbery)
Pull request description:
This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.
CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.
This was originally attempted by jl2012 in #11398 and again by me in #12360.
ACKs for top commit:
ajtowns:
ACK e78aaf41f4 ; checked diff to previous acked commit, checked tests still work
ariard:
ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected.
MarcoFalke:
ACK e78aaf41f4 (still didn't check if the mainnet block heights are correct, but the code looks good now)
Tree-SHA512: 7e951829106e21a81725f7d3e236eddbb59349189740907bb47e33f5dbf95c43753ac1231f47ae7bee85c8c81b2146afcdfdc11deb1503947f23093a9c399912
faeacf3269 ci: Add note that this assumes a fresh git clone (MarcoFalke)
fa6cbdc3c9 ci: Use ./ci/ on non-travis host (MarcoFalke)
fa31bc35eb ci: Remove dependence on travis, use it as fallback env (MarcoFalke)
fa0aac0f43 ci: Add retry (MarcoFalke)
fafe78f6ae ci: Rename .travis/ to ./ci/ (MarcoFalke)
Pull request description:
This moves the `.travis` folder to `ci` and removes dependence on travis, so that the test script can be run anywhere.
Top commit has no ACKs.
Tree-SHA512: 4d8c82f3eb4e9e047444b0e0f700485e929a3c4d27fc8777a95b8847f23ed507d2701cc92730198b14d1e753cbb558ffac883da558fc2ec72b8a12c4eaec9000
582d2cd747 Cover UTXO set access with lock annotations (James O'Beirne)
5693530685 refactor: have CCoins* data managed under CChainState (James O'Beirne)
fae6ab6aed refactor: pcoinsTip -> CChainState::CoinsTip() (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change encapsulates UTXO set data within CChainState instances, removing global data `pcoinsTip` and `pcoinsviewdb`. This is necessary if we want to maintain multiple chainstates with their own rendering of the UTXO set.
We introduce a class CoinsViews which consolidates the construction of a CCoins* hierarchy.
This commit could be broken into smaller pieces, but it would require more ephemeral diffs to, e.g., temporarily change CCoinsViewDB's constructor invocations.
ACKs for top commit:
Sjors:
reACK 582d2cd747
MarcoFalke:
ACK 582d2cd747
Tree-SHA512: ec9d904fe5dca8cd2dc4b7916daa5d8bab30856dd4645987300f905e0a19f9919fce4f9d1ff03eda982943ca73e6e9a746be6cf53b46510de36e8c81a1eafba1
i.e. any CoinsViews members. Adds a lock acquisition to `gettxoutsetinfo` RPC
to comply with added annotations.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
This change encapsulates UTXO set data within CChainState instances, removing
global data `pcoinsTip` and `pcoinsviewdb`. This is necessary if we want to
maintain multiple chainstates with their own rendering of the UTXO set.
We introduce a class CoinsViews which consolidates the construction of a
CCoins* hierarchy. Construction of its various pieces (db, coinscatcher,
in-memory cache) is split up so that we avoid flushing bad state to disk if
startup is interrupted.
We also introduce `CChainState::CanFlushToDisk()` which tells us when it is
safe to flush the chainstate based on this partial construction.
This commit could be broken into smaller pieces, but it would require more
ephemeral diffs to, e.g., temporarily change CCoinsViewDB's constructor
invocations.
Other changes:
- A parameter has been added to the CCoinsViewDB constructor that allows the
name of the corresponding leveldb directory to be specified.
Thanks to Russell Yanofsky and Marco Falke for helpful feedback.
fa3c6575ca lint: Add false positive to python dead code linter (MarcoFalke)
fa25668e1c test: Test p2sh-witness and bech32 in wallet_import_rescan (MarcoFalke)
fa79af2989 test: Replace fragile "rng" with call to random() (MarcoFalke)
fac3dcf7d0 test: Generate one block for each send in wallet_import_rescan (MarcoFalke)
Pull request description:
This adds test coverage for segwit in the `wallet_import_rescan` test, among other cleanups.
ACKs for top commit:
jnewbery:
ACK fa3c6575ca
Tree-SHA512: 877741763c62c1bf9d868864a1e3f0699857e8c028e9fcd65c7eeb73600c22cbe97b7b51093737743d9e87bcb991c1fe1086f673e18765aef0fcfe27951402f0
