c279a81e9c gui: Remove warning "unused variable 'wallet_model'" (João Barbosa)
Pull request description:
This was part of the abandoned #15150.
ACKs for top commit:
theStack:
utACK c279a81e9c
fanquake:
ACK c279a81e9c - tested wallet loading/unloading in the qt rpc console.
Tree-SHA512: 8fbd55c7e213599c7be843b52e960a16cf965b3e01489f426ac3ed9d579d78bb4b2ac230bcccd8abe0397a8b1166ee10e0d685738441a77a5dcb5135c15790fa
22c5a986e9 depends: Consistent use of package variable (Peter Bushnell)
Pull request description:
All other mk files use the package variable consistently except for the two instances here, which have always been here, since depends was introduced in 0.10.
ACKs for top commit:
fanquake:
ACK 22c5a986e9 - tested a `make boost -C depends/ -j8`.
Tree-SHA512: 41766a328603db2ebb1f23ea0c5b2936de043587dd86396eaba73524d2f5bdeff25447040e33d61de2ef612a920281cd81c6fac097913270287f344beb839c5d
Identified via -Wdocumentation, e.g.:
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
2b1641492f wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)
Pull request description:
Improve `CWallet:MarkDestinationsDirty` by skipping transactions that already have the cache invalidated. Skipping a transaction avoids at worst case extracting all output destinations.
ACKs for top commit:
meshcollider:
re-utACK 2b1641492f
Tree-SHA512: 479dc2dde4b653b856e3d6a0c59a34fe33e963eb131a2d88552a8b30471b8725a087888fe5d7db6e4ee19b74072fe64441497f033be7d1931637f756e0d8fef5
fac86ac7b3 scripted-diff: Add missed copyright headers (Hennadii Stepanov)
6fde9d5e47 script: Update EXLUDE list in copyright_header.py (Hennadii Stepanov)
1998152f15 script: Add empty line after C++ copyright (Hennadii Stepanov)
071f2fc204 script: Add ability to insert copyright to *.sh (Hennadii Stepanov)
Pull request description:
This PR improves `contrib/devtools/copyright_header.py` script and adds copyright headers to the files in `src` and `test` directories with two exceptions:
- [`src/reverse_iterator.h`](https://github.com/bitcoin/bitcoin/blob/master/src/reverse_iterator.h) (added to exceptions)
- [`src/test/fuzz/FuzzedDataProvider.h`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/FuzzedDataProvider.h) (added to exceptions)
On master 5622d8f315:
```
$ ./contrib/devtools/copyright_header.py report . | grep zero
25 with zero copyrights
```
With this PR:
```
$ ./contrib/devtools/copyright_header.py report . | grep zero
2 with zero copyrights
```
~I am uncertain about our copyright policy with `build_msvc` and `contrib` directories content, so they are out of scope of this PR.~
ACKs for top commit:
MarcoFalke:
ACK fac86ac7b3
Tree-SHA512: d7832c4a7a1a3b7806119775b40ec35d7982f49ff0e6199b8cee4c0e0a36e68d51728b6ee9924b1c161df4bc6105bd93391b79d42914357fa522f499cb113fa8
ef63f5fc11 ci: Combine 32-bit build with CentOS 7 build (Sebastian Falbesoner)
Pull request description:
Combines the CentOS build with the 32-bit (i686) build to avoid Travis bottlenecks, as suggested in #17757 by MarcoFalke. This keeps most of the properties of the 32-bit build (dash as config shell, building QT5 GUI) and just builds it with depends inside the CentOS docker container.
Making the depends in `05_before_script.sh` with unset config shell (`CONFIG_SHELL=`)
6196e93001/ci/test/05_before_script.sh (L28)
caused problems for building the library libevent (resulting in a Makefile with no shell set (`SHELL=`)), that's why I set it explicitely to `/bin/bash` if we have a CentOS Docker container.
A Travis output of this 32-bit CentOS build can be seen here: https://travis-ci.org/theStack/bitcoin/jobs/634472394 (has been restarted once due to too long build time and appearance of the `CACHE_ERR_MSG`).
For anyone wanting to verify the outputs, I found these instructions useful to reproduce a Travis build locally: https://github.com/erdc/proteus/wiki/Replicating-the-TravisCI-Environment-on-your-Local-Machine (steps 1-3). In this case it's a bit tricky since you run Docker inside Docker -- within the Travis Docker container, the CentOS Docker container is created. To make this possible, the Docker socket has to be exposed to the Travis container via bind-mounting (`docker run -v /var/run/docker.sock:/var/run/docker.sock ...`), as suggested in https://stackoverflow.com/a/33003273.
Top commit has no ACKs.
Tree-SHA512: af508241cec3a10a66c37673d56691717b78375340e910fcdd3fb3870741eba623a436e1e85b26b54f013375611896f5411c5a7fec2437d367d27172230129fe
1be0b1fb2a test: add functional test for non-standard bare multisig txs (Sebastian Falbesoner)
Pull request description:
Approaches another missing functional test of issue #17394 (counterpart to unit test in PR #17502): A transaction is rejected by the mempool with reason `"bare-multisig"` if any of the outputs' scriptPubKey has bare multisig format (`M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`) and bitcoind is started with the argument `-permitbaremultisig=0`.
ACKs for top commit:
instagibbs:
utACK 1be0b1fb2a
kristapsk:
ACK 1be0b1fb2a
Tree-SHA512: 2cade68c4454029b62278b38d0f137c2605a0e4450c435cdda2833667234edd4406f017ed12fa8df9730618654acbaeb68b16dcabb9f5aa84bad9f1c76c6d476
42ec499489 doc: developer notes guideline on RPCExamples addresses (Jon Atack)
Pull request description:
to make explicit the use of invalid addresses for user safety and to encourage
the use of bech32 addresses by default. See https://github.com/bitcoin/bitcoin/pull/17578#discussion_r361752570 and https://github.com/bitcoin/bitcoin/pull/17578#discussion_r362564492.
Fix a typo to appease the linter.
ACKs for top commit:
promag:
ACK 42ec499489, no strong opinion as whether this belongs to developer notes or not but why not.
fjahr:
ACK 42ec499
michaelfolkson:
ACK 42ec499489
Tree-SHA512: 64f90e227d256aa194c4fd48435440bdc233a51213dd4a6ac5b05d04263f729c6b4bb5f3afd3b87719b20cb1b159d5a9673d58a11b72823a4a6a16e8a26ae10e
6dd59d2e49 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
4b8f1e989f IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)
Pull request description:
Regression introduced in https://github.com/bitcoin/bitcoin/pull/17621 which causes p2sh-segwit addresses to be erroneously missed.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
I'll devise a test case to catch this going forward.
ACKs for top commit:
achow101:
ACK 6dd59d2e49
MarcoFalke:
ACK 6dd59d2
meshcollider:
Code review ACK 6dd59d2e49
Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950
486f51099f gui: hide HD & encryption icons when no wallet loaded (Harris)
Pull request description:
This PR takes care of removing (hiding) the HD wallet and encryption icons when no wallet is loaded.
Fixes#17927
ACKs for top commit:
Sjors:
ACK 486f51099f
theStack:
ACK 486f51099f
fanquake:
ACK 486f51099f - tested that this fixes#17927. Thanks for following up so quick.
emilengler:
ACK 486f510
Tree-SHA512: 6e3e5305a9eefe1692614097c05393aa0dffd561c89cefb40d501e70a8102eafcadfbc1c86a35c0b256b0f94f41598545d7a043954d6b9669c169d31d95aaf24
All other mk files use the package variable consistently except for the two instances here, which have always been here, since depends was introduced in 0.10.
f117fb00da Replace coroutine with async def in p2p_invalid_messages.py (Elichai Turkel)
Pull request description:
In Python 3.8 `p2p_invalid_messages.py` fails because of the following warning python produce:
```
2020-01-15T13:02:14.486000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_3xq0f6uh
./test/functional/p2p_invalid_messages.py:154: DeprecationWarning: "@coroutine" decorator is deprecated since Python 3.8, use "async def" instead
asyncio.run_coroutine_threadsafe(asyncio.coroutine(swap_magic_bytes)(), NetworkThread.network_event_loop).result()
2020-01-15T13:02:15.306000Z TestFramework (INFO): Sending a bunch of large, junk messages to test memory exhaustion. May take a bit...
2020-01-15T13:02:17.971000Z TestFramework (INFO): Waiting for node to drop junk messages.
2020-01-15T13:02:18.042000Z TestFramework.mininode (WARNING): Connection lost to 127.0.0.1:12826 due to [Errno 104] Connection reset by peer
2020-01-15T13:02:18.141000Z TestFramework (INFO): Sending a message with incorrect size of 2
2020-01-15T13:02:18.293000Z TestFramework (INFO): Sending a message with incorrect size of 77
2020-01-15T13:02:18.344000Z TestFramework.mininode (WARNING): Connection lost to 127.0.0.1:12826 due to [Errno 104] Connection reset by peer
2020-01-15T13:02:18.445000Z TestFramework (INFO): Sending a message with incorrect size of 78
2020-01-15T13:02:18.597000Z TestFramework (INFO): Sending a message with incorrect size of 79
2020-01-15T13:02:18.902000Z TestFramework (INFO): Stopping nodes
2020-01-15T13:02:19.154000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_3xq0f6uh on exit
2020-01-15T13:02:19.154000Z TestFramework (INFO): Tests successful
```
so as it says I replaced the co-routine with `async def` which IIUC is supported since Python 3.5, so this makes the test pass both on 3.5+ and on 3.8
https://docs.python.org/3.5/library/asyncio-task.html ("The async def type of coroutine was added in Python 3.5, and is recommended if there is no need to support older Python versions")
ACKs for top commit:
laanwj:
ACK f117fb00da if it passes travis
fanquake:
ACK f117fb00da - observed the failure (it's the only test that fails) with Python 3.8.1, tested the fix with 3.5.6 and 3.8.1. This is our only usage of `asyncio.coroutine`.
Tree-SHA512: c21d50b23ef4d8a777fd1d9dfe433c85b0b5fff35afbd338817021ffcd42caea64b4c70e46cb3a8a543a1bf2aaa9a6b4f075f6493ab64192bc12bf8bafc54a87
6fc554f591 wallet: Reset reused transactions cache (Fabian Jahr)
Pull request description:
Fixes#17603 (together with #17824)
`getbalances` is using the cache within `GetAvailableCredit` under certain conditions [here](35fff5be60/src/wallet/wallet.cpp (L1826)). For a wallet with `avoid_reuse` activated this can lead to inconsistent reporting of `used` transactions/balances between `getbalances` and `listunspent` as pointed out in #17603. When an address is reused before the first transaction is spending from this address, the cache is not updated even after the transaction is sent. This means the remaining outputs at the reused address are not showing up as `used` in `getbalances`.
With this change, any newly incoming transaction belonging to the wallet marks all the other outputs at the same address as dirty.
ACKs for top commit:
kallewoof:
Code review re-ACK 6fc554f591
promag:
ACK 6fc554f591.
achow101:
Re-ACK 6fc554f591
meshcollider:
Code review ACK 6fc554f591
Tree-SHA512: c4cad2c752176d16d77b4a4202291d20baddf9f27250896a40274d74a6945e0f6b34be04c2f9b1b2e756d3ac669b794969df8f82a98e0b16f10e92f276649ea2
5855cc564f bitcoin-wallet: Use PACKAGE_NAME in usage help (Luke Dashjr)
7f5db163a4 GUI: Use PACKAGE_NAME in modal overlay (Luke Dashjr)
Pull request description:
ACKs for top commit:
hebasto:
ACK 5855cc564f, checked with
fanquake:
ACK 5855cc564f - checked `bitcoin-wallet` and a `--disable-wallet` `bitcoin-qt`.
Tree-SHA512: 3526eb122bfdbc63349d12251f17ffa20c7f3754af4ac9c554e6d36bb14b351f31c413c30401bb3d6e0e6200b72614dfc8475489b1f742b0423bd83fba758b94
e09c701e01 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe620964 scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
ACKs for top commit:
Empact:
ACK e09c701e01 diff and scripts look correct
promag:
ACK e09c701e01
practicalswift:
ACK e09c701e01 -- scripted diff looks correct
Tree-SHA512: 4bd7b5de1befdcf91dc8f43c127a1fee49679e06895a43216f160344a395c8e426dc68d529fbd2d5e1c215625a5a392dc415b1bce4127316aae7ecf98030c855
The macos manpage for fcntl (for F_PEOFPOSMODE) states:
> Allocate from the physical end of file. In this case, fst_length indicates the number of newly allocated bytes desired.
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
When prune checkbox is toggled, the related text labels and the amount
of required space shown are updated (previously they were only updated
when the data directory was updated).
8313fa8e81 gui: Set CConnman byte counters earlier to avoid uninitialized reads (Russell Yanofsky)
Pull request description:
Initialize CConnman byte counters during construction, so GetTotalBytesRecv() and GetTotalBytesSent() methods don't return garbage before Start() is called.
Change shouldn't have any effect outside of the GUI. It just fixes a race condition during a qt test that was observed on travis: https://travis-ci.org/bitcoin/bitcoin/jobs/634989685
ACKs for top commit:
MarcoFalke:
ACK 8313fa8e81
promag:
ACK 8313fa8e81.
Tree-SHA512: 97c246da4e28e6e0b48f685b840f96746ad75c4b157a692201c6c4702db328a88ead8507d8e1b4e608aa1882513174ec60cf3977c31b7a9d76678cc9f49b45f8
This adds the (internal) Wrapper class, and the Using function that uses it. Given
a class F that implements Ser(stream, const object&) and Unser(stream, object&)
functions, this permits writing e.g. READWRITE(Using<F>(object)).
If a destination is reused we mark the cache of the other transactions going to that destination dirty so they are not accidentally reported as trusted when the cache is hit.
02b9511d6b tests: add tests for GetCoinsCacheSizeState (James O'Beirne)
b17e91d842 refactoring: introduce CChainState::GetCoinsCacheSizeState (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/master/proposal
---
This pulls out the routine for detection of how full the coins cache is from
FlushStateToDisk. We use this logic independently when deciding when to flush
the coins cache during UTXO snapshot activation ([see here](231fb5f17e (diff-24efdb00bfbe56b140fb006b562cc70bR5275))).
ACKs for top commit:
ariard:
Code review ACK 02b9511.
ryanofsky:
Code review ACK 02b9511d6b. Just rebase, new COIN_SIZE comment, and new test message since last review
Tree-SHA512: 8bdd78bf68a4a5d33a776e73fcc2857f050d6d102caa4997ed19ca25468c1358e6e728199d61b423033c02e6bc8f00a1d9da52cf17a2d37d70860fca9237ea7c
831e1220bc build: remove double LIBBITCOIN_SERVER linking (fanquake)
Pull request description:
Seems that this is no longer required. Have tested building on macOS and Debian.
ACKs for top commit:
promag:
ACK 831e1220bc.
practicalswift:
ACK 831e1220bc
laanwj:
ACK 831e1220bc
Tree-SHA512: d226d9fa0292189fae7e2af14781a511c3633f1352324f19ae642e941d06c34e2abf8b1df97d2330d76dba6024a93d8d341e02cc4882d7066f97e82585631fe1
498cdbb426 Fix improper Doxygen inline comments (Ben Woosley)
Pull request description:
The proper syntax is `//!<`
http://www.doxygen.nl/manual/docblocks.html#memberdoc
Identified via `-Wdocumentation`:
```
In file included from ./util/system.h:26:
./util/settings.h:74:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
const SettingsValue* begin() const; //<! Pointer to first non-negated value.
^~~~
///<
./util/settings.h:75:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
const SettingsValue* end() const; //<! Pointer to end of values.
^~~~
///<
./util/settings.h:76:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
bool empty() const; //<! True if there are any non-negated values.
^~~~
///<
./util/settings.h:77:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
bool last_negated() const; //<! True if the last value is negated.
^~~~
///<
./util/settings.h:78:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
size_t negated() const; //<! Number of negated values.
^~~~
///<
```
ACKs for top commit:
fanquake:
ACK 498cdbb426
Tree-SHA512: 2851fc1cbbcf700d198d82ce4923b2ef4a700f8ce19dff431ecf24f4e6fecda9fed1b4b4d148f3c1adfb6b0c6bff5d5315ee01bbcd855eb3d83e1a69b0c98893
8b2f471a1b qa: Fix double-negative arg test (Hennadii Stepanov)
Pull request description:
Commit 67518f7cc61bf59ddfa0fd7c8dbbdec3653b9556 tests do not catch that a pointer is returned instead of a value.
This PR makes test to not accept trailing characters after 0.
From [IRC](http://www.erisian.com.au/bitcoin-core-dev/log-2020-01-07.html#l-358):
> \<hebasto\> ryanofsky: hmm, why test/functional/feature_config_args.py passed on 67518f7cc61bf59ddfa0fd7c8dbbdec3653b9556 ?
> \<hebasto\> I see now: test is broken.
> \<ryanofsky\> test should be unaffected by that change, do you see a break somewhere?
> \<hebasto\> yes: "-connect=0x7fff50369968" != "-connect=0"
> ...
> \<ryanofsky\> Oh I see how that would happen, it should not be a problem in the current PR.
> \<hebasto\> going to submit a pr to fix test
> \<ryanofsky\> in the commit you mentioned, value is a pointer to a string, and it was printing the pointer address instead of the string on: LogPrintf("Warning: parsed potentially confusing double-negative -%s=%s\n", key, value);
> \<hebasto\> correct
> \<ryanofsky\> oh I see, test could be fixed to more robust and not accept trailing characters after 0
ACKs for top commit:
ryanofsky:
Code review ACK 8b2f471a1b. I don't know how you found this but it's a nice catch! This change should make the test more reliable.
Tree-SHA512: 454b3d4415771d353a2da766f6ae6e0bfae7bdf485aaa7bfdd323595282356eeaf3f40e556b39f753bc35f578cbe9684368887eef2d63c5d7f0d7d9fa971697a
0874a109da Ignore msvc linker warning and update to msvc build instructions. (Aaron Clauson)
Pull request description:
- Update Visual Studio instructions.
- Remove x64 platform conditional from bitcoin-qt project configuration.
- Set use native environment toolset to fix linker warning.
- Ignore linker warning about precompiled type information missing for test_bitcoin_qt.
ACKs for top commit:
fanquake:
ACK 0874a109da - tested building `bitcoind` and `bitcoin-qt`. Didn't open anything in Visual Studio.
Tree-SHA512: 83a4e4dfb8a52b024feadbf06bb1bf87993b6ebcb2a1b7dc3e2385815400f0beffc43591408b4abc8b6ffa406ce066c0af5028e7f53c707dca88ea5bba18346c
Initialize CConnman byte counters during construction, so GetTotalBytesRecv()
and GetTotalBytesSent() methods don't return garbage before Start() is called.
Change shouldn't have any effect outside of the GUI. It just fixes a race
condition during a qt test that was observed on travis:
https://travis-ci.org/bitcoin/bitcoin/jobs/634989685
