f3b57f4a1c17aadbf02d408e980490c88838c6ba Unrecommend making config file owned by bitcoin (setpill)
870d4152dfc3d990e336723562948835c2dbd646 Set ProtectHome in systemd service file (setpill)
639a416e3758b3005b860b198f0ec7bdd80a7f0c Chgrp config dir to bitcoin in systemd service (setpill)
aded0528f0e1e3735ce8dd26fd9e546150b73187 Improve clarity of systemd service file comments (setpill)
Pull request description:
Rationale: ran into a bug with the systemd service file, fixed it locally and figured I might as well contribute my fix.
Also fixed some unrelated confusing phrasing in the comments of the same file, after discussion in IRC.
ACKs for top commit:
sipsorcery:
tACK f3b57f4a1c17aadbf02d408e980490c88838c6ba (nothing changed since previous tACK).
ryanofsky:
utACK f3b57f4a1c17aadbf02d408e980490c88838c6ba. Only change since last review is removing ConfigurationDirectoryMode churn in early commits
Tree-SHA512: 2188345878925b9e8a5c2c3df8dfba443720e2252a164db54a8e1d8007846721497b2d98c56f1d9b60a9a9ed4fdb1156c7b02c699616b220a9b614671617d32a
1373fa7e3d3f04ce6938cdcd2124cba71ff82ca0 doc: add default bitcoin.conf locations (Chuf)
Pull request description:
Added default bitcoin.conf data directories and paths
ACKs for top commit:
practicalswift:
ACK 1373fa7e3d3f04ce6938cdcd2124cba71ff82ca0
ryanofsky:
ACK 1373fa7e3d3f04ce6938cdcd2124cba71ff82ca0
fanquake:
ACK 1373fa7e3d3f04ce6938cdcd2124cba71ff82ca0 - Already three ACKs and lots of discussion here, so I'm going to merge, and the other comment
Tree-SHA512: 8bb1ed9868c5d171b6791bd6dc9598eddfdf64977d327ff4f333323cef8e3e76b1a67da21e4199f008a12f5610ac6dc6f34f4a13235e8846754eb6d6e5075da4
9452802480bd154e23771230bbdfebde1dbaa941 doc: Tidy up shadowing section (João Barbosa)
Pull request description:
Removes the example because it violates the code format.
ACKs for top commit:
MarcoFalke:
unsigned ACK 9452802480bd154e23771230bbdfebde1dbaa941
ryanofsky:
ACK 9452802480bd154e23771230bbdfebde1dbaa941
fanquake:
ACK 9452802480bd154e23771230bbdfebde1dbaa941 - Thanks for following up.
Tree-SHA512: 1fc31355d368225713298da7803e39e99014fbfcd229f2d3b56c082de95ab2965e51c80b172a5abce4646c53f845fa62a6d94d5df714e7835cac07a8ec7d5da7
aff1c9c8845e7762e9710008998555a72045e40c Adds an option to msbuild common configuration to ignore linker warning 4221. This warning is for object files that do not include any symbols. The warning is harmless and occurs due to some classes that are *nix only having no source to compile for an msvc build. (Aaron Clauson)
Pull request description:
Adds an option to msbuild common configuration to ignore linker warning 4221. This warning is for object files that do not include any symbols. The warning is harmless and occurs due to some classes that are *nix only having no source to compile for an msvc build.
ACKs for top commit:
MarcoFalke:
ACK aff1c9c8845e7762e9710008998555a72045e40c
hebasto:
ACK aff1c9c8845e7762e9710008998555a72045e40c, I have reviewed the code, it looks OK and I agree it can be merged.
fanquake:
ACK aff1c9c8845e7762e9710008998555a72045e40c
Tree-SHA512: fda3721462d927d8734e16fc0250c7c76b197a085219a9e5267cdd14e19f23f50807d6b5516dee8402b60f3fdf188677e3d618fe6acbdbb5ac3bb635aeb7b297
e4f4ea47ebf7774fb6f445adde7bf7ea71fa05a1 lint: Catch use of [] or {} as default parameter values in Python functions (practicalswift)
25dd86715039586d92176eee16e9c6644d2547f0 Avoid using mutable default parameter values (practicalswift)
Pull request description:
Avoid common Python default parameter gotcha when mutable `dict`/`list`:s are used as default parameter values.
Examples of this gotcha caught during review:
* https://github.com/bitcoin/bitcoin/pull/16673#discussion_r317415261
* https://github.com/bitcoin/bitcoin/pull/14565#discussion_r241942304
Perhaps surprisingly this is how mutable list and dictionary default parameter values behave in Python:
```
>>> def f(i, j=[], k={}):
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1, 1], {1: True})
>>> f(2)
([1, 1, 2], {1: True, 2: True})
```
In contrast to:
```
>>> def f(i, j=None, k=None):
... if j is None:
... j = []
... if k is None:
... k = {}
... j.append(i)
... k[i] = True
... return j, k
...
>>> f(1)
([1], {1: True})
>>> f(1)
([1], {1: True})
>>> f(2)
([2], {2: True})
```
The latter is typically the intended behaviour.
This PR fixes two instances of this and adds a check guarding against this gotcha going forward :-)
ACKs for top commit:
Sjors:
Oh Python... ACK e4f4ea47ebf7774fb6f445adde7bf7ea71fa05a1. Testing tip: swap the two commits.
Tree-SHA512: 56e14d24fc866211a20185c9fdb274ed046c3aed2dc0e07699e58b6f9fa3b79f6d0c880fb02d72b7fe5cc5eb7c0ff6da0ead33123344e1a872209370c2e49e3f
403e372407db1d020eedede4d322ee79d4a85dfc qa: Relax so that the subscriber is ready before publishing zmq messages (João Barbosa)
Pull request description:
Prevents the syndrome "slow joiner" - see http://zguide.zeromq.org/py:all#sockets-and-patterns - by relaxing before publishing messages.
ACKs for top commit:
MarcoFalke:
unsigned ACK 403e372407db1d020eedede4d322ee79d4a85dfc
Tree-SHA512: 0e856accbc450a9b09160bdce5112b2103dc9436cc317d31fb1c9634ebd76823a300a2e727818057fb4d0a615271772ff23e80553a13e9aa1935500de5eeec5f
8a3b2eb17572ca2131778d52cc25ec359470a90f move-only: move coins statistics utils out of RPC (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
---
In the short-term, this move-only commit will help with fuzzing (https://github.com/bitcoin/bitcoin/pull/15606#issuecomment-524482297). Later, these procedures will be used to compute statistics (particularly a content hash) for UTXO sets coming in from snapshots.
Most easily reviewed with `git ... --color-moved=dimmed_zebra`. A nice follow-up would be adding unittests, which I'll do if nobody else gets around to it.
ACKs for top commit:
MarcoFalke:
ACK 8a3b2eb17572ca2131778d52cc25ec359470a90f, checked --color-moved=dimmed-zebra
Tree-SHA512: a187d2f7590ad2450b8e8fa3d038c80a04fc3d903618c24222d7e3172250ce51badea35860c86101f2ba266eb4354e6efb8d7d508b353f29276e4665a1efdf74
These procedures will later be used in the ChainstateManager to compute
statistics (particularly a content hash) for UTXO sets coming in from
snapshots.
eb2d64b9e0e2539b87469d311fb1affcb4fa26cf doc: Add spacing to command in translation_process.md (Chuf)
Pull request description:
Add spacing to the command used for updating `bitcoin_locale.qrc` entries. This makes it slightly easier to copy output into the relevant Makefile.
ACKs for top commit:
fanquake:
Tested ACK - eb2d64b9e0e2539b87469d311fb1affcb4fa26cf - thanks.
Tree-SHA512: 079180962062994a3127ba05f7bb27a4145cb6a50354076864bf546dbbd80802ba9f10c943c43b6b3544d7225b646b7a8f1e38e0f148902c1813426b6f5036c3
a64dbeffe486b2cd247ba160ebce1c0c1c7b9e72 doc: Update labels in CONTRIBUTING.md (Hennadii Stepanov)
Pull request description:
This PR:
- adds `build` label
- makes labels lowercase (in accordance to current customs in this repo); also a lowercase label improves readability of PR title itself, e.g.,
```
doc: Update labels in CONTRIBUTING.md
```
reads better than
```
Doc: Update labels in CONTRIBUTING.md
```
- improves label names readability
- splits long labels (as suggested by **jonatack**)
ACKs for top commit:
practicalswift:
ACK a64dbeffe486b2cd247ba160ebce1c0c1c7b9e72
fanquake:
ACK a64dbeffe486b2cd247ba160ebce1c0c1c7b9e72
Tree-SHA512: f82e5e357490978f41035763b86cc005cf4a94099012cd0b246b664e00567ac7fa7a604bbb466b8318d94568084e3c67eecff5df152e0f39997d0a2fd9cfee77
92528c260e9bd5d36da4b017098a49fda5051154 Support serialization of std::vector<bool> (Pieter Wuille)
Pull request description:
This adds support for serialization of `std::vector<bool>`, as a prerequisite of #16702.
ACKs for top commit:
MarcoFalke:
ACK 92528c260e9bd5d36da4b017098a49fda5051154 (only looked at the diff on GitHub)
practicalswift:
ACK 92528c260e9bd5d36da4b017098a49fda5051154 -- diff looks correct
jamesob:
ACK 92528c260e
Tree-SHA512: 068246a55a889137098f817bf72d99fc3218a560d62a97c59cccc0392b110f6778843cee4e89d56f271ac64e03a0f64dc5e2cc22daa833fbbbe9234c5f42d7b9
66ad75472f47e219ff980f7aba249795403dd7e0 [Doc] Add documentation for the new whitelist permissions (nicolas.dorier)
Pull request description:
Documenting the new feature https://github.com/bitcoin/bitcoin/pull/16248 . Ping Sjors .
ACKs for top commit:
Sjors:
Indeed, re-ACK 66ad75472
Tree-SHA512: e6860bb6fae921287da7920a8db534e6a1a23871dd78dd6da030f00adf23e204cd23b194d67361bf34d4ef5a7815fc3fd7c81a3f2f35e4cfbe6ee2f2e6daec25
3d50fe2c1fb566501257fc16d4606e06f518a0bb scripts: move update-translations.py to maintainer-tools repo (fanquake)
Pull request description:
As mentioned in [#16644](https://github.com/bitcoin/bitcoin/pull/16644#issuecomment-522481111), move the `update-translations.py` script out of the `bitcoin/bitcoin` repo. This script is run infrequently by maintainers, and aside from #16644, doesn't see many changes. If it is going to be changed, it probably shouldn't consume review bandwidth in bitcoin/bitcoin anyways.
PR adding the script to the maintainer-tools repo: https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/31
Could also move some other scripts, such as [`gen-manpages.sh`](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/gen-manpages.sh).
ACKs for top commit:
laanwj:
ACK 3d50fe2c1fb566501257fc16d4606e06f518a0bb
Tree-SHA512: b68da68e2df0c1b3275729890a02726af602eb398507f0a03387a313a73e7629a06db57f3472a1dbcf9e11506e88a8f5f725a397a9ce6afef7627188c88d3d48
c295cba5a2f934e51a7c8610ab4c58b8e9d56619 depends: zeromq: disable draft classes and methods (fanquake)
0072237b9e33e0b89f6c9f51dd0b946fa89a6134 depends: xproto: configure flags cleanup (fanquake)
6a8ada3a4f67affcf0ef7452e206083d7b58b2bc depends: qrencode: configure flags cleanup (fanquake)
86beb8cdc4e312bd0bed2cbb273aebb792be2747 depends: fontconfig: configure flags cleanup (fanquake)
e656d95ec74336c2bd93bd387f67aeb6aed4dc40 depends: libxcb: configure flags cleanup (fanquake)
e439388b352b7dfbf2e00c6ba2970fed0a4a5554 depends: libXau: configure flags cleanup (fanquake)
Pull request description:
Related to #16354.
This PR adds additional configure flags to packages in depends to explicitly disable features we aren't using; similar to #16183. It also fixes passing `--without-tools` to `qrencode`.
I've added `--disable-drafts` to `zeromq`:
```bash
Build and install draft classes and methods [default=yes]
```
I'm not entirely sure how far we want to take this. i.e in the `zeromq` package we explicitly pass `--without-libsodium`, even though it's disabled by default.
Do we also want to explicitly pass all the other `--without` flags? :
```bash
--with-libgssapi_krb5 require libzmq build with libgssapi_krb5
[default=no]
--with-libsodium use libsodium instead of built-in tweetnacl
[default=no]
--with-pgm build libzmq with PGM extension. Requires pkg-config
[default=no]
--with-norm build libzmq with NORM protocol extension,
optionally specifying norm path [default=no]
--with-vmci build libzmq with VMCI transport [default=no]
```
ACKs for top commit:
dongcarl:
ACK c295cba5a2f934e51a7c8610ab4c58b8e9d56619
Tree-SHA512: df6d38b863b4008ed2cb06c97eb0e21eaa4b5fde552876065ba7f3c87bf6e372e5b954a51bf3fde2151cfb6d2c022227d34337fc6e50ce0caa1d518abbd2412a
74e38769895c643d75f77634519c1442fa38bad6 Release notes: add previously undocumented changes (David A. Harding)
7e1634a927f86dcffc7e35fdd479d19c414b14ab Release notes: edit previously-detached notes (David A. Harding)
e7415a5a95f53abf1f8ffa2085217b249d7caf61 Doc: move detached release notes into release-notes.md (David A. Harding)
Pull request description:
Merges in detached release notes, edits each change down to a single paragraph bullet point (or, in a couple cases, two individual bullet points in separate sections each with a single paragraph). Adds notes for some undocumented changes I found reviewing `git log --merges`. Also tries something new: adds the PR number(s) after each entry to make it easier for both reviewers and end-user readers to look up the details behind each change. (If the PR numbers are unwanted, they're easy to remove either in this PR or later in the release process.)
I also checked the 0.18 branch but I didn't find anything in the current release notes that had been backported.
A particular focus in my editing was trying to keep things concise, particularly by pointing to RPC documentation when available (or upcoming, as in #16629). I do suspect that one downside of detached notes is that people write longer summaries than they would if they knew there were already 300 other lines of release notes. :-)
The first commit only moves notes, puts them in bullet form, adjusts indentation appropriately, and drops unneeded headers. It can be reviewed with `git diff --color-moved=dimmed-zebra` for a little bit of a speedup, but unfortunately I wasn't smart enough to split my copy/pasting and line wrapping into separate commits, so it's not a transparently move-only change.
ACKs for top commit:
fanquake:
ACK 74e38769895c643d75f77634519c1442fa38bad6 - Thanks for doing this. It's nice to have a gauge of what's going to be in the `0.19.0` release.
meshcollider:
ACK 74e38769895c643d75f77634519c1442fa38bad6
Tree-SHA512: 676668765849d5a67520dd8ac49de85ac1bfb5ba2dc09504e75db77d79c7e2c58b5cee16c58591ec575cb3682e630231baba7fd07565d19f8d02243e06fcb9ab
a4765bd77f9bc5c4d5c866ee6940807522741941 gui: Ensure tx send error highlight is visible (bpay)
Pull request description:
Rebased and squashed #14956.
> If sending to multiple recipients and one of the recipient fields is malformed, the highlighted field may not be visible due to being scrolled out of view. This results in a confusing lack of error feedback.
> Avoid this problem by ensuring the first field containing an error is scrolled into view when Send is clicked.
You can see the behavior here: https://imgur.com/a/QZG5TQc
How to test:
Add a few recipients and give any of them an invalid address or amount. Scroll the invalid recipient out of view and hit Send. With this change, the GUI will scroll to show the invalid recipient, with master it will not, "hiding" the error.
ACKs for top commit:
jonatack:
Tested ACK a4765bd77f9bc5c4d5c866ee6940807522741941 on Linux Debian with Qt 5.11.3. Change is that I had added an inadvertent typo in my make bash alias; fixed.
hebasto:
ACK a4765bd77f9bc5c4d5c866ee6940807522741941, tested on Debian 9.9 with system Qt 5.7.1.
Tree-SHA512: a5653ca44d6d540214bdb424b0b75a06a5872cff41b0cd8cffd9cef99ebf04a17a3652e561139ac75315b39c3347e5f7ae304fa35e14b48bdae4768a416df9b0
If sending to multiple recipients and one of the recipient fields is malformed,
the highlighted field may not be visible due to being scrolled out of view,
leading to a confusing lack of error feedback when clicking Send. To avoid this
problem ensure the first field containing an error is scrolled into view
when Send is clicked.
2483266c591f7b2e62df68ee2d13740a706415ec packages.md: document depends build targets (Russell Yanofsky)
be27161ee4bb7cb63346f1e79fc36ce33103a635 Clarify need to specify --prefix with depends (Russell Yanofsky)
Pull request description:
There seems to be some confusion about exactly how to use depends, when to pass a prefix to `./configure` etc (see #16367, #16654).
I've cherry-picked two of russ's commits out of #16367, as they are clear stand-alone improvements and we don't have to wait for #16367 to improve the depends documentation.
ACKs for top commit:
Sjors:
utACK 2483266
hebasto:
ACK 2483266c591f7b2e62df68ee2d13740a706415ec, I have reviewed the code and it looks OK, I agree it can be merged.
jonasschnelli:
ACK 2483266c591f7b2e62df68ee2d13740a706415ec
Tree-SHA512: a198c288248f573519a3b0ef384626b61cc803803280af9a448c28466e3d9949bed0332af6618dac19e81c5a6e9694afa83d976b176fd13c32a6c2c3fea3fc1f
bca388db0da3d40953ff65d03da62ca45a7951a6 gui: remove unused PlatformStyle::TextColorIcon (fanquake)
Pull request description:
This is unused after #16612.
ACKs for top commit:
emilengler:
Concept ACK bca388d
practicalswift:
ACK bca388db0da3d40953ff65d03da62ca45a7951a6 -- diff looks correct & less cruft is better
hebasto:
ACK bca388db0da3d40953ff65d03da62ca45a7951a6, I have reviewed the code and it looks OK, I agree it can be merge.
jonasschnelli:
utACK bca388db0da3d40953ff65d03da62ca45a7951a6
Tree-SHA512: ae5c5cfc02cf2e6f138e75e227320f4c430b93bd529a8ead7ba971ceb5bbc997cdb721c6fd8a46a5a897008f0bc6e519422d6beef240b9d2e28ecf4f8cbbcc6a
f7220e85fd7df95ca98c7b7b262b30751f70da2f build: update RapidCheck Makefile (Jon Atack)
Pull request description:
- update RapidCheck to the latest commit on master
- install only the extras needed by Bitcoin Core, e.g. ENABLE_BOOST_TEST instead of INSTALL_ALL_EXTRAS
- remove cmake warning by providing `-B` arg:
```
CMake Warning:
No source or binary directory provided. Both will be assumed to be the
same as the current working directory, but note that this warning will
become a fatal error in future CMake releases.
```
Tested with `cd depends && make RAPIDCHECK=1` on Linux Debian.
ACKs for top commit:
fanquake:
ACK f7220e85fd7df95ca98c7b7b262b30751f70da2f - Tested building using depends. `ENABLE_BOOST_TEST` instead of `INSTALL_ALL_EXTRAS` seems fine.
Tree-SHA512: 8033aa25014a87125cd0aa0885a892649039ef888688f4a855d23af21a350792a4081b181b8222ed293b5070dff39c438e9c3f80294eda0c931076a8153403ea
b9ee63c71b751fd67da777774ea8c0b27e7db2f8 Make descriptor test deterministic (David Reikher)
Pull request description:
This is an improvement to a test, inspired by #14343 - removing non determinism from a test.
The test `descriptor_test` is non-deterministic, as it relies on the `MaybeUseHInsteadOfApostrophy` function which randomly either swaps all apostrophes with 'h' or doesn't at all in a descriptor. This fix makes both cases always run, if an apostrophe is found in a test descriptor.
This does not reduce test coverage but removes the non-determinism.
Additionally, the `MaybeUseHInsteadOfApostrophy` function removed the checksum if found at the end of a descriptor when the apostrophes are swapped by 'h's, since after being swapped the checksum is no longer correct. I instead added re-calculation of the checksum using the `DescriptorChecksum` function, which adds coverage for the case of a descriptors having 'h's instead of apostrophes and a checksum. This was previously lacking.
To achieve this I had to move `DescriptorChecksum` and `PolyMod` out of the anonymous namespace in descriptor.cpp to make `DescriptorChecksum` accessible in descriptor_tests.cpp.
All tests complete successfully (functional as well as unit tests).
ACKs for top commit:
achow101:
Code Review ACK b9ee63c71b751fd67da777774ea8c0b27e7db2f8
Tree-SHA512: 992c73a6644a07bfe7c72301ee2666f3c4845a012aaedd7a099a05cea8bdac84fa8280b28e44a7856260c00c0be1a6f1b6768f5694c2a22edf4c489e53fec424
faebf6271467048dc8a9a0c526a0f8565023a966 rpc: Use Join helper in rpc/util (MarcoFalke)
fa8cd6f9c13319baca467864661982a3dfb2320c util: Add Join helper to join a list of strings (MarcoFalke)
Pull request description:
We have a lot of enumerations in the code and sometimes those enumerations need to be mentioned in the RPC or command line documentation. Previously, each caller would have a couple of lines inline to join the strings or the joined string is hardcoded in the documentation. A helper to join strings would make code such as https://github.com/bitcoin/bitcoin/pull/16629#discussion_r315852446 less verbose and easier to read.
Also, warnings commonly accumulate in complex RPCs, since a warning doesn't lead to an early return. A helper to join those warnings would make code such as https://github.com/bitcoin/bitcoin/pull/16394/files#r309324997 less verbose and easier to read.
ACKs for top commit:
practicalswift:
ACK faebf6271467048dc8a9a0c526a0f8565023a966
Tree-SHA512: 80f2db86a05c63b686f510585c1c631250271a8958fd71fafaac91559ffd2ec25d609bf7d53412ba27f87eff5893ac9dd9c2f296fc0c73581556e1d6a734a36f
153d9dd9acffa95bb97a4b1579bd20237fdc9c52 refactor: replace qLowerBound & qUpperBound with std:: upper_bound & lower_bound (fanquake)
59373e3e94015316bcaa03a7b9c2e6f442641720 refactor: replace qSort with std::sort (fanquake)
fea33cbbdfb4673033f3414bf1613591ff654aac refactor: replace qStableSort with std::stable_sort (fanquake)
Pull request description:
`qStablesort`, `qSort`, `qLowerBound` and `qUpperBound` have been marked as obsolete since at least Qt 5.9: [Obsolete Members for QtAlgorithms](https://doc.qt.io/qt-5.9/qtalgorithms-obsolete.html).
This pull request replaces their usage with the suggested `std::` replacements.
This also removes some warning spam when compiling against newer Qt (5.13.0 via brew):
```bash
CXX qt/libbitcoinqt_a-walletcontroller.o
qt/transactiontablemodel.cpp:96:52: warning: 'qLowerBound<QList<TransactionRecord>::iterator, uint256, TxLessThan>' is deprecated: Use std::lower_bound [-Wdeprecated-declarations]
QList<TransactionRecord>::iterator lower = qLowerBound(
qt/transactiontablemodel.cpp:98:52: warning: 'qUpperBound<QList<TransactionRecord>::iterator, uint256, TxLessThan>' is deprecated: Use std::upper_bound [-Wdeprecated-declarations]
QList<TransactionRecord>::iterator upper = qUpperBound(
```
```bash
CXX qt/libbitcoinqt_a-moc_walletcontroller.o
qt/bantablemodel.cpp:64:13: warning: 'qStableSort<QList<CCombinedBan>::iterator, BannedNodeLessThan>' is deprecated: Use std::stable_sort [-Wdeprecated-declarations]
qStableSort(cachedBanlist.begin(), cachedBanlist.end(), BannedNodeLessThan(sortColumn, sortOrder));
```
```bash
CXX qt/libbitcoinqt_a-sendcoinsentry.o
qt/recentrequeststablemodel.cpp:205:5: warning: 'qSort<QList<RecentRequestEntry>::iterator, RecentRequestEntryLessThan>' is deprecated: Use std::sort [-Wdeprecated-declarations]
qSort(list.begin(), list.end(), RecentRequestEntryLessThan(column, order));
```
ACKs for top commit:
hebasto:
ACK 153d9dd9acffa95bb97a4b1579bd20237fdc9c52
promag:
ACK 153d9dd9acffa95bb97a4b1579bd20237fdc9c52.
jonasschnelli:
utACK 153d9dd9acffa95bb97a4b1579bd20237fdc9c52
Tree-SHA512: 22f7290ed798ce8b0f5f313405377845d4c8e48dc8687be7464e27fff53363b451a40e9e18910a8c3b4b9d4dcc236a366c92e7d171fcb8576c48f149a1886c26
6011c9d72d1df5c2cd09de6f85c21eb4f7eb1ba8 QA: fix rpc_setban.py race (Jonas Schnelli)
Pull request description:
The new `rpc_setban.py` test failes regularly on CIs due to a race between injecting the ban and testing the log "on the other side".
The problem is, that the test immediately after the `addnode` command on node0 checks for the `dropped (banned)` entry on node1 (without giving some time).
Adding a 2 seconds sleep seems to solve the race (I guess there is no better event-driven delay).
Example of a failed test: https://bitcoinbuilds.org/index.php?ansilog=bf743910-103f-4b54-9a97-960c471061bd.log#l2906
Top commit has no ACKs.
Tree-SHA512: 680f8ea3e5ddb07e93f824f1aeff4a459e25e6c14715a39fc7670e50506d7cf25925348672c5c2d8ba3e1243ccf5effbc2456bcd094fb96868349f8d26e008f1
2dbfb37b407ed23b517f507d78fb77334142dce5 Fix Char as Bool in interfaces (Jeremy Rubin)
Pull request description:
In a few places in src/wallet/wallet.h, we use a char when semantically we want a bool.
This is kind of an issue because it means we can unserialize the same transaction with different fFromMe flags (as differing chars) and evaluate the following section in wallet/wallet.cpp
```c++
if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe)
{
wtx.fFromMe = wtxIn.fFromMe;
fUpdated = true;
}
```
incorrectly (triggering an fUpdated where both fFromMe values represent true, via different chars).
I don't think this is a vulnerability, but it's just a little messy and unsemantic, and could lead to issues with stored wtxIns not being findable in a map by their hash.
The serialize/unserialize code for bool internally uses a char, so it should be safe to make this substitution.
NOTE: Technically, this is a behavior change -- I haven't checked too closely that nowhere is depending on storing information in this char. Theoretically, this could break something because after this change a tx unserialized with such a char would preserve it's value, but now it is converted to a ~true~ canonical bool.
ACKs for top commit:
achow101:
Code review ACK 2dbfb37b407ed23b517f507d78fb77334142dce5
meshcollider:
Code review ACK 2dbfb37b407ed23b517f507d78fb77334142dce5
Tree-SHA512: 8c0dc9cf672aa2276c694facbf50febe7456eaa8bf2bd2504f81a61052264b8b30cdb5326e1936893adc3d33504667aee3c7e207a194c71d87b3e7b5fe199c9d
