710a7136f9 rpc: Speedup getaddressesbylabel (João Barbosa)
Pull request description:
Fixes#15447. Same approach of #14984, this change avoids duplicate key check when building the JSON response in memory.
ACKs for commit 710a71:
MarcoFalke:
utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d
ryanofsky:
utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d. Just new comments and assert since last review.
Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2
backports bitcoin@fab2950 doc: Switch boost::thread to std::thread in scheduler
After commit d0ebd93 the scheduler itself no longer cares if the
serviceQueue is run in a std::thread or boost::thread. Change the
documentation to std::thread because we switched to C++11.
This backport does not include changes that depend on bitcoin pr 18037
70a6b529f306ff72ea1badf25e970a92b2b17ab3 lint-cppcheck: Remove -DHAVE_WORKING_BOOST_SLEEP_FOR (Anthony Towns)
294937b39de5924e772f8ed90d35c53290c8acab scheduler_tests: re-enable mockforward test (Anthony Towns)
cea19f685915be8affb2203184a549576194413f Drop unused reverselock.h (Anthony Towns)
d0ebd93270758ea97ea956b8821e17a2d001ea94 scheduler: switch from boost to std (Anthony Towns)
b9c426012770d166e6ebfab27689be44e6e89aa5 sync.h: add REVERSE_LOCK (Anthony Towns)
306f71b4eb4a0fd8e64f47dc008bc235b80b13d9 scheduler: don't rely on boost interrupt on shutdown (Anthony Towns)
Pull request description:
Replacing boost functionality with C++11 stuff.
Motivated by #18227, but should stand alone. Changing from `boost::condition_var` to `std::condition_var` means `threadGroup.interrupt_all` isn't enough to interrupt `serviceQueue` anymore, so that means calling `stop()` before `join_all()` is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from `g_lockstack` which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour.
Fixes#16027, Fixes#14200, Fixes#18227
ACKs for top commit:
laanwj:
ACK 70a6b529f306ff72ea1badf25e970a92b2b17ab3
Tree-SHA512: d1da13adeabcf9186d114e2dad9a4fdbe2e440f7afbccde0c13dfbaf464efcd850b69d3371c5bf8b179d7ceb9d81f4af3cc22960b90834e41eaaf6d52ef7d331
# Conflicts:
# src/reverselock.h
# src/rpc/misc.cpp
# src/scheduler.cpp
# src/scheduler.h
# src/sync.cpp
# src/sync.h
# src/test/reverselock_tests.cpp
# src/test/scheduler_tests.cpp
# src/test/test_dash.cpp
# test/lint/extended-lint-cppcheck.sh
* Avoid locking cs_main before logging, instead log, then lock where possible
* use WITH_LOCK in a number of locations as a replacement of simple scopes
* add EXCLUSIVE_LOCKS_REQUIRED
* minimize locked scope in rpc
* fix macOS build error
Signed-off-by: pasta <pasta@dashboost.org>
* add missing EXCLUSIVE_LOCKS_REQUIRED(cs_main)
Signed-off-by: pasta <pasta@dashboost.org>
* revert an rpc scope reduction
* revert an rpc scope reduction
765d5890be Bugfix: dummywallet: Add -ignorepartialspends to list of ignored wallet options (Luke Dashjr)
0f09eb779d dummywallet: Reformat ignored wallet options list (Luke Dashjr)
Pull request description:
When building w/o wallet support, we add all the wallet options as hidden options to avoid throwing errors/warnings that they're unknown.
`-ignorepartialspends` is missing from this list. This PR adds it.
(This seems like a good candidate for a linter? Or maybe we can autogenerate it?)
Also reformats the dummywallet options list across multiple lines to make conflicts less often.
ACKs for commit 765d58:
meshcollider:
utACK 765d5890be
MarcoFalke:
utACK 765d5890be46ed5f5557c1d2b3cb931e6b8eb3b0
promag:
utACK 765d589.
practicalswift:
utACK 765d5890be46ed5f5557c1d2b3cb931e6b8eb3b0
fanquake:
utACK 765d589
Tree-SHA512: 37c8037148bdc1b7a8bde201eff51ee6a64c042c17eb8b6c68faef490d16575348c2f22ab81f48302b8ad80a5559222af23b721a8b5acc1d89c0757fb88796a6
Signed-off-by: pasta <pasta@dashboost.org>
* partial Merge #14454: Add SegWit support to importmulti
c11875c5908a17314bb38caa911507dc6401ec49 Add segwit address tests for importmulti (MeshCollider)
201451b1ca3c6db3b13f9491a81db5b120b864bb Make getaddressinfo return solvability (MeshCollider)
1753d217ead7e2de35b3df6cd6573a1c9a068f84 Add release notes for importmulti segwit change (MeshCollider)
353c064596fc2e2c149987ac3b3c11b4c90c4d5f Fix typo in test_framework/blocktools (MeshCollider)
f6ed748cf045d7f0d9a49e15cc0c0001610b9231 Add SegWit support to importmulti with some ProcessImport cleanup (MeshCollider)
Pull request description:
Add support for segwit to importmulti, supports P2WSH, P2WPKH, P2SH-P2WPKH, P2SH-P2WSH. Adds a new `witnessscript` parameter which must be used for the witness scripts in the relevant situations.
Also includes some tests for the various import types.
~Also makes the change in #14019 redundant, but cherry-picks the test from that PR to test the behavior (@achow101).~
Fixes#12253, also addresses the second point in #12703, and fixes#14407
Tree-SHA512: 775a755c524d1c387a99acddd772f677d2073876b72403dcfb92c59f9b405ae13ceedcf4dbd2ee1d7a8db91c494f67ca137161032ee3a2071282eeb411be090a
# Conflicts:
# src/wallet/rpcdump.cpp
# test/functional/test_framework/blocktools.py
# test/functional/wallet_importmulti.py
* make linter happy
Signed-off-by: pasta <pasta@dashboost.org>
* Fixes: trivial + linter + add missing consistency check + more/redo 14679
Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
4de3c15671 depends: add patch to common dependencies (fanquake)
Pull request description:
Building on a bare system:
```
/bitcoin/depends/work/download/zeromq-4.3.1/zeromq-4.3.1.tar.gz.temp: OK
Extracting zeromq...
/bitcoin/depends/sources/zeromq-4.3.1.tar.gz: OK
Preprocessing zeromq...
/bin/sh: 1: patch: not found
```
ACKs for commit 4de3c1:
practicalswift:
utACK 4de3c15671fea211c22b14c64ec9ac2524fcfca9
Tree-SHA512: d1a7b6b591e9de395a3bc54d9df9f97adff5f0a8b5f7a35792c27f49a610543216b2a3f3470f1e3c7dff51276e560d77d123a6d20871b0ed3e5a83da3495c5f2
0b3a65455a20504a66355ae2e6f0a83cb8462f3c Avoid redefine warning (Peter Bushnell)
Pull request description:
Wrap preprocessor definition of NOMINMAX in ifndef conditional to suppress warning when cross compiling Windows.
`fs.cpp:6:0: warning: "NOMINMAX" redefined`
`/usr/lib/gcc/x86_64-w64-mingw32/7.3-posix/include/c++/x86_64-w64-mingw32/bits/os_defines.h:45:0: note: this is the location of the previous definition
#define NOMINMAX 1`
#define NOMINMAX was introduced in the following merge.
https://github.com/bitcoin/bitcoin/pull/14426
ACKs for commit 0b3a65:
practicalswift:
utACK 0b3a65455a20504a66355ae2e6f0a83cb8462f3c
promag:
utACK 0b3a654.
Tree-SHA512: 0175195b88e63d3d44ffac2b8cc87ae7b285a45ed4e49605bca0cc82db073006c22024ef9c2f287980d357dac1099f798f1eeaa0bd75bb7a625919dc1632366c
fab98992043f47fa7240d7c1217920d0c4f783a2 test: Try once more when RPC connection fails on Windows (MarcoFalke)
faa655731eac751d4eb494268e2c815493ba9382 test: Document why connection is re-constructed on windows (MarcoFalke)
fa9f4f663c36b0824406036445e5cff0a78174e9 test: Remove python 3.4 workaround (MarcoFalke)
fae760f2b24cb26494b65c0a7ac38b92ead345af cirrus: Bump freebsd to 12.1 (MarcoFalke)
Pull request description:
Fixes: #18548
ACKs for top commit:
hebasto:
ACK fab98992043f47fa7240d7c1217920d0c4f783a2, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: c4e9ed8d995b63a820ca66984f152ac216c83ba1f318b61b15c6d375c0e936c08f6bc3d38c255dddf3ee8952f848c7ababf684854e07a7c1b1d8504e6b7208ba
6a2e6b0600077e5903400dc74bc8b0c26592fde6 Remove out of date comments for CalculateMaximumSignedTxSize (Gregory Sanders)
Pull request description:
These paths can be hit for probably a number of reasons, and ISMINE spendability is not a requirement to call it.
For example: During watch-only transaction creation, previous transaction in wallet, pubkey imported, but not the witnessscript associated with the prevout.
In this case I think no/minimal comment is better than specific and soon to be out of date.
ACKs for top commit:
achow101:
ACK 6a2e6b0600077e5903400dc74bc8b0c26592fde6
darosior:
ACK 6a2e6b0600077e5903400dc74bc8b0c26592fde6
Tree-SHA512: ad4c26fd2409eb5aed19d67c19cb5479d226bd11e9298630309c4344f6562ace2e10c2850ebe22770331d71e91320a606e79619b9fe52dd478ce1f589a740122
369244f654 utils: Fix broken Windows filelock (Chun Kuan Lee)
Pull request description:
Fix broken filelock on Windows, also add a test for this. It's a regression introduced by #13862.
Tree-SHA512: 15665b1930cf39ec71f3ab07def8e2897659f6fd4d2de749d63a5a8ec920e4a04282f12bc262f242b1b3d14d2dd9fa191ddbcf16a46fb927b5b2b14d9f6b5d01
36c8e68585 Various textual improvements in build docs (Martin Erlandsson)
Pull request description:
While reading the build docs, I found some opportunities for textual improvements (Force of habit, I used to work as a technical writer...)
* Added a few missing words, should be uncontroversial.
* Changed/added some punctuation, for better flow and readability.
* Fixed one Markdown issue, where two list item headings rendered without a line break. (See image)
This one needs to be verified after a build, I don't have a proper build environment yet.
<img width="403" alt="layout_issue" src="https://user-images.githubusercontent.com/453092/47555613-893b4d00-d90c-11e8-8a31-943846059ae7.png">
Tree-SHA512: 1e40a0414e2ce91d223933cca169d3cef25f9d2c606fd75476cef946095eee161f700f9dbf8afe60388ab8c400283d057537266d171ea63125257b7156838ecb
faa4043c66 qa: Run more tests with wallet disabled (MarcoFalke)
Pull request description:
Instead of skipping the whole test, only skip the wallet specific section of a test if the wallet is not compiled in. This is mostly an indentation change, so can be reviewed with `--ignore-all-space`.
Tree-SHA512: 5941a8b6b00dca5cf9438c5f6f010ba812115188a69e427d7ade4c1ab8cfe7a57c73daf52c66235dbb24b1cd9ab7c7a17c49bc23d931e041b605d79116a71f66
Merge #14324: qa: Run more tests with wallet disabled
faa4043c66 qa: Run more tests with wallet disabled (MarcoFalke)
Pull request description:
Instead of skipping the whole test, only skip the wallet specific section of a test if the wallet is not compiled in. This is mostly an indentation change, so can be reviewed with `--ignore-all-space`.
Tree-SHA512: 5941a8b6b00dca5cf9438c5f6f010ba812115188a69e427d7ade4c1ab8cfe7a57c73daf52c66235dbb24b1cd9ab7c7a17c49bc23d931e041b605d79116a71f66
f9e37f33ce2d8b463a0bcbe7189c9bc5b36530b7 doc: IsFinalTx comment about nSequence & OP_CLTV (Yuval Kogman)
Pull request description:
It's somewhat surprising that a transaction's `nLockTime` field is ignored
when all `nSequence` fields are final, so this change aims to clarify this
behavior and cross reference relevant details of `OP_CHECKLOCKTIMEVERIFY`.
ACKs for top commit:
MarcoFalke:
ACK f9e37f33ce2d8b463a0bcbe7189c9bc5b36530b7
Tree-SHA512: 88460dacbe4b8115fb1948715f09b21d4f34ba1da9e88d52f0b774a969f845e9eddc5940e7fee66eacdd3062dc40d6d44c3f282b0e5144411fd47eb2320b44f5
fa92e60f38cb109fe5a3c7acfe1017ffebc388cc refactor: Make httpserver work queue a unique_ptr (MarcoFalke)
Pull request description:
This simplifies the code a bit because `if (p) { delete p; p = nullptr; }` can be replaced by a call to the `reset()` member.
ACKs for top commit:
promag:
Core review ACK fa92e60f38cb109fe5a3c7acfe1017ffebc388cc.
jonatack:
ACK fa92e60f38cb109fe5a3c7acfe1017ffebc388cc code review, debug build clean, ran test/functional/interface*.py tests locally as a sanity check
hebasto:
ACK fa92e60f38cb109fe5a3c7acfe1017ffebc388cc, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 6b122162317dd4ad6889341745c7ac1903a3ee510f6548f46dc356308442a6eff13eb8dc604c38ba18783e7a66d2b836d641a8594ff980a010c12c97f3856684
168b6c317ca054c1287c36be532964e861f44266 add dummy file param to fix jupyter (Josiah Baker)
Pull request description:
this fixes argparse to use `parse_known_args`. previously, if an unknown argument was passed, argparse would fail with an `unrecognized arguments: %s` error.
## why
the documentation mentions being able to run `TestShell` in a REPL interpreter or a jupyter notebook. when i tried to run inside a jupyter notebook, i got the following error:
this was due to the notebook passing the filename of the notebook as an argument. this is a known problem with notebooks and argparse, documented here: https://stackoverflow.com/questions/48796169/how-to-fix-ipykernel-launcher-py-error-unrecognized-arguments-in-jupyter
## testing
to test, make sure you have jupyter notebooks installed. you can do this by running:
```
pip install notebook
```
or following instructions from [here](https://jupyterlab.readthedocs.io/en/stable/getting_started/installation.html).
once installed, start a notebook (`jupyter notebook`), launch a python3 kernel and run the following snippet:
```python
import sys
# make sure this is the path for your system
sys.path.insert(0, "/path/to/bitcoin/test/functional")
from test_framework.test_shell import TestShell
test = TestShell().setup(num_nodes=2, setup_clean_chain=True)
```
you should see the following output, without errors:
if you are unfamiliar with notebooks, here is a short guide on using them: https://jupyter.readthedocs.io/en/latest/running.html
ACKs for top commit:
MarcoFalke:
review ACK 168b6c317ca054c1287c36be532964e861f44266
jamesob:
crACK 168b6c317c
practicalswift:
cr ACK 168b6c317ca054c1287c36be532964e861f44266
Tree-SHA512: 4fee1563bf64a1cf9009934182412446cde03badf2f19553b78ad2cb3ceb0e5e085a5db41ed440473494ac047f04641311ecbba3948761c6553d0ca4b54937b4
3f05a9e681c4b72d99ddda8ccb6911d6ffad44ec zmq: use msg: prefix over errno= in zmqError (fanquake)
9a7cb57bbc61b2dfb772f8486db2a44c1673983a zmq: use std::string in zmqError() (fanquake)
Pull request description:
This is two minor changes. The first is to change `zmqError` to take a `const std::string&` instead of a `const char*`. The second is to change the second portion of `zmqError` to print `msg: message` rather than `errno=message`, given that `zmq_strerror` returns a message. To me, this seems more readable / useful than output like: `Error: Unable to initialize context errno=No such file or directory`.
ACKs for top commit:
practicalswift:
cr ACK 3f05a9e681c4b72d99ddda8ccb6911d6ffad44ec
instagibbs:
utACK 3f05a9e681
theStack:
Code-Review ACK 3f05a9e681c4b72d99ddda8ccb6911d6ffad44ec
Tree-SHA512: 197cf381e8b3ced271d0e575e0c6d8e5e9ed93c4b284338b17873c5232eaabe64d6c4b66e1aeb5e76befc89e316abae2b28b7fd760f178481d7b9f4e3f85da67
3b36395b96c533dde47256b505cf1cbb2844c96e depends: Fix qt.mk for mac arm64 (João Barbosa)
Pull request description:
With f16d4cd8c5412890ee0b73f4ef142b59d130e5d5 `depends/config.guess` gives `aarch64-apple-darwin20.3.0` where before would give `arm-apple-darwin20.3.0`. Fix `qt.mk` accordingly.
ACKs for top commit:
hebasto:
ACK 3b36395b96c533dde47256b505cf1cbb2844c96e, I have reviewed the code and it looks OK, I agree it can be merged.
fanquake:
ACK 3b36395b96c533dde47256b505cf1cbb2844c96e
Tree-SHA512: bd20402d0a6e9a5bb652198de189cf2b4f3f76fd03d0cba8c4d657c60b8a088cf3532efe6c1efbbedd94c00a155e6d180b77f1cd8bc24e0e35764839e8b77e30
7e2a9890e50969cdfdd08d735fa8f3c611a663a7 depends: latest config.sub (2021-04-30) (fanquake)
f16d4cd8c5412890ee0b73f4ef142b59d130e5d5 depends: latest config.guess (2021-05-24) (fanquake)
Pull request description:
This is split out of #21851. Updating these files should be mechanical, and shouldn't have to wait for that PR. Also, having support in depends for the new `arm-apple-darwin` target (added in [2593751ef276497e312d7c4ce7fd049614c7bf80](https://git.savannah.gnu.org/cgit/config.git/commit/?id=2593751ef276497e312d7c4ce7fd049614c7bf80)) is useful when debugging. i.e #22070.
If you try and compile depends for a `arm-apple-darwin` target using master, on a x86_64 darwin machine, currently you'll get:
```bash
gmake -C depends -j9 HOST=arm64-apple-darwin
Invalid configuration `arm64-apple-darwin': machine `arm64-apple' not recognized
shasum: hosts/.mk: No such file or directory
<omitted>
Makefile:111: hosts/.mk: No such file or directory
gmake: *** No rule to make target 'hosts/.mk'. Stop.
```
ACKs for top commit:
laanwj:
ACK 7e2a9890e50969cdfdd08d735fa8f3c611a663a7
Tree-SHA512: 8ed99b5d486c6cbca8929a752460338b6ee17f6bf93013c76589605678853c3a01ebd631b4d3f5d6aaeb6e5c21b7bbe39afc4454d3a697fafb27678f6d2c021e
a10972bc0 gui: Defer removeAndDeleteWallet when no modal widget is active (João Barbosa)
Pull request description:
Fixes#15310.
Tree-SHA512: ac91a4e37020d3a854830c50c0a7a45c2c0537f80be492ec5e9ba7daf90725e912f9dcc324605493599c36180e1d3bcdfa86840b7325cba208b7e93fbe7be368
64fee489448c62319e77941c30152084695b5a5d qt: Assert QMetaObject::invokeMethod result (João Barbosa)
f27bd96b5fdc2921d93c44bbf422bff0e979c4de 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 64fee489448c62319e77941c30152084695b5a5d
Tree-SHA512: d332e5d7eb2c7be5d3fe90e2e4ff20a67800b9664f6637c122a23647a964f7915703d3f086e2de440f695cfe14de268ff581d0092b7736e911952a4f4d248e25
90c0f267bdedc261d8fdab188e96ca58c206652a Squashed 'src/crc32c/' changes from 224988680f..b5ef9be675 (MarcoFalke)
Pull request description:
Except for the ARM64 darwin fix this is just code-shuffling in files/functions we don't use
ACKs for top commit:
jonasschnelli:
Tested ACK fa7c8d136f6590e54d60c37fb34ebec8da84ebbb - Tested this on an ARM Mac. Linking issue went away (successful depends compilation). Also tested that the ARM64 hardware acceleration code part was used.
laanwj:
Code review ACK fa7c8d136f6590e54d60c37fb34ebec8da84ebbb
Tree-SHA512: 1fa156d72c75d22ead2677b165e566978331f795d52a637e478d83d1cf2adddd84eed259d617df6d11270af2e4e57ae6991aec3bc4c0bdf5dec959f44daa14eb
ce9dd45422e1f4ecce6df68da086b8bfc2100756 Add [[nodiscard]] to RenameOver(...) (practicalswift)
9429a398e291a1b5edcfc657b94fcaf52cd1d8f9 Handle rename failure in DumpMempool(...) by using RenameOver(...) return value (practicalswift)
Pull request description:
Handle rename failure in `DumpMempool(...)` by using the `RenameOver(...)` return value.
Add `[[nodiscard]]` to `RenameOver(...)` to reduce the risk of similar rename issues in the future.
ACKs for top commit:
vasild:
ACK ce9dd454
theStack:
ACK ce9dd45422e1f4ecce6df68da086b8bfc2100756 🏷️
Tree-SHA512: 1e63d7f3061e1f6ea2df5750dbc1547a39bd50b6c529812a0c8a0c11d3100c241afdf14094e69b69a38bade7e54a12b2a42888545874398eaf5d02421b57e874
05c10953887bd78af2e21ef6d3c07f90dd885572 test: Add testing of ParseInt/ParseUInt edge cases with leading +/-/0:s (practicalswift)
Pull request description:
Add testing of `ParseInt`/`ParseUInt` edge cases with leading `+`/`-`/`0`:s.
Context: While working on #20457 and #20452 I noticed some edge cases which our unit tests are currently not covering.
ACKs for top commit:
MarcoFalke:
review ACK 05c10953887bd78af2e21ef6d3c07f90dd885572
laanwj:
Code review ACK 05c10953887bd78af2e21ef6d3c07f90dd885572
jonatack:
ACK 05c10953887bd78af2e21ef6d3c07f90dd885572
promag:
Code review ACK 05c10953887bd78af2e21ef6d3c07f90dd885572.
Tree-SHA512: bdfb94d8fa0293512dbba89907cb6dd0f8b1418d878267dd6d49c8c397a0e5b9714441345565d41a6a909a1cda052ef7cccece822f355ff604fcf85f2dc8136f
af3b0dfc5463c42fb9bff39f020fc1728ed44bc7 net: fix output of peer address in version message (Vasil Dimov)
Pull request description:
If `-logips -debug=net` is specified then we print the contents of the
version message we send to the peer, including his address. Because the
addresses in the version message use pre-BIP155 encoding they cannot
represent a Tor v3 address and we would actually send 16 `0`s instead (a
dummy IPv6 address). However we would print the full address in the log
message. Before this fix:
```
2020-10-21T12:24:17Z send version message: version 70016, blocks=653500, us=[::]:0, them=xwjtp3mj427zdp4tljiiivg2l5ijfvmt5lcsfaygtpp6cw254kykvpyd.onion:8333, peer=0
```
This is confusing because we pretend to send one thing while we actually
send another. Adjust the printout to reflect what we are sending. After
this fix:
```
2020-10-21T12:26:54Z send version message: version 70016, blocks=653500, us=[::]:0, them=[::]:0, peer=0
```
ACKs for top commit:
MarcoFalke:
review ACK af3b0dfc5463c42fb9bff39f020fc1728ed44bc7
jnewbery:
utACK af3b0dfc5463c42fb9bff39f020fc1728ed44bc7
Tree-SHA512: f169d7b4f07c219e541f7c37ea23b82c77e50085fc72ec62f1dd46970389916e177268d07d45c7be94dd209d1903f8f23eaff62b7fa782f6057dd36bb96bba82
c2cf8a18c25bf19ade51fedfa5c352bd7145edb0 fuzz: Check for addrv1 compatibility before using addrv1 serializer on CService (practicalswift)
Pull request description:
Check for addrv1 compatibility before using addrv1 serializer/deserializer on `CService`:
Before this patch:
```
$ src/test/fuzz/service_deserialize
service_deserialize: test/fuzz/deserialize.cpp:85:
void (anonymous namespace)::AssertEqualAfterSerializeDeserialize(const T &, const int) [T = CService]:
Assertion `Deserialize<T>(Serialize(obj, version)) == obj' failed.
```
After this patch:
```
$ src/test/fuzz/service_deserialize
…
```
Related change: #20247
ACKs for top commit:
MarcoFalke:
review ACK c2cf8a18c25bf19ade51fedfa5c352bd7145edb0
Tree-SHA512: dba6ddc60e8ef621011d844281461f1741de08c4af1a2b7156c810af44306cef7ec582de5974752db02ca085cfd23da0296d70b694e59ee262589d829fa0626e
886be97af5d4aba338b23a7b20b8560be8156231 Ignore incorrectly-serialized banlist.dat entries (Pieter Wuille)
883cea7dea3cedc9b45b6191f7d4e7be2d9a11ca Restore compatibility with old CSubNet serialization (Pieter Wuille)
Pull request description:
#19628 changed CSubNet for IPv4 netmasks, using the first 4 bytes of `netmask` rather than the last 4 to store the actual mask. Unfortunately, CSubNet objects are serialized on disk in banlist.dat, breaking compatibility with existing banlists (and bringing them into an inconsistent state where entries reported in `listbanned` cannot be removed).
Fix this by reverting to the old format (just for serialization). Also add a sanity check to the deserializer so that nonsensical banlist.dat entries are ignored (which would otherwise be possible if someone added IPv4 entries after #19628 but without this PR).
Reported by Greg Maxwell.
ACKs for top commit:
laanwj:
Code review ACK 886be97af5d4aba338b23a7b20b8560be8156231
vasild:
ACK 886be97af
Tree-SHA512: d3fb91e8ecd933406e527187974f22770374ee2e12a233e7870363f52ecda471fb0b7bae72420e8ff6b6b1594e3037a5115984c023dbadf38f86aeaffcd681e7
3491bf358a81d41a386cd14581d15396354a6e6c test: Mention commit id in scripted diff error (Wladimir J. van der Laan)
Pull request description:
Add commit id to make spotting the issue easier.
ACKs for top commit:
robot-dreams:
ACK 3491bf358a81d41a386cd14581d15396354a6e6c
sipa:
utACK 3491bf358a81d41a386cd14581d15396354a6e6c
hebasto:
~ACK~ Concept ACK 3491bf358a81d41a386cd14581d15396354a6e6c, should help in situations like https://travis-ci.org/github/bitcoin/bitcoin/jobs/732481553
Tree-SHA512: 1ae66fa760f9e5d52e029bae71f6b5863f1efd7b95de3723ea09290944c9d7687f5ec6927aa115a3aebd6f2b993baa0c2433975c6ad5cd2858089013362eb599
7a89f2e6c539a54bcaa24bff41aae3910244ad3d build: Fix target name (Hennadii Stepanov)
Pull request description:
It seems like a typo :)
This PR:
- fixes errors when building a package in depends for `HOST=x86_64-apple-darwin16` (fix#19799)
- is a correct alternative to d25e0e308f from #19764
ACKs for top commit:
icota:
tACK 7a89f2e6c5
dongcarl:
Code Review ACK 7a89f2e6c539a54bcaa24bff41aae3910244ad3d
theuni:
ACK 7a89f2e6c539a54bcaa24bff41aae3910244ad3d.
Tree-SHA512: a0bcbc6805d3450e201476ef1e22e0eb53903db1586c5515314c19afd337bded887e56de0fbe62feaf359b2de15dbccd49a44f1a8b566b4c64f5ae3d94a2ab6d
e1fdd2963baab68bb6a77af2ad7a07fcacd4e73e Test batch rpc with params (Gregory Sanders)
Pull request description:
Useful as an example and test case.
ACKs for top commit:
laanwj:
ACK e1fdd2963baab68bb6a77af2ad7a07fcacd4e73e
theStack:
ACK e1fdd2963baab68bb6a77af2ad7a07fcacd4e73e
Tree-SHA512: 2d2ba8960916342b264a14624857d6dd10005be12efafb3e970b82656f721c8f3700ebc9b8809de1b2f887d482b772043504aeaeebc7f2e1c8203f076a451526
812037cb80f72096738cf2b0c15b39536c6c1e24 Change CSipHasher's count variable to uint8_t (Pieter Wuille)
Pull request description:
SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). `uint8_t` is sufficient, however.
Fixes#19930.
ACKs for top commit:
laanwj:
anyhow re-ACK 812037cb80f72096738cf2b0c15b39536c6c1e24
elichai:
utACK 812037cb80f72096738cf2b0c15b39536c6c1e24
practicalswift:
ACK 812037cb80f72096738cf2b0c15b39536c6c1e24
theStack:
ACK 812037cb80f72096738cf2b0c15b39536c6c1e24
Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
d780293e1ee0f9e66bd2d88914694c17f9aaa0ca net: improve nLastBlockTime and nLastTXTime documentation (Jon Atack)
Pull request description:
Follow-up to #19731 to help alleviate confusion around `nLastBlockTime` and `nLastTXTime`, now also provided by the JSON-RPC API as `last_block` and `last_transaction` in `getpeerinfo` output.
Thanks to John Newbery, credited in the commit, and to Dave Harding and Adam Jonas during discussions on how to best explain these in this week's Optech newsletter.
ACKs for top commit:
practicalswift:
ACK d780293e1ee0f9e66bd2d88914694c17f9aaa0ca
MarcoFalke:
ACK d780293e1ee0f9e66bd2d88914694c17f9aaa0ca
harding:
ACK d780293e1ee0f9e66bd2d88914694c17f9aaa0ca . The added documentation matches my reading of the code and answers a question I had after seeing #19731
0xB10C:
ACK d780293e1ee0f9e66bd2d88914694c17f9aaa0ca
Tree-SHA512: 72d47cf50a099913c7e4753cb80e11785b26fb66fa3a8b6c382fde4ea725116f3d215f93d32a567246d269768e66159f8dcf017a1bbc6d5f2489a35f81c316fa
c276df775914e4e42993c76e172ef159e3b830d4 zmq: enable tcp keepalive (mruddy)
Pull request description:
This addresses https://github.com/bitcoin/bitcoin/issues/12754.
These changes enable node operators to address the silent dropping (by network middle boxes) of long-lived low-activity ZMQ TCP connections via further operating system level TCP keepalive configuration. For example, ZMQ sockets that publish block hashes can be affected in this way due to the length of time it sometimes takes between finding blocks (e.g.- sometimes more than an hour).
Prior to this patch, operating system level TCP keepalive configurations would not take effect since the SO_KEEPALIVE option was not enabled on the underlying socket.
There are additional ZMQ socket options related to TCP keepalive that can be set. However, I decided not to implement those options in this changeset because doing so would require adding additional bitcoin node configuration options, and would not yield a better outcome. I preferred a small, easily reviewable patch that doesn't add a bunch of new config options, with the tradeoff that the fine tuning would have to be done via well-documented operating system specific configurations.
I tested this patch by running a node with:
`./src/qt/bitcoin-qt -regtest -txindex -datadir=/tmp/node -zmqpubhashblock=tcp://127.0.0.1:28332 &`
and connecting to it with:
`python3 ./contrib/zmq/zmq_sub.py`
Without these changes, `ss -panto | grep 28332 | grep ESTAB | grep bitcoin` will report no keepalive timer information. With these changes, the output from the prior command will show keepalive timer information consistent with the configuration at the time of connection establishment, e.g.-: `timer:(keepalive,119min,0)`.
I also tested with a non-TCP transport and did not witness any adverse effects:
`./src/qt/bitcoin-qt -regtest -txindex -datadir=/tmp/node -zmqpubhashblock=ipc:///tmp/bitcoin.block &`
ACKs for top commit:
adamjonas:
Just to summarize for those looking to review - as of c276df775914e4e42993c76e172ef159e3b830d4 there are 3 tACKs (n-thumann, Haaroon, and dlogemann), 1 "looks good to me" (laanwj) with no NACKs or any show-stopping concerns raised.
jonasschnelli:
utACK c276df775914e4e42993c76e172ef159e3b830d4
Tree-SHA512: b884c2c9814e97e666546a7188c48f9de9541499a11a934bd48dd16169a900c900fa519feb3b1cb7e9915fc7539aac2829c7806b5937b4e1409b4805f3ef6cd1
