d27b9a2248476439ddab7700327f074005a810d5 test: fix feature_init.py file perturbation (Martin Zumsande)
ad66ca1e475d2546dbbda206465307613108a15d init: abort loading of blockindex in case of missing height. (Martin Zumsande)
Pull request description:
When the block index database is non-contiguous due to file corruption (i.e. it contains indexes of height `x-1` and `x+1`, but not `x`), bitcoind can currently crash with an assert in `BuildSkip()` / `GetAncestor()` during `BlockManager::LoadBlockIndex()`:
```
bitcoind: chain.cpp:112: const CBlockIndex* CBlockIndex::GetAncestor(int) const: Assertion `pindexWalk->pprev' failed.
```
This PR changes it such that we instead return an `InitError` to the user.
I stumbled upon this because I noticed that the file perturbation in `feature_init.py` wasn't working as intended, which is fixed in the second commit:
* Opening the file twice in one `with` statement would lead to `tf_read` being empty, so the test wouldn't perturb anything but replace the file with a new one. Fixed by first opening for read, then for write.
* We need to restore the previous state after perturbations, so that only the current perturbation is active and not a mix of the current and previous ones.
* I also added `checkblocks=200` to the startup parameters so that corruption in earlier blocks of `blk00000.dat` is detected during init verification and not ignored.
After fixing `feature_init.py` like that I'd run into the `assert` mentioned above (so running the testfix from the second commit without the first one is a way to reproduce it).
ACKs for top commit:
achow101:
ACK d27b9a2248476439ddab7700327f074005a810d5
furszy:
Code ACK d27b9a224
fjahr:
Code review ACK d27b9a2248476439ddab7700327f074005a810d5
Tree-SHA512: 2e54da6030c5813c86bd58f816401e090bb43c5b834764a5e3c0e55dbfe09e423f88042cab823db3742088204b274d4ad2abf58a3832a4b18328b11a30bf7094
7e4003226030a04a19c718a4b1b83b4ca40ca33f tests: assumeutxo: accept final height from either chainstate (James O'Beirne)
5bd2010f024b5bcccf1d57bae6fc36c53f5facc5 test: assumeutxo: avoid race in functional test (James O'Beirne)
7005a01c19001ab5821731597656f8bc5e8c11e3 test: add wait_for_connect to BitcoinTestFramework.connect_nodes (James O'Beirne)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/28585.
Fixes a few races within the assumeutxo tests:
- In general, `-stopatheight` can't be used with `connect_nodes` safely because the latter performs blocking assertions that are racy with the stopatheight triggering.
- Now that the snapshot chainstate is listed as `normal` after background validation, accept the final height from either chainstate.
ACKs for top commit:
MarcoFalke:
lgtm ACK 7e4003226030a04a19c718a4b1b83b4ca40ca33f
fjahr:
Code review ACK 7e4003226030a04a19c718a4b1b83b4ca40ca33f
achow101:
ACK 7e4003226030a04a19c718a4b1b83b4ca40ca33f
ryanofsky:
Code review ACK 7e4003226030a04a19c718a4b1b83b4ca40ca33f
Tree-SHA512: 8cbd2a0ca8643f94baa0ae3561dcf68c3519d5ba851c6049e1768f28cae6434f47ffc28d404bf38ed11030ce3f00aae0a8be3f6d563e6ae6680d83c928a173d8
aba4a5887b44bf7cbee9ea0a8e02bb92c1b4147b ci: Only run functional tests on windows in master (Fabian Jahr)
Pull request description:
This idea was discussed [here](https://github.com/bitcoin/bitcoin/pull/28509#issuecomment-1740841988).
ACKs for top commit:
hebasto:
ACK aba4a5887b44bf7cbee9ea0a8e02bb92c1b4147b
Tree-SHA512: 89fd6352b585bae3538d5350b0404c216a8225fe356d408c1ebe3394e7b9a190d65639f4eef310056e020909928d7a1f2de25585c97d2ac087d1a9f72af281eb
The descriptor documentation (doc/descriptors.md) and BIP380 explicitly
require that hex-encoded public keys start with 02 or 03 (compressed) or
04 (uncompressed). However, the current parsing/inference code permit 06
and 07 (hybrid) encoding as well. Fix this.
A "version" message in the V1 protocol starts with a fixed 16 bytes:
* The 4-byte network magic
* The 12-byte zero-padded command "version" plus 5 0x00 bytes
The current code detects incoming V1 connections by just looking at the
first 12 bytes (matching an earlier version of BIP324), but 16 bytes is
more precise. This isn't an observable difference right now, as a 12 byte
prefix ought to be negligible already, but it may become observable with
future extensions to the protocol, so make the code match the
specification.
68f23f57d77bc172ed39ecdd4d2d5cd5e13cf483 http: bugfix: track closed connection (stickies-v)
084d0372311e658a486622f720d2b827d8416591 http: log connection instead of request count (stickies-v)
41f9027813f849a9fd6a1479bbb74b9037990c3c http: refactor: use encapsulated HTTPRequestTracker (stickies-v)
Pull request description:
#26742 significantly increased the http server shutdown speed, but also introduced a bug (#27722 - see https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1559453982 for steps to reproduce on master) that causes http server shutdown to halt in case of a remote client disconnection. This happens because `evhttp_request_set_on_complete_cb` is never called and thus the request is never removed from `g_requests`.
This PR fixes that bug, and improves robustness of the code by encapsulating the request tracking logic. Earlier approaches (#27909, #27245, #19434) attempted to resolve this but [imo are fundamentally unsafe](https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1265614783) because of differences in lifetime between an `evhttp_request` and `evhttp_connection`.
We don't need to keep track of open requests or connections, we just [need to ensure](https://github.com/bitcoin/bitcoin/pull/19420#issue-648067169) that there are no active requests on server shutdown. Because a connection can have multiple requests, and a request can be completed in various ways (the request actually being handled, or the client performing a remote disconnect), keeping a counter per connection seems like the approach with the least overhead to me.
Fixes#27722
ACKs for top commit:
vasild:
ACK 68f23f57d77bc172ed39ecdd4d2d5cd5e13cf483
theStack:
ACK 68f23f57d77bc172ed39ecdd4d2d5cd5e13cf483
Tree-SHA512: dfa711ff55ec75ba44d73e9e6fac16b0be25cf3c20868c2145a844a7878ad9fc6998d9ff62d72f3a210bfa411ef03d3757b73d68a7c22926e874c421e51444d6
bdee8589644fac121320e95f53457c3ddfc71e1b typo: in packages.md (Erik McKelvey)
Pull request description:
Removed extra word `the` in packages.md
ACKs for top commit:
fanquake:
ACK bdee8589644fac121320e95f53457c3ddfc71e1b
Tree-SHA512: 14a745e5f8ad97f38c21c7b80e88592f84f50d87bc71930c1212fb9ba5a46b129ffec0aa1dd53780f872c0067b58bd2a65ed9def4d9f5f50dc8c7d8e34a429d4
- make `getaddrmaninfo` RPC public since it's not for development
purposes only and regular users might find it useful
- add missing `all_networks` key to RPC help
- use clang format spacing
fac054d24c4b6fe3370e458ad6b626f98bd018d6 ci: Print Linux kernel info (MarcoFalke)
Pull request description:
Required to debug issues like https://github.com/bitcoin/bitcoin/pull/28487#issuecomment-1729717923. For example:
```
FATAL: ThreadSanitizer: unexpected memory mapping 0x57cf8f031000-0x57cf8f173000
ACKs for top commit:
hebasto:
ACK fac054d24c4b6fe3370e458ad6b626f98bd018d6
Tree-SHA512: 7eb158e52daffe5cbcdfa3ed1efb45e1930b80a2672558fe400c8d72ce59a8cbeb02296dfc2032721d511410885a1f057672fe8086ba1c72a494aef541bf7eb4
352d5eb2a9e89cff4a2815d94a9d81fcc20c4b2c test: getrawaddrman RPC (0xb10c)
da384a286bd84a97e7ebe7a64654c5be20ab2df1 rpc: getrawaddrman for addrman entries (0xb10c)
Pull request description:
Inspired by `getaddrmaninfo` (#27511), this adds a hidden/test-only `getrawaddrman` RPC. The RPC returns information on all addresses in the address manager new and tried tables. Addrman table contents can be used in tests and during development.
The RPC result encodes the `bucket` and `position`, the internal location of addresses in the tables, in the address object's string key. This allows users to choose to consume or to ignore the location information. If the internals of the address manager implementation change, the location encoding might change too.
```
getrawaddrman
EXPERIMENTAL warning: this call may be changed in future releases.
Returns information on all address manager entries for the new and tried tables.
Result:
{ (json object)
"table" : { (json object) buckets with addresses in the address manager table ( new, tried )
"bucket/position" : { (json object) the location in the address manager table (<bucket>/<position>)
"address" : "str", (string) The address of the node
"port" : n, (numeric) The port number of the node
"network" : "str", (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the address
"services" : n, (numeric) The services offered by the node
"time" : xxx, (numeric) The UNIX epoch time when the node was last seen
"source" : "str", (string) The address that relayed the address to us
"source_network" : "str" (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the source address
},
...
},
...
}
Examples:
> bitcoin-cli getrawaddrman
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getrawaddrman", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
```
ACKs for top commit:
willcl-ark:
reACK 352d5eb2a9
amitiuttarwar:
reACK 352d5eb2a9e
stratospher:
reACK 352d5eb.
achow101:
ACK 352d5eb2a9e89cff4a2815d94a9d81fcc20c4b2c
Tree-SHA512: cc462666b5c709617c66b0e3e9a17c4c81e9e295f91bdd9572492d1cb6466fc9b6d48ee805ebe82f9f16010798370effe5c8f4db15065b8c7c0d8637675d615e
7df450836969b81e98322c9a09c08b35d1095a25 test: improve sock_tests/move_assignment (Vasil Dimov)
5086a99b84367a45706af7197da1016dd966e6d9 net: remove Sock default constructor, it's not necessary (Vasil Dimov)
7829272f7826511241defd34954e6040ea963f07 net: remove now unnecessary Sock::Get() (Vasil Dimov)
944b21b70ae490a5a746bcc1810a5074d74e9d34 net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov)
aeac68d036e3cff57ce155f1a904d77f98b357d4 net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov)
5ac1a51ee5a57da59f1ff1986b7d9054484d3c80 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing.
Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary.
The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it.
ACKs for top commit:
ajtowns:
ACK 7df450836969b81e98322c9a09c08b35d1095a25
jonatack:
ACK 7df450836969b81e98322c9a09c08b35d1095a25
Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf
bae209e3879fa099302d3b211362c49bbbfbdd14 gui: macOS, make appMenuBar part of the main app window (furszy)
e14cc8fc69cb3e3a98076fbb23a94eba7873368a gui: macOS, do not process dock icon actions during shutdown (furszy)
Pull request description:
As the 'QMenuBar' is created without a parent window in MacOS, the app crashes when the user presses the shutdown button and, right after it, triggers any action in the menu bar.
This happens because the QMenuBar is manually deleted in the BitcoinGUI destructor but the events attached to it children actions are not disconnected, so QActions events such us the 'QMenu::aboutToShow' could try to access null pointers.
Instead of guarding every single QAction pointer inside the QMenu::aboutToShow slot, or manually disconnecting all registered events in the destructor, we can check if a shutdown was requested and discard the event.
The 'node' field is a ref whose memory is held by the main application class, so it is safe to use here. Events are disconnected prior destructing the main application object.
Furthermore, the 'MacDockIconHandler::dockIconClicked' signal can make the app crash during shutdown for the very same reason. The 'show()' call triggers the 'QApplication::focusWindowChanged' event, which is connected to the 'minimize_action' QAction, which is also part of the app menu bar, which could no longer exist.
Another cause of crashes stems from the shortcuts provided by the `appMenuBar` submenus during shutdown. For instance, executing actions like opening the information dialog (command + I) or the console dialog (command + T) lead to access null pointers. The second commit addresses and resolves these issues.
Basically, in the present setup, we create a parentless `appMenuBar` whose submenus `QActions` are connected to `qApp` events (the app's global instance). However, at the `BitcoinGUI` destructor, we manually destruct this object without properly disconnecting the events. This leaves `qApp` events, such as `focusWindowChanged`, tied to submenus' `QAction` pointers, which causes the application to crash when it attempts to access them.
Important Note:
This happened to me few times. The worst consequence was an inconsistent chain state during IBD. Which triggered a full "replay blocks" process on the next startup. Which was painfully slow.
ACKs for top commit:
RandyMcMillan:
utACK bae209e
hebasto:
ACK bae209e3879fa099302d3b211362c49bbbfbdd14.
Tree-SHA512: 432e19c5f7e02c3165b7e7bd7f96f2a902bae5b5e439c2594db1c69d74ab6e0d4509d90f02db8c076f616e567e6a07492ede416ef651b5f749637398291b92fd
It is possible that the client disconnects before the request is
handled. In those cases, evhttp_request_set_on_complete_cb is never
called, which means that on shutdown the server we'll keep waiting
endlessly.
By adding evhttp_connection_set_closecb, libevent automatically
cleans up those dead connections at latest when we shutdown, and
depending on the libevent version already at the moment of remote
client disconnect. In both cases, the bug is fixed.
Introduces and uses a HTTPRequestTracker class to keep track of
how many HTTP requests are currently active, so we don't stop the
server before they're all handled.
This has two purposes:
1. In a next commit, allows us to untrack all requests associated
with a connection without running into lifetime issues of the
connection living longer than the request
(see https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1265614783)
2. Improve encapsulation by making the mutex and cv internal members,
and exposing just the WaitUntilEmpty() method that can be safely
used.
360b917674e63c1e95119040463b3f50976bf331 contrib/bash-completions: use package naming conventions (Erik Arvstedt)
Pull request description:
#### Copy of commit msg
This naming scheme supports auto-detection and on-demand loading of completions.
See
ba109693ee/README.md (faq),
section "Where should I put it to be sure that interactive bash shells will find it and source it", keyword `foo.bash`.
Previously, distro package maintainers had to rename these files manually.
ACKs for top commit:
willcl-ark:
ACK 360b917674e63c1e95119040463b3f50976bf331
Tree-SHA512: 6dd1f62309e877402fac2f7aec785d3053a1fd3fdeae38abc961c3f61269eb664a2489b6fa8294bd706d8ac387c7af3ac66cd7a852810f69371fa8ae991afacb
fa6e6a3f03a38f8b431bf694268ed344d1815b3b doc: Remove confusing assert linter (MarcoFalke)
Pull request description:
The `assert()` documentation and linter are redundant and confusing:
* The source code already refuses to compile with `assert()` disabled.
* They violate the assumptions about `Assert()`, which *requires* side effects.
* The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects.
Fix all issues by removing the docs and the linter. See also https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1287370102
Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit.
ACKs for top commit:
hebasto:
ACK fa6e6a3f03a38f8b431bf694268ed344d1815b3b, I have reviewed the code and it looks OK.
theStack:
ACK fa6e6a3f03a38f8b431bf694268ed344d1815b3b
Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236
75a329103505736acb9036224da2dfa8ab038c43 doc: mention BIP324 support in bips.md (Pieter Wuille)
64ca7210f05c4003228f4cb0b160d869e15f47d2 test: enable v2 transport between nodes in some functional tests (Pieter Wuille)
05d19fbcc10f26c7f1e3a9afc660eb7fa71b1d8c test: Functional test for opportunistic encryption (dhruv)
b815cce50e4bfa0efea8ea02659b7042c8fb18be net: expose transport types/session IDs of connections in RPC and logs (Pieter Wuille)
432a62c4dce908729c62edcfaebc3da6387c3afe net: reconnect with V1Transport under certain conditions (Pieter Wuille)
4d265d0342ae7e92df07ba51e8355db57c44f811 sync: modernize CSemaphore / CSemaphoreGrant (Pieter Wuille)
c73cd423636e06df46742f573640ca773b281ffc rpc: addnode arg to use BIP324 v2 p2p (dhruv)
62d21ee0974b582a6a32aa97ee35ef51c977ea4b net: use V2Transport when NODE_P2P_V2 service flag is present (Pieter Wuille)
a4706bc877504057e8522c929cc0704d3eaa7302 rpc: don't report v2 handshake bytes in the per-type sent byte statistics (Sebastian Falbesoner)
abf343b32026c3f8246f98c416e2c6cf5b66aa38 net: advertise NODE_P2P_V2 if CLI arg -v2transport is on (Pieter Wuille)
Pull request description:
Part of #27634.
This makes BIP324 support feature complete, through a (default off) `-v2transport` option for enabling V2 connections. If it is enabled:
* The `NODE_P2P_V2` service flag (*1 << 11*) is advertized.
* Inbound connections can use V1 or V2 (automatically detected based on the protocol used by the peer)
* V2 connections are used on outbound when the `NODE_P2P_V2` service is available (or the new `use_v2` parameter is set on the `addnode` RPC).
* V2 outbound connections that instantly fail get retried as V1.
There are two new RPC fields, `"transport_protocol_type"` and `"session_id"`, in `getpeerinfo`.
ACKs for top commit:
mzumsande:
re-ACK 75a329103505736acb9036224da2dfa8ab038c43
theStack:
re-ACK 75a329103505736acb9036224da2dfa8ab038c43
Tree-SHA512: 90ea1cd37f3dce410a59ff5de1c2405891e8aa62318d0e06dcb68b21603fb0c061631526633f3d4fb630e63d2b8db407eed48e246befcbef3503bea893a4ff15
When an outbound v2 connection is disconnected without receiving anything, but at
least 24 bytes of our pubkey were sent out (enough to constitute an invalid v1
header), add them to a queue of reconnections to be tried.
The reconnections are in a queue rather than performed immediately, because we should
not block the socket handler thread with connection creation (a blocking operation
that can take multiple seconds).
