07cd510ffe [refactor] consolidate invalid MempoolAcceptResult processing (glozow)
9353aa4066 [refactor] consolidate valid MempoolAcceptResult processing (glozow)
Pull request description:
Every time we try to `ProcessTransaction` (i.e. submit a tx to mempool), we use the result to update a few net processing data structures. For example, after a failure, the {wtxid, txid, both, neither} (depending on reason) should be cached in `m_recent_rejects` so we don't try to download/validate it again.
There are 2 current places and at least 1 future place where we need to process `MempoolAcceptResult`:
- In the `ProcessMessage` logic after receiving and validating a tx
- In `ProcessOrphanTx` where we retry orphans
- With #28970, after processing a package of transactions, we should do these updates for each tx in the package.
Consolidate this code so it isn't repeated in 2 places and so we can reuse it in a future PR.
ACKs for top commit:
instagibbs:
ACK 07cd510ffe
achow101:
ACK 07cd510ffe
dergoegge:
Code review ACK 07cd510ffe
TheCharlatan:
ACK 07cd510ffe
Tree-SHA512: c4e74cb65e4f52882fca52e6682efa5dcf1562d98418454e09be256ffd026caae642a90aa5b9cccaae214be240d6f4be9d87b516953b2ee69a655f16ea569ed9
567cec9a05 doc: add release notes and help text for unix sockets (Matthew Zipkin)
bfe5192891 test: cover UNIX sockets in feature_proxy.py (Matthew Zipkin)
c65c0d0163 init: allow UNIX socket path for -proxy and -onion (Matthew Zipkin)
c3bd43142e gui: accomodate unix socket Proxy in updateDefaultProxyNets() (Matthew Zipkin)
a88bf9dedd i2p: construct Session with Proxy instead of CService (Matthew Zipkin)
d9318a37ec net: split ConnectToSocket() from ConnectDirectly() for unix sockets (Matthew Zipkin)
ac2ecf3182 proxy: rename randomize_credentials to m_randomize_credentials (Matthew Zipkin)
a89c3f59dc netbase: extend Proxy class to wrap UNIX socket as well as TCP (Matthew Zipkin)
3a7d6548ef net: move CreateSock() calls from ConnectNode() to netbase methods (Matthew Zipkin)
74f568cb6f netbase: allow CreateSock() to create UNIX sockets if supported (Matthew Zipkin)
bae86c8d31 netbase: refactor CreateSock() to accept sa_family_t (Matthew Zipkin)
adb3a3e51d configure: test for unix domain sockets (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27252
UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.
There has been work on [unix domain sockets before](https://github.com/bitcoin/bitcoin/pull/9979) but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by `-onion=` or `-proxy=`
I copied the prefix `unix:` usage from Tor. With this patch built locally you can test with your own filesystem path (example):
`tor --SocksPort unix:/Users/matthewzipkin/torsocket/x`
`bitcoind -proxy=unix:/Users/matthewzipkin/torsocket/x`
Prep work for this feature includes:
- Moving where and how we create `sockaddr` and `Sock` to accommodate `AF_UNIX` without disturbing `CService`
- Expanding `Proxy` class to represent either a `CService` or a UNIX socket (by its file path)
Future work:
- Enable UNIX sockets for ZMQ (https://github.com/bitcoin/bitcoin/pull/27679)
- Enable UNIX sockets for I2P SAM proxy (some code is included in this PR but not tested or exposed to user options yet)
- Enable UNIX sockets on windows where supported
- Update Network Proxies dialog in GUI to support UNIX sockets
ACKs for top commit:
Sjors:
re-ACK 567cec9a05
tdb3:
re ACK for 567cec9a05.
achow101:
ACK 567cec9a05
vasild:
ACK 567cec9a05
Tree-SHA512: de81860e56d5de83217a18df4c35297732b4ad491e293a0153d2d02a0bde1d022700a1131279b187ef219651487537354b9d06d10fde56225500c7e257df92c1
d0e6564240 log: Remove error() reference (Fabian Jahr)
Pull request description:
Mini-followup to #29236 that was just merged. Removes a reference to `error()` that was missed in a comment.
ACKs for top commit:
ryanofsky:
Code review ACK d0e6564240. Just dropped LogPrintf reference since last review
stickies-v:
ACK d0e6564240
Empact:
ACK d0e6564240
Tree-SHA512: 8abe4895951013c2ceca9a57743aacabaf8af831d07eee9ae8372c121c16e88b7226f0e537200c3464792e19ac7e03b57ba0be31f43add8802753972b0aefc48
e710cefd57 rest: read raw block in rest_block and deserialize for json (Andrew Toth)
95ce0783a6 rpc: read raw block in getblock and deserialize for verbosity > 0 (Andrew Toth)
0865ab8712 test: check more details on zmq raw block response (Andrew Toth)
38265cc14e zmq: read raw block with ReadRawBlockFromDisk (Andrew Toth)
da338aada7 blockstorage: check nPos in ReadRawBlockFromDisk before seeking back (Andrew Toth)
Pull request description:
For the `getblock` endpoint with `verbosity=0`, the `rest_block` REST endpoint for `bin` and `hex`, and zmq `NotifyBlock` we don't have to deserialize the block since we're just sending the raw data. This PR uses `ReadRawBlockFromDisk` instead of `ReadBlockFromDisk` to serve these requests, and only deserializes for `verbosity > 0` and `json` REST requests. See benchmarks in https://github.com/bitcoin/bitcoin/pull/26684.
Benchmarked using ApacheBench. Requesting block 750,000 in binary 10k times on a single core (set `-rest=1` in config):
`ab -n 10000 -c 1 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"`
On master, mean time 15ms.
On this branch, mean time 1ms.
For RPC
```
echo '{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 0]}' > /tmp/data.json
ab -p /tmp/data.json -n 1000 -c 1 -A user:password "http://127.0.0.1:8332/"
```
On master, mean time 32ms
On this branch, mean time 13ms
ACKs for top commit:
achow101:
re-ACK e710cefd57
Tree-SHA512: 4cea13c7b563b2139d041b1fdcfdb793c8cc688654ae08db07e7ee6b875c5e582b8185db3ae603abbfb06d2164724f29205774620b48c493726b991999af289e
0a533613fb docs: add release notes for #27114 (brunoerg)
e6b8f19de9 test: add coverage for whitelisting manual connections (brunoerg)
c985eb854c test: add option to speed up tx relay/mempool sync (brunoerg)
66bc6e2d17 Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections (Luke Dashjr)
8e06be347c net_processing: Move extra service flag into InitializeNode (Luke Dashjr)
9133fd69a5 net: Move `NetPermissionFlags::Implicit` verification to `AddWhitelistPermissionFlags` (Luke Dashjr)
2863d7dddb net: store `-whitelist{force}relay` values in `CConnman` (brunoerg)
Pull request description:
Revives #17167. It allows whitelisting manual connections. Fixes#9923
Since there are some PRs/issues around this topic, I'll list some motivations/comments for whitelisting outbound connections from them:
- Speed-up tx relay/mempool sync for testing purposes (my personal motivation for this) - In #26970, theStack pointed out that we whitelist peers to speed up tx relay for fast mempool synchronization, however, since it applies only for inbound connections and considering the topology `node0 <--- node1 <---- node2 <--- ... <-- nodeN`, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
- https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1865155764 - "Before enabling -v2transport by default (which I'd image may happen after https://github.com/bitcoin/bitcoin/pull/24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if https://github.com/bitcoin/bitcoin/pull/27114 makes it in."
- https://github.com/bitcoin/bitcoin/pull/17167#issuecomment-1168606032 - "This would allow us to use https://github.com/bitcoin/bitcoin/pull/25355 when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
- Force-relay/mempool permissions for a node you intentionally connected to.
ACKs for top commit:
achow101:
ACK 0a533613fb
sr-gi:
re-ACK [0a53361](0a533613fb)
pinheadmz:
ACK 0a533613fb
Tree-SHA512: 97a79bb854110da04540897d2619eda409d829016aafdf1825ab5515334b0b42ef82f33cd41587af235b3af6ddcec3f2905ca038b5ab22e4c8a03d34f27aebe1
Note that for speed this commit also removes the proof of work and
signet signature checks before returning the block in getblock.
It is assumed if a block is stored it will be valid.
Note that for speed this commit also removes the proof of work and
signet signature checks before returning the block in getblock.
It is assumed if a block is stored it will be valid.
ReadRawBlockFromDisk assumes a non-null pos that has an nPos >= 8.
This simple check makes the function safer to call in the future,
so callers don't need to worry about causing UB if the pos is null.
1342a31f3a [functional test] sibling eviction (glozow)
5fbab37859 [unit test] sibling not returned from SingleV3Checks if 1p2c or 3gen (glozow)
170306728a [policy] sibling eviction for v3 transactions (glozow)
b5d15f764f [refactor] return pair from SingleV3Checks (glozow)
Pull request description:
When we receive a v3 transaction that would bust a mempool transaction's descendant limit, instead of rejecting the new tx, consider replacing the other descendant if it is much higher feerate (using existing RBF criteria to assess that it's more incentive compatible and to avoid DoS).
Delving post with more background and motivation: https://delvingbitcoin.org/t/sibling-eviction-for-v3-transactions/472
ACKs for top commit:
sdaftuar:
ACK 1342a31f3a
achow101:
ACK 1342a31f3a
instagibbs:
ACK 1342a31f3a
Tree-SHA512: dd957d49e51db78758f566c49bddc579b72478e371275c592d3d5ba097d20de47a6c81952045021b99d82a787f5b799baf16dd0ee0e6de90ba12e21e275352be
115c283516 ci: add print of powershell version to win64 job (Max Edwards)
Pull request description:
Extraction of just printing powershell version from closed PR: https://github.com/bitcoin/bitcoin/pull/29581
See https://github.com/bitcoin/bitcoin/pull/29581#issuecomment-1984212990 for the cause of a CI failure which was a powershell update.
This PR will make it easier to notice in the future that PS has changed.
ACKs for top commit:
hebasto:
ACK 115c283516. We still use PowerShell in some steps of the "Win64 native" CI job.
Tree-SHA512: 4c7ba9df4f0a98491120326f05e877a995f43a387fe9bbd193549b32f5a4488f85f83e472c9277db457110a7deda04f08832fe6e8129aff4b0b7278be23d4e35
acc06bc91f ci, macos: Use `--break-system-packages` with Homebrew's python (Hennadii Stepanov)
ae5f72027f ci: Add workaround for Homebrew's python link error (Hennadii Stepanov)
Pull request description:
Homebrew [promoted](https://github.com/Homebrew/homebrew-core/pull/150390) `python@3.12` to the default `python3`. Now, our "macOS native" CI job is facing the following issues:
1. Installing `qt@5` [requires](https://github.com/bitcoin/bitcoin/actions/runs/8216848118/job/22471875454#step:4:51) re-installing `python@3.12`:
```
==> Fetching dependencies for qt@5: readline, python@3.12 and gettext
```
2. Re-installing `python@3.12` [fails](https://github.com/bitcoin/bitcoin/actions/runs/8216848118/job/22471875454#step:4:127) due to symbolic link conflicts on macOS `x86_64`:
```
==> Pouring python@3.12--3.12.2_1.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
```
3. Homebrew's `python@3.12` is marked as externally managed (according to PEP 668), necessitating different approaches for installing Python packages.
This pull request resolves all the issues mentioned above.
ACKs for top commit:
m3dwards:
reACK acc06bc91f to get the CI passing again.
Tree-SHA512: 82cf72bff328f1e0725342431ac14ad4bae7a758186d97db6c7a558e4b661dcbf3fabe536978e26e709c5f6f7f5c11aac46642634c6685f1291592d8d825ad87
fa39151394 refactor: Remove unused error() (MarcoFalke)
fad0335517 scripted-diff: Replace error() with LogError() (MarcoFalke)
fa808fb749 refactor: Make error() return type void (MarcoFalke)
fa1d624348 scripted-diff: return error(...); ==> error(...); return false; (MarcoFalke)
fa9a5e80ab refactor: Add missing {} around error() calls (MarcoFalke)
Pull request description:
`error(...)` has many issues:
* It is often used in the context of `return error(...)`, implying that it has a "fancy" type, creating confusion with `util::Result/Error`
* `-logsourcelocations` does not work with it, because it will pretend the error happened inside of `logging.h`
* The log line contains `ERROR: `, as opposed to `[error]`, like for other errors logged with `LogError`.
Fix all issues by removing it.
ACKs for top commit:
fjahr:
re-utACK fa39151394
stickies-v:
re-ACK fa39151394, no changes since 4a903741b0
ryanofsky:
Code review ACK fa39151394. Just rebase since last review
Tree-SHA512: ec5bb502ab0d3733fdb14a8a00762638fce0417afd8dd6294ae0d485ce2b7ca5b1efeb50fc2cd7467f6c652e4ed3e99b0f283b08aeca04bbfb7ea4f2c95d283a
8aff3fd292 depends: don't use -h with touch on OpenBSD (fanquake)
Pull request description:
Should fix#29447.
ACKs for top commit:
theStack:
Tested ACK 8aff3fd292
hebasto:
ACK 8aff3fd292, tested on OpenBSD 7.1 by running the following commands twice and observing the same output:
Tree-SHA512: c054f80d347600617b21d5a7051315d43ebf858088a28f9b4bd43515f16f957d8033857a194f50556a6f0c67a8afbc2a50e143a477fbb4ef2d36e6365976b82f
2cc8ca19f4 [test] Use deterministic addrman in addrman info tests (stratospher)
a897866109 [test] Restart a node with empty addrman (stratospher)
71c19915c0 [test] Use deterministic addrman in addpeeraddress test (stratospher)
7b868e6b67 Revert "test: avoid non-determinism in asmap-addrman test" (stratospher)
69e091f3e1 [init] Create deterministic addrman in tests using -test=addrman (stratospher)
be25ac3092 [init] Remove -addrmantest command line arg (stratospher)
802e6e128b [init] Add new command line arg for use only in functional tests (stratospher)
Pull request description:
An address is placed in a `[bucket,position]` in the addrman table (new table or tried table) using the `addpeeraddress` RPC. This `[bucket,position]` is calculated using `nKey`(and other metrics) for the addrman which is chosen randomly during every run.
Supposing there are 2 addresses to be placed in an addrman table. During every test run, a different `[bucket,position]` would be calculated for each address.These calculated `[bucket,position]` could even be the same for the 2 addresses in some test runs and result in collisions in the addrman. We wouldn't be able to predict when the collisions are going to happen because we can't predict the `nKey` value which is chosen at random. This can cause flaky tests.
Because of these non deterministic collisions, we are limited in what we can do to test addrman functionality. Currently in our tests don't add a second address to prevent these collisions from happening - we only keep 1 address in the new table and 1 address in the tried table. See https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1091145647, https://github.com/bitcoin/bitcoin/pull/23084, [#22831(comment)](https://github.com/bitcoin/bitcoin/pull/22831/files#r708302639).
This PR lets us create a deterministic addrman with fixed `nKey` so that we can know the `[bucket,position]` collisions beforehand, safely add more addresses in an addrman table and write more extensive tests.
ACKs for top commit:
amitiuttarwar:
ACK 2cc8ca19f4
achow101:
ACK 2cc8ca19f4
0xB10C:
ACK 2cc8ca19f4
mzumsande:
Code Review ACK 2cc8ca19f4
Tree-SHA512: 8acd9bdfe7de1eb44d22373bf13533d8ecf602df966fdd5b8b78afcd8cc35a286c95d2712f67a89473a0d68dded7d38f5599f6e4bf95a6589475444545bfb189
ecc036c5d6 ci: add --v2transport to an existing CI job (Martin Zumsande)
3a25a575f0 test: ignore --v2transport for older versions instead of asserting (Martin Zumsande)
547aacff08 test: add -v1transport option and use it in test_runner (Martin Zumsande)
Pull request description:
This suggests a strategy to run the functional tests with both v1 and v2 transport in the CI.
**Status Quo:**
There is both the global `--v2transport` option for the `test_runner.py` (not enabled by default), plus the possibility to specify `--v2transport` for particular tests, which is used for a handful of tests. Currently, when running `test_runner.py --v2transport`, these tests are run twice with the same `--v2transport` configuration, as has been noted in https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1485626063, which is wasteful.
**Suggested Change:**
Fix this by adding a `--v1transport` option and using it in `test_runner.py`, so that irrespective of the global `--v2transport` flag, the tests that run twice use v1 in one run and v2 in the other.
Also add `--v2transport` to one CI task (`multiprocess, i686, DEBUG`).
This means, that for each CI task, the majority of functional tests will run once using the global `--v2transport` option if provided, while a few selected tests will always run two times, once with `v1` and once with `v2`.
**Rationale:**
A simpler alternative would have been to remove all test-specific `--v2transport` commands from `test_runner.py` and just enable `--v2transport` option for a few CI tasks. I didn't do that because it would have meant that v2 would never be running in the CI for some platforms, and also be run a lot less locally by users and devs (who would have to actively enable the `--v2transport` option).
ACKs for top commit:
tdb3:
ACK for ecc036c5d6.
achow101:
ACK ecc036c5d6
stratospher:
ACK ecc036c.
vasild:
ACK ecc036c5d6
Tree-SHA512: 375b2293d49991f2fbd8e1bb646c0034004a09cee36063bc32996b721323eb19a43d7b2f36b3f9a3fdca846d74f48d8f1387565c03ef5d34b3481d2a0fe1d328
a951dba3a9 wallet: default wallet migration, modify inconvenient backup filename (furszy)
Pull request description:
Fixes#29584
On default legacy wallets, the backup filename starts with an "-" due to the wallet name being empty. This is inconvenient for systems who treat what follows the initial "-" character as flags.
Note:
As the user can freely set the wallet name to anything, we could also guard the backup filename against other inconvenient characters in the future (we need to be careful here, because the wallet name could also be a path).
ACKs for top commit:
achow101:
ACK a951dba3a9
brunoerg:
utACK a951dba3a9
vasild:
ACK a951dba3a9
Tree-SHA512: 6347bb12cfb50526a4baad96e4f1df9d82b493f79f0a0f7e0a1c8335a86a1e8e147c7b7f95cec6ede6f4507506a7eaf7972bd35131a2d5ed4cbbf38d94f0a9ca
This fixes the log output when -logsourcelocations is used.
Also, instead of 'ERROR:', the log will now say '[error]', like other
errors logged with LogError.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's! error\("([^"]+)"! LogError("\1\\n"!g' $( git grep -l ' error(' ./src/ )
-END VERIFY SCRIPT-
This is needed for the next commit.
-BEGIN VERIFY SCRIPT-
# Separate sed invocations to replace one-line, and two-line error(...) calls
sed -i --regexp-extended 's!( +)return (error\(.*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
sed -i --null-data --regexp-extended 's!( +)return (error\([^\n]*\n[^\n]*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
-END VERIFY SCRIPT-
c5b5843d8f test: avoid requesting blocks beyond limited peer threshold (furszy)
2f6a05512f p2p: sync from limited peer, only request blocks below threshold (furszy)
73127722a2 refactor: Make FindNextBlocks friendlier (furszy)
Pull request description:
Even when the node believes it has IBD completed, need to avoid
requesting historical blocks from network-limited peers.
Otherwise, the limited peer will disconnect right away.
The simplest scenario could be a node that gets synced, drops
connections, and stays inactive for a while. Then, once it re-connects
(IBD stays completed), the node tries to fetch all the missing blocks
from any peer, getting disconnected by the limited ones.
Note:
Can verify the behavior by cherry-picking the test commit alone on
master. It will fail there.
ACKs for top commit:
achow101:
ACK c5b5843d8f
vasild:
ACK c5b5843d8f
mzumsande:
Code Review ACK c5b5843d8f
pinheadmz:
ACK c5b5843d8f
Tree-SHA512: 9e550698bc6e63cc587b2b988a87d0ab555a8fa188c91c3f33287f8201d77c28b373331845356ad86f17bb21c15950b6466bc1cafd0ce8139d70364cb71c2ad2
a3badf75f6 tests: Provide more helpful assert_equal errors (Anthony Towns)
Pull request description:
In the functional tests, we often compare dicts with assert_equal, but the output makes it very hard to tell exactly which entry in the dicts don't match when there are a lot of entries and only minor differences. Change the output to make it clearer.
ACKs for top commit:
achow101:
ACK a3badf75f6
vasild:
ACK a3badf75f6
brunoerg:
utACK a3badf75f6
josibake:
ACK a3badf75f6
BrandonOdiwuor:
Code Review ACK a3badf75f6
Tree-SHA512: 1d4b4a3b2e2e28ab09f10b41b04b52b37f64e0d8a54e2306f37de0c3eb3299a7ad4ba225b9efa67057a75e90d008a17385c810a32c9b212d240be280c2dcf2e5
Deduplicate code that exists in both tx processing and ProcessOrphanTx.
Additionally, this can be reused in a function that handles multiple
MempoolAcceptResults from package submission.
Deduplicate code that exists in both tx processing and ProcessOrphanTx.
Additionally, this can be reused in a function that handles multiple
MempoolAcceptResults from package submission.
d27e2d87b9 test: test_bitcoin: allow -testdatadir=<datadir> (Larry Ruane)
Pull request description:
This backward-compatible change would help with code review, testing, and debugging. When `test_bitcoin` runs, it creates a working or data directory within `/tmp/test_common_Bitcoin\ Core/`, named as a long random (hex) string.
This small patch does three things:
- If the (new) argument `-testdatadir=<datadir>` is given, use `<datadir>/test_temp/<test-name>/datadir` as the working directory
- When the test starts, remove `<datadir>/test_temp/<test-name>/datadir` if it exists from an earlier run (currently, it's presumed not to exist due to the long random string)
- Don't delete the working directory at the end of the test if a custom data directory is being used
Example usage, which will remove, create, use `/somewhere/test_temp/getarg_tests/boolarg`, and leave it afterward:
```
$ test_bitcoin --run_test=getarg_tests/boolarg -- -testdatadir=/somewhere
Running 1 test case...
Test directory (will not be deleted): "/somewhere/test_temp/getarg_tests/boolarg/datadir"
*** No errors detected
$ ls -l /somewhere/test_temp/getarg_tests/boolarg/datadir
total 8
drwxrwxr-x 2 larry larry 4096 Feb 22 10:28 blocks
-rw-rw-r-- 1 larry larry 1273 Feb 22 10:28 debug.log
```
(A relative pathname also works.)
This change affects only `test_bitcoin`; it could also be applied to `test_bitcoin-qt` but that's slightly more involved so I'm skipping that for now.
The rationale for this change is that, when running the test using the debugger, it's often useful to watch `debug.log` as the test runs and inspect some of the other files (I've looked at the generated `blknnnn.dat` files for example). Currently, that requires figuring out where the test's working directory is since it changes on every test run. Tests can be run with `-printtoconsole=1` to show debug logging to the terminal, but it's nice to keep `debug.log` continuously open in an editor, for example.
Even if not using a debugger, it's sometimes helpful to see `debug.log` and other artifacts after the test completes.
Similar functionality is already possible with the functional tests using the `--tmpdir=` and `--nocleanup` arguments.
ACKs for top commit:
davidgumberg:
ACK d27e2d87b9
tdb3:
re-ACK for d27e2d87b9
achow101:
ACK d27e2d87b9
cbergqvist:
ACK d27e2d87b95b7982c05b4c88e463cc9626ab9f0a! (Already did some testing with `fs::remove()` to make sure it was compatible with the `util::Lock/UnlockDirectory` implementation).
marcofleon:
ACK d27e2d87b9. I ran all the tests with my previous open file limit and no errors were detected. Also ran some individual tests with no, relative, and absolute paths and everything looks good.
furszy:
ACK d27e2d8
Tree-SHA512: a8f535f34a48b6699cb440f97f5562ec643f3bfba4ea685768980b871fc8b6e1135f70fc05dbe19aa2c8bacb1ddeaff212d63473605a7422ff76332b3a6b1f68
5b358cdd1a i2p: log connection was refused due to arbitrary port (brunoerg)
Pull request description:
For I2P, we do not try to connect if port is != 0. However, we do not have anything that indicates it or any error when trying to connect with port != 0. This PR adds a log for it. Also, it improves the functional test. With this log we can ensure the reason we won't connect is the port, in the current test, we cannot ensure it.
ACKs for top commit:
jonatack:
ACK 5b358cdd1a
epiccurious:
re-ACK 5b358cdd1a.
achow101:
ACK 5b358cdd1a
kristapsk:
re-ACK 5b358cdd1a
vasild:
ACK 5b358cdd1a
Tree-SHA512: 027245afa771c9295fff0bfd17c251dca4a9f4c739e5773922de3c030a65ef05d96291edcbdeeaa50ba3add61f75f28d8c00be503e03fc33d3491d1956fc549f
d5228efb53 kernel: Remove dependency on CScheduler (TheCharlatan)
06069b3913 scripted-diff: Rename MainSignals to ValidationSignals (TheCharlatan)
0d6d2b650d scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner (TheCharlatan)
4abde2c4e3 [refactor] Make MainSignals RAII styled (TheCharlatan)
84f5c135b8 refactor: De-globalize g_signals (TheCharlatan)
473dd4b97a [refactor] Prepare for g_signals de-globalization (TheCharlatan)
3fba3d5dee [refactor] Make signals optional in mempool and chainman (TheCharlatan)
Pull request description:
By defining a virtual interface class for the scheduler client, users of the kernel can now define their own event consuming infrastructure, without having to spawn threads or rely on the scheduler design.
Removing `CScheduler` also allows removing the thread and exception modules from the kernel library.
To make the `CMainSignals` class easier to use from a kernel library perspective, remove its global instantiation and adopt RAII practices.
Renames `CMainSignals` to `ValidationSignals`, which more accurately describes its purpose and scope.
Also make the `ValidationSignals` in the `ChainstateManager` and CTxMemPool` optional. This could be useful in the future for using or testing these classes without having to instantiate any form of signal handling.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). It improves the kernel API and removes two modules from the kernel library.
ACKs for top commit:
maflcko:
re-ACK d5228efb53🌄
ryanofsky:
Code review ACK d5228efb53. Just comment change since last review.
vasild:
ACK d5228efb53
furszy:
diff ACK d5228ef
Tree-SHA512: e93a5f10eb6182effb84bb981859a7ce750e466efd8171045d8d9e7fe46e4065631d9f6f533c5967c4d34c9bb7d7a67e9f4593bd4c5b30cd7b3bbad7be7b331b
On default legacy wallets, the backup filename starts with an "-" due
to the wallet name being empty. This is inconvenient for systems who
treat what follows the initial "-" character as flags.
8e17f00a5d build, msvc: Cleanup `bitcoin_config.h.in` (Hennadii Stepanov)
Pull request description:
This PR mirrors changes from https://github.com/bitcoin/bitcoin/pull/29263 into the MSVC build system.
ACKs for top commit:
fanquake:
ACK 8e17f00a5d
Tree-SHA512: b8e5cca015ff112c2969a60436524e97007ff2c559b3c12425d0549af694b16248311cc3e7c33f798bc095a679933641496836bb846eee6a2a377956ef53f56e
738a53720e [fuzz] Apply fuzz env (suppressions, etc.) when fetching harness list (dergoegge)
Pull request description:
The fuzz test runner does not add the UBSan suppressions when fetching the harness list. We can observe this in CI as lots of UBSan errors prior to the harnesses actually executing: https://api.cirrus-ci.com/v1/task/5678606140047360/logs/ci.log
```
+ test/fuzz/test_runner.py -j10 -l DEBUG /ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/ --empty_min_time=60
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string_view:578:38: runtime error: unsigned integer overflow: 12 - 23 cannot be represented in type 'size_type' (aka 'unsigned long')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string_view:578:38 in
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string_view:578:33: runtime error: implicit conversion from type 'size_type' (aka 'unsigned long') of value 18446744073709551605 (64-bit, unsigned) to type 'const difference_type' (aka 'const long') changed the value to -11 (64-bit, signed)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/string_view:578:33 in
crypto/sha256.cpp:75:57: runtime error: left shift of 1359893119 by 26 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:75:57 in
crypto/sha256.cpp:75:79: runtime error: left shift of 1359893119 by 21 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:75:79 in
crypto/sha256.cpp:75:101: runtime error: left shift of 1359893119 by 7 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:75:101 in
crypto/sha256.cpp:82:47: runtime error: unsigned integer overflow: 2968370640 + 2483695512 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:82:47 in
crypto/sha256.cpp:74:57: runtime error: left shift of 1779033703 by 30 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:74:57 in
crypto/sha256.cpp:74:79: runtime error: left shift of 1779033703 by 19 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:74:79 in
crypto/sha256.cpp:74:101: runtime error: left shift of 1779033703 by 10 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:74:101 in
crypto/sha256.cpp:83:29: runtime error: unsigned integer overflow: 3458249854 + 980412007 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:83:29 in
crypto/sha256.cpp:82:21: runtime error: unsigned integer overflow: 528734635 + 4228187651 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:82:21 in
crypto/sha256.cpp:84:7: runtime error: unsigned integer overflow: 1013904242 + 3720769133 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:84:7 in
crypto/sha256.cpp:85:12: runtime error: unsigned integer overflow: 3720769133 + 2654153126 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:85:12 in
crypto/sha256.cpp:82:33: runtime error: unsigned integer overflow: 4165002546 + 1259303586 cannot be represented in type 'uint32_t' (aka 'unsigned int')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:82:33 in
crypto/sha256.cpp:125:50: runtime error: unsigned integer overflow: 3835390401 + 1367343104 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior crypto/sha256.cpp:125:50 in
crypto/sha256.cpp:77:58: runtime error: left shift of 1367343104 by 15 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
...
```
To fix this we simply apply the usual fuzz env variables (that apply the suppressions) when fetching the harness list as well.
ACKs for top commit:
ismaelsadeeq:
Tested ACK 738a53720e
fanquake:
ACK 738a53720e
Tree-SHA512: befebaeb4ee5f2eddca67fc6dc69e997c6a250ea54844e5e6e93d1f6a13be49364a3ace31eaa942b02dcf73612af29ec4ace86c9eb7567b92f6f5dc3ea14dc11
Specifying this argument overrides the path location for test_bitcoin;
it becomes <datadir>/test_common_Bitcoin Core/<testname>/datadir. Also,
this directory isn't removed after the test completes. This can make it
easier for developers to study the results of a test (see the state of
the data directory after the test runs), and also (for example) have an
editor open on debug.log to monitor it across multiple test runs instead
of having to re-open a different pathname each time.
Example usage (note the "--" is needed):
test_bitcoin --run_test=getarg_tests/boolarg -- \
-testdatadir=/somewhere/mydatadir
This will create (if necessary) and use the data directory:
/somewhere/mydatadir/test_common_Bitcoin Core/getarg_tests/boolarg/datadir
Co-authored-by: furszy <mfurszy@protonmail.com>
8b7630cb1f build: ignore deprecated-declaration warnings in objc++ macOS code (fanquake)
bd8f0354ba build: Add missed definition for `AM_OBJCXXFLAGS` (Hennadii Stepanov)
Pull request description:
These come from GUI code, and haven't/aren't being fixed, see discussion in https://github.com/bitcoin-core/gui/issues/112. For now, just ignore them entirely. Note that this only applies to ObjCXX code, so will not hide any relevant warnings coming from C or CXX code (and they would be unlikely in any case).
Alternative to https://github.com/bitcoin/bitcoin/pull/29362 (which disables all compiler warnings, for macOS builds in the CI). This PR includes one commit from that PR.
Relevant output:
```bash
qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
^
qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
^
qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
[[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
@interface NSUserNotificationCenter : NSObject {
^
3 warnings generated.
```
ACKs for top commit:
hebasto:
re-ACK 8b7630cb1f.
Tree-SHA512: 2f1fec97d9aa46aa23989d9fb283fc574dff9dc8f44847bb273b8fcf942f56f64c6d93dfcd7af8fbb52bf152e0fe76818118ce416d8cec5de852c32b6697a243
These come from GUI code, and haven't/aren't being fixed, see discussion
in https://github.com/bitcoin-core/gui/issues/112. For now, just ignore
them entirely. Note that this only applies to ObjCXX code, so will not
hide any relevant warnings coming from C or CXX code (and they would be
unlikely in any case).
Alternative to #29362, which disables all compiler warnings, for macOS
builds in the CI.
Relevant output:
```bash
qt/macnotificationhandler.mm:27:9: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
^
qt/macnotificationhandler.mm:27:50: warning: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:24:12: note: 'NSUserNotification' has been explicitly marked deprecated here
@interface NSUserNotification : NSObject <NSCopying> {
^
qt/macnotificationhandler.mm:30:11: warning: 'NSUserNotificationCenter' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Wdeprecated-declarations]
[[NSUserNotificationCenter defaultUserNotificationCenter] deliverNotification: userNotification];
^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSUserNotification.h:118:12: note: 'NSUserNotificationCenter' has been explicitly marked deprecated here
@interface NSUserNotificationCenter : NSObject {
^
3 warnings generated.
```
MarcoFalke