9576614d2d doc: Describe log files + consistent paths in test READMEs (Martin Erlandsson)
Pull request description:
picks up #15830
I saw this was almost ready to merge but the test logging part was not 100% correct. I reworked that part, the rest is the same.
ACKs for top commit:
GChuf:
ACK 9576614d2d
Tree-SHA512: 3de7f1b0a1b0419df6e7b55964d00e715b6cb7874b1849ad6f120597610d7df4182c4b61b9c9691ce04f4e392ed3caead4c623374be2066ac31319e702d45d09
1f6c650c99 travis: run tests on macOS native (Sjors Provoost)
Pull request description:
Adds an additional Travis machine to run the functional test suite on native macOS
Homebrew is not particularly Travis compatible, but I found some useful hints here: https://discourse.brew.sh/t/best-practice-for-homebrew-on-travis-brew-update-is-5min-to-build-time/5215/11
ACKs for top commit:
MarcoFalke:
re-ACK 1f6c650c99
Tree-SHA512: 3f19a1695fac53d4d6c2033a9c20be69294e3a798c84fd9bf6ae2aa7a6d92aa1dad1f62f4ee1ada9413fe7d05ee974050fa030fd2c547f33e0d5c0a3e74f64db
85016e52f6 [rpc] Fix broken bitcoin-cli examples (Andrew Toth)
Pull request description:
This fixes the `bitcoin-cli` examples for `combinerawtransaction`, `combinepsbt` and `testmempoolaccept`. They currently return `Error parsing JSON`.
ACKs for top commit:
laanwj:
ACK 85016e52f6
Tree-SHA512: b561f68f7a188dc91dab1ceb98da3ac3e232143ab2b906c90f95c6b74b584599d0f3b51f067cdd3b1153931f95b3dc385e453b1a0dde86f9cb549b94560f219d
5013171eaf doc: correct function name in ReportHardwareRand() (fanquake)
Pull request description:
The function is `InitHardwareRand` not `HWRandInit`.
46d6930f8c/src/random.cpp (L99)
ACKs for top commit:
laanwj:
ACK 5013171eaf
theStack:
ACK 5013171eaf
Tree-SHA512: c25e1bb56e923961fc8a9178d751222b60f5ca36be84abf8fd1ac971f3a9b79b587ed9d8a4a175981b66f3fd5ad7edd6697d343e4dc4852351a1510718745455
f59bbb61af test: Fix bug in blockfilter_index_tests. (Jim Posen)
Pull request description:
The test case tests a chain reorganization, however the two chains were generated in the same manner and thus produced the same blocks.
This issue was [pointed out](https://github.com/bitcoin/bitcoin/pull/14121#discussion_r334282663) by MarcoFalke.
ACKs for top commit:
MarcoFalke:
Thanks! ACK f59bbb61af (looked at the diff on GitHub, didn't compile, nor run tests)
Tree-SHA512: a2f063ae9312051ffc2a3fcc1116a6a8ac09beeef261bc40aa3ff7270ff4de22a790eb19fec6b15ba1eb46e78f1f317bfd91472d8581b95bb9441a56b102554e
084e17cebd Remove unused includes (practicalswift)
Pull request description:
As requested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/16273#issuecomment-521332089:
This PR removes unused includes.
Please note that in contrast to #16273 I'm limiting the scope to the trivial cases of pure removals (i.e. no includes added) to make reviewing easier.
I'm seeking "Concept ACK":s for this obviously non-urgent minor cleanup.
Rationale:
* Avoids unnecessary re-compiles in case of header changes.
* Makes reasoning about code dependencies easier.
* Reduces compile-time memory usage.
* Reduces compilation time.
* Warm fuzzy feeling of being lean :-)
ACKs for top commit:
ryanofsky:
Code review ACK 084e17cebd. PR only removes include lines and it still compiles. In the worst case someone might have to explicitly add an include later for something now included implicitly. But maybe some effort was taken to avoid this, and it wouldn't be a tragedy anyway.
Tree-SHA512: 89de56edc6ceea4696e9579bccff10c80080821685b9fb4e8c5ef593b6e43cf662f358788701bb09f84867693f66b2e4db035b92b522a0a775f50b7ecffd6a6d
d7820a1250 util: Filter control characters out of log messages (Wladimir J. van der Laan)
Pull request description:
Belts and suspenders: make sure outgoing log messages don't contain potentially suspicious characters, such as terminal control codes.
This escapes control characters except newline ('\n') in C syntax. It escapes instead of removes them to still allow for troubleshooting issues where they accidentally end up in strings (it is a debug log, after all).
(more checks could be added such as UTF-8 validity and unicode code-point range checking—this is substantially more involved and would need to keep track of state between characters and even `LogPrint` calls as they could end up split up—but escape codes seem to be the most common attack vector for terminals.)
ACKs for top commit:
practicalswift:
ACK d7820a1250 - tested and works as expected :)
Tree-SHA512: 0806265addebdcec1062a6def3e903555e62ba5e93967ce9ee6943d16462a222b3f41135a5bff0a76966ae9e7ed75f211d7785bceda788ae0b0654bf3fd891bf
b3b26e149c rpc: fix -rpcclienttimeout 0 option (Fabian Jahr)
Pull request description:
fixes#17117
I understood the bug as the help string being wrong, rather than that this feature is missing and should be added. Let me know if it should be the other way around.
It is notable that if 0 is given as an argument, the fallback that is being used is the libevent default of 50 seconds, rather than `DEFAULT_HTTP_CLIENT_TIMEOUT` (900 seconds). This is not intuitive for the user. I could handle this in this PR but I am unsure which would be the better solution then: Actually adding the feature as described in the help string or falling back to `DEFAULT_HTTP_CLIENT_TIMEOUT`? Happy to hear opinions.
ACKs for top commit:
MarcoFalke:
unsigned ACK b3b26e149c
Tree-SHA512: 65e526a652c0adcdb4f895e8d78d60c7caa5904c9915b165a3ae95725c87d13af1f916359f80302452a2fcac1a80f4c58cd805ec8c28720fa4b91b3c8baa4155
a0daea459c [build] depends macOS: point --sysroot to SDK (Sjors Provoost)
Pull request description:
Fixes errors like `fatal error: 'unistd.h' file not found` when building depends on macOS.
Replaces #14352 (which doesn't work on Catalina).
ACKs for top commit:
jonasschnelli:
utACK a0daea459c
Tree-SHA512: 995b1e1e84e635b32d1d4038bc63730c94a7c318b7240f6d62825977e5c97fe52c5aa5a0f39070beb0df8271dd294b36d6b5cf7f09ad07494fb15d5bd4d77f68
8019b6b150 gui: Make RPCConsole::TabTypes an enum class (João Barbosa)
Pull request description:
This change makes the compiler emit a warning/error if a missing enum value is not handled. See also #17134.
ACKs for top commit:
MarcoFalke:
unsigned ACK 8019b6b150
hebasto:
re-ACK 8019b6b150
fanquake:
ACK 8019b6b150
Tree-SHA512: 329161097f4d079f48d5fb33bf3a07e314fbb2ac325cafb08bafa9e76229ecff0f9010fe3c1c15ccd02d4539b5c93839c846b42bfeaffa897a917cea599bf811
b96ed03962 [wallet] Remove pruning check for -rescan option (John Newbery)
eea462de9c [wallet] Remove package limit config access from wallet (John Newbery)
Pull request description:
Removes wallet access to `-limitancestorcount`, `-limitdescendantcount` and `-prune`:
- `-limitancestorcount` and `-limitdescendantcount` are now accessed with a method `getPackageLimits` in the `Chain` interface.
- `-prune` is not required. It was only used in wallet component initiation to prevent running `-rescan` when pruning was enabled. This check is not required.
Partially addresses #17137.
ACKs for top commit:
MarcoFalke:
Tested ACK b96ed03962
ryanofsky:
Code review ACK b96ed03962
promag:
Code review ACK b96ed03962.
ariard:
ACK b96ed03, check there isn't left anymore wallet access to node arguments.
Tree-SHA512: 90c8e3e083acbd37724f1bccf63dab642cf9ae95cc5e684872a67443ae048b4fdbf57b52ea47c5a1da6489fd277278fe2d9bbe95e17f3d4965a1a0fbdeb815bf
610d9384de gui: Added label & tooltip for Verify Message labels (dannmat)
Pull request description:
When using the Verify Message functionality, I found the input boxes to be rather confusing as they had no guidance for their purpose.
I have added tooltips and labels to aid users when verifying messages in future
ACKs for top commit:
promag:
Code review ACK 610d9384de. Nit, commit and title are a little weird. Suggestion: "gui: Add toolTip and placeholderText to sign message fields"
MarcoFalke:
ACK 610d9384de (looks good, didn't compile or tested the changes)
fanquake:
ACK - 610d9384de
Tree-SHA512: d6a1bc872ad270dce440e96a163ce72cdd4708913d87a0fea749fc8cf2d8163b791cbb96a82030e0cb7d239920ceb0e3f05e0eec113f45a1a8e1309fbd92b4b0
eebcdfa86a [test] rename SegwitVersion1SignatureHash() (John Newbery)
Pull request description:
The function implementing segwit v0 signature hash was originally named
SegwitVersion1SignatureHash() (presumably before segwit v0 was named
segwit v0). Rename it to SegwitV0SignatureHash().
Also rename SignatureHash() to LegacySignatureHash() for disambiguation.
ACKs for top commit:
laanwj:
ACK eebcdfa86a
elichai:
ACK eebcdfa86a (Checked to see you didn't miss any renaming)
theStack:
ACK eebcdfa86a
Tree-SHA512: ae504ac33dc4fca38079a113beb5ebcaf509162aef121edec5368a460a24c2ac040ef84f0be1dfc6186c32d94d68f8129db049907f1d6449f92eea9d39a40dbd
4441e58497 Update macdeploy README to include correctly named `.dmg` file produced from `make deploy` (Zakk)
Pull request description:
Fixes issue #16909 to update the `contrib/macdeploy/README.md` to match the files produced from `make deploy`
The files produced from `make deploy` are as follows:
- `Bitcoin-QT.dmg`
- `Bitcoin Core.app`
- `dist/Bitcoin Core.app`
ACKs for top commit:
jonasschnelli:
ACK 4441e58497
fanquake:
ACK 4441e58497 - checked that `Bitcoin-Qt.dmg` is produced.
Tree-SHA512: 99bfadab59c7c516005b051e4a369f330178313a284bb665c22c40f70a6159f175909c08db1b32976ad7b130b53b414f8ba96f8ff7cbd164f2724c0cc151704a
5f40d2770a github: Add warning for bug reports (Wladimir J. van der Laan)
Pull request description:
I've noticed the "Bug" label being added redundantly fairly frequently. I think this might be due to github's templates.
All in all, the link in https://github.com/bitcoin/bitcoin/issues/new/choose to open a regular issue is a bit hidden from sight. Direct people's attention to it.
ACKs for top commit:
practicalswift:
ACK 5f40d2770a - currently it is very easy to miss the tiny "Open a regular issue" link :)
jonasschnelli:
ACK 5f40d2770a
hebasto:
ACK 5f40d2770a
Tree-SHA512: e6c94c02f9f7d00621b580d406d03f8754173150bf456409ccc474b76fb93ff857ff4a0c652bf5c03d4f1b97ecf29ae0ff7bf8b763207f9c8522b8dcecc20109
44f7a8d7a7 Disable _FORTIFY_SOURCE when enable-debug (Andrew Chow)
Pull request description:
The `_FORTIFY_SOURCE` macro is enabled by default when hardening is enabled, but it requires optimization in order to be used. Since we disable all optimization with `--enable-debug`, this macro doesn't actually do anything and instead just causes a lot of warnings to be printed. This PR explicitly disables `_FORTIFY_SOURCE` so that these useless warnings aren't printed.
ACKs for top commit:
laanwj:
Thanks. ACK 44f7a8d7a7
Tree-SHA512: e9302aef794dfd9ca9d0d032179ecc51d3212a9a0204454419f410011343b27c32e6be05f385051b5b594c607b91b8e0e588f644584d6684429a649a413077d9
d48f664440 tests: Fix fs_tests for unknown locales (Daki Carnhof)
Pull request description:
Fix by removing "L" as suggested by meeDamian in
https://github.com/bitcoin/bitcoin/issues/14948#issuecomment-522355441
```
# all in .../bitcoin/src/test
$ uname -m
x86_64
$ export LC_ALL=randomnonexistentlocale
$ ./test_bitcoin
Running 369 test cases...
unknown location(0): fatal error: in "fs_tests/fsbridge_fstream": boost::system::system_error: boost::filesystem::path codecvt to string: error
test/fs_tests.cpp(13): last checkpoint: "fsbridge_fstream" test entry
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
```
After the patch is applied, the same test under the same conditions runs fine.
```
$ export LC_ALL=randomnonexistentlocale
$ ./test_bitcoin
Running 369 test cases...
*** No errors detected
```
Co-Authored-By: bugs@meedamian.com
ACKs for top commit:
laanwj:
ACK d48f664440
Tree-SHA512: a9910252b8ce6a05cab5530874549c2999ca2c28e835fc18aa8e5468fb417bd7d245864ec71d9233dd53e02940a9f0691b247430257f27eb0d7c20745d1c846d
fa6ed82794 doc: update bips.md with buried BIP9 deployments (MarcoFalke)
Pull request description:
Also, remove the activation heights, as they can be retrieved from `./src/chainparams.cpp` (if needed)
ACKs for top commit:
laanwj:
ACK fa6ed82794, needs backport to 0.19 I guess.
Tree-SHA512: 9c069cc14589a3e2309d76f042677c024a9e14d16dbfccef54c4a2963ca7853d01f042b0237e346538c557591b7553deed9dd811ba64bbd0ced88883d562c59a
Belts and suspenders: make sure outgoing log messages don't contain
potentially suspicious characters, such as terminal control codes.
This escapes control characters except newline ('\n') in C syntax.
It escapes instead of removes them to still allow for troubleshooting
issues where they accidentally end up in strings.
I've noticed the "Bug" label being added redundantly fairly frequently.
I think this might be due to github's templates.
All in all, the link in https://github.com/bitcoin/bitcoin/issues/new/choose
to open a regular issue is a bit hidden from sight. Direct people's
attention to it.
The function implementing segwit v0 signature hash was originally named
SegwitVersion1SignatureHash() (presumably before segwit v0 was named
segwit v0). Rename it to SegwitV0SignatureHash().
Also rename SignatureHash() to LegacySignatureHash() for disambiguation.
Prior to this PR, the wallet would not allow the `-rescan` option at
startup if pruning was enabled. This is unnecessarily restrictive. It
should be possible to rescan if pruning is enabled, as long as no blocks
have actually been pruned yet.
Remove the pruning check from WalletInit::ParameterInteraction(). If any
blocks have been pruned, that will be caught in CreateWalletFromFile().
The wallet should not be able to directly access global configuration
from the node. Remove access of "-limitancestorcount" and
"-limitdescendantcount".
fba4baa4fa test: speed up wallet_address_types by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
approaches another part of #16613 ("Functional test suite bottlenecks")
As for `wallet_backup.py` (Commit 581c9be0d8), the
bottleneck is in relaying transactions. By whitelisting the peers, the
inventory is transmissioned immediately rather than on average every 5 seconds,
speeding up the test significantly:
before:
```
$ time ./wallet_address_types.py
real 1m30.072s
user 0m6.478s
sys 0m2.298s
```
with this PR:
```
$ time ./wallet_address_types.py
real 0m26.785s
user 0m5.525s
sys 0m1.888s
```
ACKs for top commit:
fanquake:
ACK - fba4baa4fa
Tree-SHA512: 6728ae44bd8839426fa943d06af884e40c2d88de5d7807269a1e78ff987077160aa7e8d395f4468e6ca1d6f2110c7a03cd346a3339b256702f4cdabd285f7f86
32d665c265 test: fix "tx-size-small" errors after default address change (Sebastian Falbesoner)
Pull request description:
Addresses #17043, affects RBF and BIP68 functional tests.
The "tx-size-small" policy rule rejects transactions with a non-witness size of
smaller than 82 bytes (see `src/validation.cpp:MemPoolAccept::PreChecks(...)`),
which corresponds to a transaction with 1 segwit input and 1 P2WPKH output.
Through the default address change, the created test transactions have segwit
inputs now and sending to short scriptPubKeys might violate this rule. By
bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH
scriptPubKey), on all occurences the problem is solved.
The dummy scriptPubKey has the format:
```21 <21-byte-long string of 'a' or 1s>```
ACKs for top commit:
instagibbs:
reACK 32d665c265 just s/Bytes/bytes/
MarcoFalke:
ACK 32d665c265
Tree-SHA512: 80e0386ff3c3f462901ba5c1e5ef2cbf095d9c0a40c8c3cfeacd4a3ab676afe744aa95b9eed77b4b3eec88bed930b33aa718117ed0977f6374e858a2f3bd5c57
Addresses #17043, affects RBF and BIP68 functional tests.
The "tx-size-small" policy rule rejects transactions with a non-witness size of
smaller than 82 bytes (see src/validation.cpp:MemPoolAccept::PreChecks(...)),
which corresponds to a transaction with 1 segwit input and 1 P2WPKH output.
Through the default address change, the created test transactions have segwit
inputs now and sending to short scriptPubKeys might violate this rule. By
bumping the dummy scriptPubKey size to 22 bytes (= the size of a P2WPKH
scriptPubKey), on all occurences the problem is solved.
The dummy scriptPubKey has the format:
21 <21-byte-long string of 'a' or 1s>
former commit messages, now squashed:
test: rbf, bip68: use constant DUMMY_P2WPKH_SCRIPT for bumped scriptPubKey
test: rbf, bip68: use constant DUMMY_P2WPKH_SCRIPT for dummy scriptPubKeys (b'a' * 35)
test: rbf, bip68: comment DUMMY_P2WPKH_SCRIPT constant, put into common (new) module
bd3f5a90ec build: remove mingw linker workaround from win gitian descriptor (fanquake)
Pull request description:
This workaround was added as part of the switch to gitian building using Ubuntu 14.04 (#6900).
However, it should no longer be required, as we have switched to Bionic (#13171) and that
has a far newer version of binutils.
Original discussion: https://github.com/bitcoin/bitcoin/pull/6900
binutils patch: https://sourceware.org/bugzilla/show_bug.cgi?id=16192
ACKs for top commit:
MarcoFalke:
ACK bd3f5a90ec
theuni:
ACK bd3f5a90ec
laanwj:
ACK bd3f5a90ec
Tree-SHA512: 01a5789994decf8cdedf7aaa0a449d2100a77e2e6b422d6b9dd5a4ac3e2e0b538c3d43aae4a1c3713614782f3c6b09d8d8bb21c20e86ce3c1734183dedd02d0c
approaches another part of #16613 ("Functional test suite bottlenecks")
As for wallet_backup.py (Commit 581c9be0d8), the
bottleneck is in relaying transactions. By whitelisting the peers, the
inventory is transmissioned immediately rather than on average every 5 seconds,
speeding up the test significantly:
before:
$ time ./wallet_address_types.py
real 1m30.072s
user 0m6.478s
sys 0m2.298s
with this PR:
$ time ./wallet_address_types.py
real 0m26.785s
user 0m5.525s
sys 0m1.888s
581c9be0d8 test: speedup wallet_backup by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
Pull request description:
approaches part of #16613 ("Functional test suite bottlenecks")
The majority of the test time is spent in `sync_mempools()` after sending to
addresses, i.e. the bottleneck is in relaying transactions. By whitelisting the
peers via `-whitelist`, the inventory is transmissioned immediately rather than
on average every 5 seconds, speeding up the test by at least a factor of two:
before:
```
$ time ./wallet_backup.py
real 2m2.523s
user 0m6.093s
sys 0m2.454s
```
with this PR:
```
$ time ./wallet_backup_with_whitelist.py
real 0m36.570s
user 0m5.365s
sys 0m1.696s
```
Note that the test is not deterministic (the `sendtoaddress` RPC in function
`one_send()` is executed with a probability of 50%), hence the times could vary
between individual runs.
ACKs for top commit:
MarcoFalke:
ACK 581c9be0d8, this test is testing the backup behaviour, not the tx relay behaviour
fanquake:
ACK 581c9be0d8
Tree-SHA512: d016f39cdb85501e17a74a4c4db5a9f7404baa76fbcc3675a34d3cd7bf03d7a4cb4fd3e5f17cb0597248120bb5ac8b15d3db7663007b76b010902be72954bde0
approaches part of #16613 ("Functional test suite bottlenecks")
The majority of the test time is spent in sync_mempools() after sending to
addresses, i.e. the bottleneck is in relaying transactions. By whitelisting the
peers via -whitelist, the inventory is transmissioned immediately rather than
on average every 5 seconds, speeding up the test by at least a factor of two:
before:
$ time ./wallet_backup.py
real 2m2.523s
user 0m6.093s
sys 0m2.454s
with this PR:
$ time ./wallet_backup_with_whitelist.py
real 0m36.570s
user 0m5.365s
sys 0m1.696s
Note that the test is not deterministic (the sendtoaddress RPC in function
one_send() is executed with a probability of 50%), hence the times could vary
between individual runs.
MarcoFalke