This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
63ce882760 doc: link to homebrew's troubleshooting page (Gastón I. Silva)
Pull request description:
A trivial documentation update.
When I was following the build steps for mac, I had some errors installing the dependencies. After searching on the Internet, and correcting the errors, I found that `brew doctor` had all the answers I needed. Could have skipped the Internet searches all together.
ACKs for top commit:
fanquake:
ACK 63ce882760 - a link to the troubleshooting page seems fine. I wouldn't really want our README to have anything more specific than that.
Tree-SHA512: 12c96cd9c9bd39ada21f3f27cbec3ed4bef4b8e74dec7872c892fc6a92a70418a5cc0882ff449883e91d96c01e1ca7104b076590917f397334c82931ec7fda1c
7f1475c711 rpc: update validateaddress RPCExamples to bech32 (Sebastian Falbesoner)
Pull request description:
Another small step to get rid of legacy addresses in the RPC help texts and by that encourage the use of bech32 addresses by default. The (invalid) address is the same as in the `getaddressinfo` RPC (see 2ee0cb3330, kudos to jonatack!), I don't think it adds any value to have a different example address per RPC.
ACKs for top commit:
fanquake:
ACK 7f1475c711
MarcoFalke:
ACK 7f1475c711
Tree-SHA512: 2350f61fa942a9053f9f5c860ea446965dc7209c71c81bdb98a859d03ca23b225ad72c9c506e4a55c8d8988823d9cfbe808c1a452a1eeadb70ab186b146dd4ca
f2472f6460 tests: Improve test runner output in case of target errors (practicalswift)
733bbec34f tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift)
5ea81449f3 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift)
555236f769 tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift)
a3b539a924 ci: Run fuzz testing test cases under valgrind (practicalswift)
Pull request description:
Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.
This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (#18162) and similar cases.
ACKs for top commit:
MarcoFalke:
ACK f2472f6460👼
Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493
This tests that a mempool transaction expires after a given timeout
and its children are removed as well.
Both the default expiry timeout defied by DEFAULT_MEMPOOL_EXPIRY and
a user definable expiry timeout via the -mempoolexpiry=<n> command
line argument (<n> is the timeout in hours) are tested.
a304a3632f Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92cc wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)
Pull request description:
Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.
This change adds a lot of comments and allows reverting commit 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible
ACKs for top commit:
Sjors:
re-ACK a304a3632f (rebase, slight text changes and my test)
achow101:
re-ACK a304a3632f
meshcollider:
utACK a304a3632f
Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
Before, macros defined in `<event2/http.h>` have been used
for some HTTP status codes.
`<event2/http.h>` is included implicitly and the usage
of its status code macros is inconsistent with the majority
HTTP response implementations in this file.
Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is
consistently used for all HTTP response implementations.
8bca30ea17 [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5b52 [lib] add scheduler to node context (Amiti Uttarwar)
930d837542 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e83c6 [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f63598ad [util] allow scheduler to be mocked (Amiti Uttarwar)
Pull request description:
This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.
It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.
This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.
ACKs for top commit:
MarcoFalke:
ACK 8bca30ea17, only change is some style fixups 🕓
Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
530d02addb build: pass -fno-ident in Windows gitian descriptor (fanquake)
Pull request description:
`-fno-ident` prevents compilers from emitting compiler name and version number information that can needlessly bloat binaries.
For example, in the `v0.19.0.1` Windows release binaries, there are > 1000 GCC compiler version strings embedded:
```bash
# GCC: (GNU) 7.3-posix 20180312... & GCC: (GNU) 6.3.0 20170415.......
strings bitcoind.exe | rg GCC | wc -l
1021
```
They end up collected in the end of the`.rdata` section, and cannot be removed by `strip`. i.e:
```bash
objdump --section=.rdata --full-contents bitcoind.exe
...
cfcc00 00000000 00000000 00000000 00000000 ................
cfcc10 00000000 00000000 00000000 00000000 ................
cfcc20 4743433a 2028474e 55292036 2e332e30 GCC: (GNU) 6.3.0
cfcc30 20323031 37303431 35000000 00000000 20170415.......
cfcc40 4743433a 2028474e 55292037 2e332d70 GCC: (GNU) 7.3-p
cfcc50 6f736978 20323031 38303331 32000000 osix 20180312...
cfcc60 4743433a 2028474e 55292037 2e332d70 GCC: (GNU) 7.3-p
cfcc70 6f736978 20323031 38303331 32000000 osix 20180312...
```
The flag is available for [Clang](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-qn) and [GCC](https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-ident).
Relevant code in [GCC](https://github.com/gcc-mirror/gcc/blob/master/gcc/toplev.c#L565-L578):
```c
/* Attach a special .ident directive to the end of the file to identify
the version of GCC which compiled this code. The format of the .ident
string is patterned after the ones produced by native SVR4 compilers. */
if (!flag_no_ident)
{
const char *pkg_version = "(GNU) ";
char *ident_str;
if (strcmp ("(GCC) ", pkgversion_string))
pkg_version = pkgversion_string;
ident_str = ACONCAT (("GCC: ", pkg_version, version_string, NULL));
targetm.asm_out.output_ident (ident_str);
}
```
ACKs for top commit:
practicalswift:
ACK 530d02addb
laanwj:
ACK 530d02addb
Tree-SHA512: b3b28f43ec483dee28d1df8548fe72425bf00e750701825c256395f6aa7b23256eb27609b51779b86aed108b6eaa3912181a9d8282e23eebf9cee7784f9fabe0
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)
Pull request description:
Fixes#13237.
ACKs for top commit:
laanwj:
ACK 4e9efac678
Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
fad027fb0c scripted-diff: Add missing spaces in RPCResult, Fix type names (MarcoFalke)
Pull request description:
This makes the rendered diff smaller when the RPCResult is machine generated later on (Previous attempts: #14601 and #14459)
ACKs for top commit:
Sjors:
ACK fad027fb0c
Tree-SHA512: 48afd571b1cd349ca0b29bb444c1c7cda657e07dd96c610d479f931ccd938186aec98e533d0552b5b10afc9a3d7b911359260a49448e8e1106e3647b2c71f3ba
1b068c50dd tests: Add --valgrind option to test/fuzz/test_runner.py for running fuzzing test cases under valgrind (practicalswift)
Pull request description:
Add `--valgrind` option to `test/fuzz/test_runner.py` for running fuzzing test cases under `valgrind`.
Test this PR using:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=fuzzer
$ make
$ git clone https://github.com/bitcoin-core/qa-assets
$ test/fuzz/test_runner.py --valgrind -l DEBUG qa-assets/fuzz_seed_corpus/
```
ACKs for top commit:
MarcoFalke:
ACK 1b068c50dd🌒
Tree-SHA512: e6eb99af1bceaa6f36f49092a05de415848099ccc1497cc098a62e925954c978cb37a46410b44ed5eef2c6464ca4ecb06397b75b5d35701f5a8525436e47b9fd
c98c26ee99 ci: use --enable-werror on more hosts (Sjors Provoost)
6ba617dbe2 build: add Wreturn-type to Werror flags (Sjors Provoost)
Pull request description:
I overlooked a missing `return false` in https://github.com/bitcoin/bitcoin/pull/17577#discussion_r379137776 and the warning only showed up on one Travis machine (`warning: control reaches end of non-void function [-Wreturn-type]`).
This PR promotes `Wreturn-type` to an error when configured with `--enable-werror`. I also added `--enable-werror` to the Travis machine that happened to catch this particular instance.
ACKs for top commit:
vasild:
ACK c98c26e.
practicalswift:
ACK c98c26ee99
Tree-SHA512: 64e86c67fef2c5048aab201a8400b7e4a6f27b93d626159ba0b2807b5f119d2b0a83e3372db88f692cb4b0d059722d6a642d130c74a4f991a27f3a6b21780b5f
And add unit test for it.
The purpose of using a preamble or "magic" text as part of signing and
verifying a message was not given when the code was repeated in a few
locations. Make a test showing how it is used to prevent inadvertently
signing a transaction.
The logic of signing a message was duplicated in 3 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
src/rpc/misc.cpp
signmessagewithprivkey()
src/wallet/rpcwallet.cpp
signmessage()
Move the logic into
src/util/message.cpp
MessageSign()
and call it from all the 3 places.
The logic of verifying a message was duplicated in 2 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()
src/rpc/misc.cpp
verifymessage()
with the only difference being the result handling. Move the logic into
a dedicated
src/util/message.cpp
MessageVerify()
which returns a set of result codes, call it from the 2 places and just
handle the results differently in the callers.
25bc17fceb refactor: rpc: Remove vector copy from listtransactions (João Barbosa)
Pull request description:
Current approach
- copy accumulated `ret` vector to `arrTmp`
- drop unnecessary elements from `arrTmp`
- reverse `arrTmp`
- clear `ret`
- copy `arrTmp` to the `ret`
New approach
- create a vector from the accumulated `ret` with just the necessary elements already reversed
- copy it to the result
This PR doesn't change behavior.
ACKs for top commit:
ryanofsky:
Code review ACK 25bc17fceb. Just comment and commit message tweaks since last review
Tree-SHA512: 87906561e3accdbdb0f4a8194cbcd76ea53ae53d0ce135b90bc54a5f77e300b14ef08505e7daf1fe52426f135442a743da5a027416a769bd454922357cebe7c0
also contains the following changes:
- rpc: factor out example bech32 address for RPCExamples
- doc: update developer notes wrt RPCExamples addresses
(mention the EXAMPLE_ADDRESS constant as an example for an invalid bech32
address suitable for RPCExamples help documentation)
c9fe61291e gui: Throttle GUI update pace when -reindex (Hennadii Stepanov)
Pull request description:
This is grabbed from #17565.
All **laanwj**'s and **ryanofsky**'s suggestions are implemented.
With this PR, the GUI does not freeze when a user runs:
```
$ ./src/qt/bitcoin-qt -reindex
```
ACKs for top commit:
jonasschnelli:
utACK c9fe61291e
Tree-SHA512: c7be316cb73d3d286bdf8429a960f71777d13a73d059869a64e23ad276499252b561a3a5b9613c4c1ad58cc0de26283c1ec72be745c401f604eaa05f70bf7d64
bf36a3ccc2 gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky)
Pull request description:
Poll function was wrongly setting cached height to the current chain height instead of the chain height at the time of polling.
This bug could cause balances to appear out of date, and was first introduced a0704a8996 (diff-2e3836af182cfb375329c3463ffd91f8L117). Before that commit, there wasn't a problem because cs_main was held during the poll update.
Currently, the problem should be rare. But if 8937d99ce81a27ae5e1012a28323c0e26d89c50b from #17954 were merged, the problem would get worse, because the wrong cachedNumBlocks value would be set if the wallet was polled in the interval between a block being connected and it processing the BlockConnected notification.
MarcoFalke also points out that a0704a8996 could lead to GUI hangs as well, because previously the pollBalanceChanged method, which runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call, but after could make blocking LOCK(cs_main) calls, potentially locking up the GUI.
Thanks to John Newbery for finding this bug this while reviewing https://github.com/bitcoin/bitcoin/pull/17954.
ACKs for top commit:
Empact:
utACK bf36a3ccc2
jonasschnelli:
utACK bf36a3c
Tree-SHA512: 1f4f229fa70a6d1fcf7be3806dca3252e86bc1755168fb421258389eb95aae67f863cb1216e6dc086b596c33560d1136215a4c87b5ff890abc8baaa3333b47f4
5f26855f10 test: Remove ubsan alignment suppressions (Wladimir J. van der Laan)
9d933ef919 prevector: avoid misaligned member accesses (Anthony Towns)
Pull request description:
Ensure prevector data is appropriately aligned. Earlier discussion in #17530.
**Edit laanwj**: In contrast to #17530, it does this without increase in size of any of the coin cache data structures (x86_64, clang)
| Struct | (size,align) before | (size,align) after |
| ------------- | ------------- | ------- |
| Coin | 48, 8 | 48, 8 |
| CCoinsCacheEntry | 56, 8 | 56, 8 |
| CScript | 32, 1 | 32, 8 |
ACKs for top commit:
laanwj:
ACK 5f26855f10
practicalswift:
ACK 5f26855f10
jonatack:
ACK 5f26855f10
Tree-SHA512: 98d112d6856f683d5b212410b73f3071d2994f1efb046a2418a35890aa1cf1aa7c96a960fc2e963fa15241e861093c1ea41951cf5b4b5431f88345eb1dd0a98a
