6927933782 [net processing] Add ChainSyncTimeoutState default initializers (John Newbery)
55966e0cc0 [net processing] Remove CNodeState ctor body (John Newbery)
Pull request description:
This addresses the two outstanding review comments from #21370.
ACKs for top commit:
practicalswift:
cr ACK 6927933782: patch looks correct
hebasto:
ACK 6927933782, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: b3ef5c8a096e447887df255406b3a760f01c73e2b942374595416b4b4031fc69b89cd93168c45040489d581f340b2a62d3fbabd207d4307f587c00a7a7daacd1
7c8c140ecc fuzz: Implement fuzzed_dns_lookup_function as lambda (practicalswift)
Pull request description:
Implement `fuzzed_dns_lookup_function` as a lambda.
As wisely suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/19415#discussion_r594244506. Thanks! :)
ACKs for top commit:
MarcoFalke:
cr ACK 7c8c140ecc
vasild:
ACK 7c8c140ecc
Tree-SHA512: b175f2ad42e9a2be1f769ac677b2872e73ae621731d27e9a24bedadc14d9a6682c7fd1946a0df436d37e7b0cc0d212c1eef69f0409fb975cf9c460cd45f6e4ac
e528075189 tests: Add fuzzing harness for Lookup(...)/LookupHost(...)/LookupNumeric(...)/LookupSubNet(...) (practicalswift)
c6b4bfb4b3 net: Make DNS lookup code testable (practicalswift)
Pull request description:
Make DNS lookup mockable/testable/fuzzable.
Add fuzzing harness for `Lookup(…)`/`LookupHost(…)`/`LookupNumeric(…)`/`LookupSubNet(…)`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
Crypt-iQ:
cr ACK e528075189
vasild:
ACK e528075189
Tree-SHA512: 9984c2e2fedc3c1e1c3dbd701bb739ebd2f01766e6e83543dae5ae43eb8646c452bba0e101dd2f06079e5258bd5846c7d27a60ed5d77c1682b54c9544ffad443
2f0b25a156 rpc: remove scantxoutset EXPERIMENTAL warning (Jon Atack)
Pull request description:
Remove old warning per IRC wallet meeting discussion at http://www.erisian.com.au/bitcoin-core-dev/log-2021-03-12.html#l-467
This RPC was merged 3 years ago in #12196.
ACKs for top commit:
MarcoFalke:
cr ACK 2f0b25a156
Tree-SHA512: 874ccd5bd19ecbbe91912171ac85af7a4658dc92f6db484ff3d03f07f1b9ba97e1c69d33a5c3ae5c5ec46cac3595a211f55fec0fbf81bac30d66a891c376ce26
9048c58e10 Remove pointer cast in CRPCTable::dumpArgMap (Russell Yanofsky)
14f3d9b908 refactor: Add RPC server ExecuteCommands function (Russell Yanofsky)
6158a6d397 refactor: Replace JSONRPCRequest fHelp field with mode field (Russell Yanofsky)
Pull request description:
This change is needed to fix the `rpc_help.py` test failing in #10102: https://cirrus-ci.com/task/5469433013469184?command=ci#L2275
The [`CRPCTable::dumpArgMap`](16b784d953/src/rpc/server.cpp (L492)) method currently works by casting RPC `unique_id` integer field to a function pointer, and then calling it. The `unique_id` field wasn't supposed to be used this way (it's meant to be used to detect RPC aliases) and as a result, this code segfaults in the `rpc_help.py` test in multiprocess PR #10102 because wallet RPC functions aren't directly accessible from the node process.
Fix this by adding a new `GET_ARGS` RPC request mode to retrieve argument information similar to the way the `GET_HELP` mode retrieves help information.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
MarcoFalke:
re-ACK 9048c58e10👑
Tree-SHA512: cd1a01c1daa5bde2c2455b63548371ee4cf39688313969ad2016d9a0fd4344102e3fd43034058f253364518e9632d57cf21abffad0d6a2c0c94b7a6921cbe615
def1e64bb4 scripted-diff: Drop redundant QString calls (Hennadii Stepanov)
Pull request description:
The return type of `QObject::tr` function _is_ `QString` 🐅
ACKs for top commit:
jarolrod:
ACK def1e64bb4, tested on macOS 10.14.6 Qt 5.15.2
Tree-SHA512: ef405c87a30d6965f6887511d8666b6da57d258ca07833a3fa2dc9fd147d0539d33c57f7551ee13c1dd8024d6057139595c6ce5d088dd6efd7aa13db2a3eebdb
fa7ff0790e rpc: Properly document submitblock return value (MarcoFalke)
fae542c28b rpc: Properly document getblocktemplate return value (MarcoFalke)
fabaccf031 rpc: Properly document scantxoutset return value (MarcoFalke)
faa2059547 rpc: Properly document gettxout return value (MarcoFalke)
Pull request description:
Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476
ACKs for top commit:
fjahr:
utACK fa7ff0790e
amitiuttarwar:
tACK fa7ff0790e
Tree-SHA512: 933cb8f003163d93dbedb302d4c162514c2698ec6d58dbb9a053da8b8b9a4459b0701a3d9e830ecdabd7f278a46b7a07a3af49ec60703a80fcd75390877294ea
e4c0cada79 ci, gitian: Drop unneeded python3-dev package for macOS builds (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
fanquake:
ACK e4c0cada79 - gitian builds match and I checked that this doesn't end up installed as a side-effect of another package.
Tree-SHA512: 520a3909b106a0e005b195c5395691edf62b76ee2df43b6971b7aa193648d68e6dac69cb4f1dc474f594b015a2fc2074061865e571d89365174beb5c1780356f
95f97111dd contrib/init: (OpenRC) quote some unquoted variables. (parazyd)
737feadff7 contrib/init: (OpenRC) Do not fail if both rpcuser and rpcpassword are unset. (parazyd)
Pull request description:
This pull request improves the available OpenRC initscripts in
`contrib/init`.
The first commit (737feadff7) reworks
`checkconfig()` to not fail if **both** `rpcuser` and `rpcpassword`
are unset, because this implies that bitcoind shall use the `.cookie`
file for RPC authentication. Currently, the initscript does not allow
starting bitcoind without a set `rpcuser` and `rpcpassword`.
The second commit (95f97111dd) simply
quotes some unquoted variables.
ACKs for top commit:
kristapsk:
ACK 95f97111dd
Tree-SHA512: 62bebcd07143c147e349c0cfc17b54ef21bd4684377b444f58c6bd1f509a4d3e1af58746fa7215f18e33021f691bbbc5e42f4df497458322b055e545b7f30d46
1a6323bdbe doc: update developer notes for removal of MakeUnique (fanquake)
3ba2840e7e scripted-diff: remove MakeUnique<T>() (fanquake)
Pull request description:
Since requiring C++17, this is just pointless abstraction. I think we should just "tear the band-aid off" and remove it. Similar to the changes happening in #21366.
Also, having a comment saying this is deprecated doesn't prevent it's usage in new code. i.e : https://github.com/bitcoin/bitcoin/pull/20946#discussion_r561949731.
The repository is fairly quiet at the moment, so any potential complaints about having to rebase should be minimal. Might as well get this over and done with.
ACKs for top commit:
jnewbery:
utACK 1a6323bdbe
practicalswift:
cr ACK 1a6323bdbe: patch looks correct
ajtowns:
ACK 1a6323bdbe -- code review only
glozow:
ACK 1a6323bdbe looks correct
Tree-SHA512: 4a14b9611b60b9b3026b54d6f5a2dce4c5d9b63a7b93d7de1307512df736503ed84bac66e7b93372c76e3117f49bf9f29cd473d3a47cb41fb2775bc10234736f
ebde946a52 [doc] Improve comment about protected peers (Amiti Uttarwar)
Pull request description:
The comment currently suggests a long-standing node would infrequently protect peers under normal circumstances. Clarify that we also protect peers that are synced to the same work as our chain tip. [Relevant check here](ee0dc02c6f/src/net_processing.cpp (L1997)).
ACKs for top commit:
Empact:
ACK ebde946a52
jnewbery:
ACK ebde946a52
Tree-SHA512: 3692f4098e95f935d801e0ee6bbd3a7c9480e66ca070a7c68ba79c4fc2e62377f5d37080c7b6a7d15ab617aaf4d3df9b26abc4f1b090d572ba46fdd092a6a64a
cc3971c9ff GUI: Write PSBTs to file with binary mode (Andrew Chow)
Pull request description:
As noted in https://github.com/bitcoin/bitcoin/issues/20959, PSBT files should be opened in binary mode as on windows, all newlines are turned into CRLF which produces invalid PSBTs.
Fixes https://github.com/bitcoin/bitcoin/issues/20959
ACKs for top commit:
Talkless:
utACK cc3971c9ff.
Tree-SHA512: fee62b66da844017a44d7d6da6d2d2794b097a7dec33fb07711615df1e94dccc76f987ffcbb325ad1f8db2a2dd6eaf514b6cbd2453e7658b9f6c9fb5c4c41dab
c62f9bc0e9 test: use fewer blocks in wallet_groups and move sync call (Jon Atack)
3a16b5ef95 test: add missing logging to wallet_groups.py (Jon Atack)
Pull request description:
- add logging (particularly useful as the tests are somewhat slow)
- generate 101 blocks instead of 110
- move `sync_all` call into the loop, so fewer blocks are synced on each call, to hopefully see fewer CI timeouts as in https://bitcoinbuilds.org/index.php?ansilog=88eee99e-1727-44ed-b778-3b9c75c33928.log
```
L2742 File "/home/ubuntu/src/test/functional/wallet_groups.py", line 162, in run_test
L2743 self.sync_all()
test_framework.authproxy.JSONRPCException: 'syncwithvalidationinterfacequeue' RPC took longer than 960.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
ACKs for top commit:
MarcoFalke:
cr ACK c62f9bc0e9
Tree-SHA512: 711deafcd589cb8196cb207ff882e0f2ab6b70828a6abad91f83f535974cc430a56b9e8a960fd233d31d610932a0d48b49ee681aae564d145a3040288ecda8f8
e017a913d0 bitcoind: Add -daemonwait option to wait for initialization (Wladimir J. van der Laan)
c3e6fdee6d shutdown: Use RAII TokenPipe in shutdown (Wladimir J. van der Laan)
612f746a8f util: Add RAII TokenPipe (Wladimir J. van der Laan)
Pull request description:
This adds a `-daemonwait` flag that does the same as `-daemon` except that it, from a user perspective, backgrounds the process only after initialization is complete. This is similar to the behaviour of some other software such as c-lightning.
This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result.
The use of the libc function `daemon()` is replaced by a custom implementation which is inspired by the [glibc implementation](https://github.com/lattera/glibc/blob/master/misc/daemon.c#L44), but which also creates a pipe from the child to the parent process for communication.
An additional advantage of having our own `daemon()` implementation is that no MACOS-specific pragmas are needed anymore to silence a deprecation warning.
TODO:
- [x] Factor out `token_read` and `token_write` to an utility, and use them in `shutdown.cpp` as well—this is exactly the same kind of communication mechanism.
- [x] RAII-ify pipe endpoints.
- [x] Improve granularity of the `configure.ac` checks. This currently still checks for the function `daemon()` which makes no sense as it's not used. It should check for individual functions such as
`fork()` and `setsid()` etc—the former being required, the second optional.
- [-] ~~Signal propagation during initialization: if say, pressing Ctrl-C during `-daemonwait` it would be good to pass this SIGINT on to the child process instead of detaching the parent process and letting the child run free.~~ This is not necessary, see https://github.com/bitcoin/bitcoin/pull/21007#issuecomment-769007341.
Future:
- Consider if it makes sense to use this in the RPC tests (there would be no more need for "is RPC ready" polling loops). I think this is out of scope for this PR.
ACKs for top commit:
jonatack:
Tested ACK e017a913d0 checked change since previous review is move-only
Tree-SHA512: 53369b8ca2247e4cf3af8cb2cfd5b3399e8e0e3296423d64be987004758162a7ddc1287b01a92d7692328edcb2da4cf05d279b1b4ef61a665b71440ab6a6dbe2
550ed1bed2 build: update qt qpaint non determinism patch for 5.12.10 (fanquake)
6093ae4d30 build: update qt no-xlib patch for 5.12.10 (fanquake)
84928c4e73 build: update qt android jni static patch for 5.12.10 (fanquake)
cc6f47d51a build: update qt lrelease patch for 5.12.10 (fanquake)
286d07ff17 build, qt: Fix lib paths in *.pc files (Hennadii Stepanov)
fa5e97e8c2 build: disable qt SDK version checking (fanquake)
1be8e0f238 build: Add QMacStyle support (Hennadii Stepanov)
e674e94302 build: revert to using Qts internal zlib (fanquake)
06cd0da21f build: qt 5.12.10 (fanquake)
3272e34f9c build: Add xkbcommon 0.8.4 (Hennadii Stepanov)
d769b3372d build: only pass -optimized-tools to qt in debug mode (fanquake)
Pull request description:
Switch to Qt 5.12.10 in depends. Based on #21363. This is a much smaller changeset, and should be easier to review than #19716. Also postpones needing to bring a bunch of new libs into depends.
Big thanks to Hebasto that has been helping with this.
ACKs for top commit:
laanwj:
Code review ACK 550ed1bed2
hebasto:
ACK 550ed1bed2
jarolrod:
ACK 550ed1bed2 , tested on macOS 11.2 built from depends
Tree-SHA512: cb6b70f5a5372ba0b64f7ddfa696eda0411922cd261c67bfa2d9332c685a7b358ab18e5cfaa677b414ae8ad78296bba6ed0eecd071fdacdf736a0d030f679fe5
a67983cd6d net_processing: Add review-only assertion to PeerManager (Carl Dong)
272d993e75 scripted-diff: net_processing: Use existing chainman (Carl Dong)
021a04a469 net_processing: Move some static functions to PeerManager (Carl Dong)
91c5b68acd node/ifaces: ChainImpl: Use existing NodeContext member (Carl Dong)
8a1d580b21 node/ifaces: NodeImpl: Use existing NodeContext member (Carl Dong)
4cde4a701b node: Use existing NodeContext (Carl Dong)
106bcd4f39 node/coinstats: Pass in BlockManager to GetUTXOStats (Carl Dong)
2c3ba00693 miner: Pass in blockman to ::RegenerateCommitments (Carl Dong)
2afcf24408 miner: Remove old CreateNewBlock w/o chainstate param (Carl Dong)
46b7f29340 scripted-diff: Invoke CreateNewBlock with chainstate (Carl Dong)
d0de61b764 miner: Pass in chainstate to BlockAssembler::CreateNewBlock (Carl Dong)
a04aac493f validation: Remove extraneous LoadGenesisBlock function prototype (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #21055 | [Bundle 3/n] Prune g_chainman usage in mempool-related validation functions
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
laanwj:
Code review ACK a67983cd6d
ryanofsky:
Code review ACK a67983cd6d. Only change since last review new first commit fixing header declaration, and rebase
glozow:
code review ACK a67983cd6d
Tree-SHA512: dce182a18b88be80cbf50978d4ba8fa6ab0f01e861d09bae0ae9364051bb78f9334859d164b185b07f1d70a583e739557fab6d820cac8c37b3855b85c2a6771b
c524dc54bb qt: Fix regression with initial sorting after pr205 (Hennadii Stepanov)
Pull request description:
Unfortunately, #205 introduced a regression. After opening the "Receive" or "Transaction" tab at first time despite of the "Date" header is marked as sorted, table rows are not sorted actually:
It appears that sorting the table must be triggered _after_ the `QTableView::setModel` call.
With this PR (and pre-#205):
ACKs for top commit:
Talkless:
tACK c524dc54bb, tested on Debian Sid with Qt 5.15.2. I can confirm @leonardojobim observations.
leonardojobim:
Tested ACK c524dc54bb on Ubuntu 20.04.2 Qt 5.12.8
jonatack:
ACK c524dc54bb
jarolrod:
ACK c524dc54bb, tested on macOS 11.1 Qt 5.15.2
Tree-SHA512: e370229979a70d63a0b64dbc11c4eca338695a070881d4d8f015644617f180e6accc24d6bdf98a75e7c9ba9be2a0ace9a2b7eb9c783ebb2992c3b2c3b3deb408
7c90c67b7e rpc: refactor rpc wallet functions to take references instead of pointers (fanquake)
4866934008 rpc: remove calls to CWallet.get() (fanquake)
Pull request description:
This is a rebased #18592.
> This PR replaces raw pointers in `rpcwallet.cpp` and `rpcdump.cpp` with **shared_ptr**. The motivation for this PR is described here https://github.com/bitcoin/bitcoin/issues/18590
> It seems that this PR is indirectly related to this issue: https://github.com/bitcoin/bitcoin/pull/13063#discussion_r186740049
> Notice: I have deliberately **not** changed the class `WalletRescanReserver ` whose constructor expects a raw pointer, because it's external and affects other areas, which I didn't touch to avoid making this PR "viral".
> Fixes https://github.com/bitcoin/bitcoin/issues/18590
ACKs for top commit:
MarcoFalke:
ACK 7c90c67b7e🐧
ryanofsky:
Code review ACK 7c90c67b7e. Changes easy to review with `--word-diff-regex=. -U0`
Tree-SHA512: 32d69c813026b02260e8a89de9d6a5ab9e87826ba230687246583ac7a80c8c3fd00318da4658f1450e04c23d2c77ae765862de0d2a110b1312b3b69a1161e7ba
fad0ae6bb8 doc: Rename fuzz seed_dir to corpus_dir (MarcoFalke)
Pull request description:
The fuzz corpus directory might contain hand-crafted seeds, but generally it is a set of test inputs. See also https://github.com/google/fuzzing/blob/master/docs/glossary.md#corpus
ACKs for top commit:
practicalswift:
cr ACK fad0ae6bb8: patch looks correct and "why not?" :)
fanquake:
ACK fad0ae6bb8 - did not test
Tree-SHA512: 38c952feb07aeeeb038b3261a12c824fab9ce5153d75f0ecf6d3f43db4f50998eeb2b14b11b7155f529189c93783fa2c11c81059021a04398c43f3505b31a2d4
See:
- QTBUG-72903, commit 9864d2c6f3b628ca9f07a56b197e77bd43931cca
- QTBUG-78873, commit e55a61a77f0c87c05661a0335dfdb12673c6a27f
Could be dropped for Qt 5.14+.
This tries to invoke xcrun, which is not available when cross-compiling.
Given we are in control of the SDK versions being used, removing this
check has minimal-no effect.
remove fix_configure_mac.patch
Fixed upstream: https://bugreports.qt.io/browse/QTBUG-67286
remove fix_riscv64_arch.patch
Was fixed upstream in 6a39e49a6cdeb28a04a3657bb6a22f848d5dfa9d
remove fix_rcc_determinism.patch
Fixed upstream in https://bugreports.qt.io/browse/QTBUG-62511
remove freetype_back_compat.patch
By the time we ship a release with Qt 5.12, we'll certainly no-longer be
supporting Ubuntu 14.04 and Ubuntu 16.04 ships with FreeType 2.6.1,
which is new enough that using the symbol is no-longer an issue.
The renaming of FT_Get_X11_Font_Format() happened in FreeType 2.6
remove xkb-default.patch
This was removed upstream in d5abf545971da717014d316127045fc19edbcd65
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
ffdd7de690 build, qt: Fix regression introduced in #21363 (Hennadii Stepanov)
Pull request description:
After #21363 the `_BITCOIN_QT_CHECK_STATIC_PLUGIN` never fails due to the ill-formed code.
Sorry for breaking it.
ACKs for top commit:
fanquake:
ACK ffdd7de690
Tree-SHA512: 0af3334af5332f3b349dba219565ee336090541c2c4fae53d7dc4dd2aa05291b3c7c13bfc506219f642fb854f3c887f0f9621b42d0df68d5d794e0563b2cf5d4
fa476f188e Use C++11 member initializer in CNodeState (MarcoFalke)
Pull request description:
This removes a bunch of boilerplate, makes the code easier to read. Also, C++11 member initialization avoids accidental uninitialized members.
ACKs for top commit:
practicalswift:
cr ACK fa476f188e
hebasto:
cr ACK fa476f188e
jnewbery:
utACK fa476f188e
Tree-SHA512: 5c876717d30ded975e29bfbc77804012179588a13f950f0b2ec93fa9dbd5cf6b52fe86414fd5d1cce021db2ec77e271d533b0f7a8d6eeaac0feb9e6dbaec9ff2
fa4e088cba wallet: Mark replaced tx to not be in the mempool anymore (MarcoFalke)
Pull request description:
The wallet does not mark the replaced tx as out-of-mempool. This causes failures in user scripts, because later RPCs may depend on this state change from `bumpfee`.
For example, the following might fail on current master:
```
txid = sendtoaddress(...)
bumpfee(txid)
abandontransaction(txid) # fails because txid is still marked as "in mempool"
```
Fixes#18831
ACKs for top commit:
meshcollider:
utACK fa4e088cba
ryanofsky:
Code review ACK fa4e088cba, and previous ACK faeedff5c87091fd83d2fb2b29eb49c948363f29 is also still valid in case there's a preference for the original fix
Tree-SHA512: 9858f40f5fb5a43a7b584b5c4268b6befa82e6a84583be5206fe721bcb6c255e8d35479d347d0b9aed72703df49887c02b14ab680e8efdd28b90dd6b93d9439a
The comment currently suggests a long-standing node would infrequently protect
peers under normal circumstances. Clarify that we also protect peers that are
synced to the same work as our chain tip.
