💬 fanquake commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2597826643)
https://github.com/bitcoin/bitcoin/actions/runs/12826202676/job/35765674846?pr=30975#step:7:3078:
```bash
node0 2025-01-17T09:33:34.664692Z [capnp-loop] [ipc/capnp/protocol.cpp:35] [IpcLogFn] [ipc] {bitcoin-node-25673/b-capnp-loop-71206} EventLoop::loop done, cancelling event listeners.
node0 2025-01-17T09:33:34.664706Z [capnp-loop] [ipc/capnp/protocol.cpp:35] [IpcLogFn] [ipc] {bitcoin-node-25673/b-capnp-loop-71206} EventLoop::loop bye.
test 2025-01-17T09:33:34.712000Z TestFramework (E
...
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2597826643)
https://github.com/bitcoin/bitcoin/actions/runs/12826202676/job/35765674846?pr=30975#step:7:3078:
```bash
node0 2025-01-17T09:33:34.664692Z [capnp-loop] [ipc/capnp/protocol.cpp:35] [IpcLogFn] [ipc] {bitcoin-node-25673/b-capnp-loop-71206} EventLoop::loop done, cancelling event listeners.
node0 2025-01-17T09:33:34.664706Z [capnp-loop] [ipc/capnp/protocol.cpp:35] [IpcLogFn] [ipc] {bitcoin-node-25673/b-capnp-loop-71206} EventLoop::loop bye.
test 2025-01-17T09:33:34.712000Z TestFramework (E
...
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2597838860)
@fanquake I'm unable to reproduce that locally, will file an issue on the multiprocess repo to track.
Rebased after #31675 and fix the confusion between freebsd and openbsd.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2597838860)
@fanquake I'm unable to reproduce that locally, will file an issue on the multiprocess repo to track.
Rebased after #31675 and fix the confusion between freebsd and openbsd.
💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2597892874)
@bigspider thanks for your [reply](https://groups.google.com/g/bitcoindev/c/OPx9qvmr5Nc/m/8mp8VHjQBQAJ) on the mailinglist. So I guess another way to test this PR is to patch HWI to not do the conversion between PSBT 0 and 2, and then make (any?) transaction with a Ledger that runs at least 2.0.0.
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2597892874)
@bigspider thanks for your [reply](https://groups.google.com/g/bitcoindev/c/OPx9qvmr5Nc/m/8mp8VHjQBQAJ) on the mailinglist. So I guess another way to test this PR is to patch HWI to not do the conversion between PSBT 0 and 2, and then make (any?) transaction with a Ledger that runs at least 2.0.0.
💬 l0rinc commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2597893092)
> I would expect both lines to be essentially overlapping except during flushes.
I was only measuring the memory here *during* flushes. There is no direct height available there, but if we instrument `UpdateTipLog` instead (and fetch some data from the assumeUTXO height), we'd get:
> dbbatchsize=16MiB:
> dbbatchsize=64MiB (+ experimental sorting):
> I would expect both lines to be essentially overlapping except during flushes.
I was only measuring the memory here *during* flushes. There is no direct height available there, but if we instrument `UpdateTipLog` instead (and fetch some data from the assumeUTXO height), we'd get:
> dbbatchsize=16MiB:
> dbbatchsize=64MiB (+ experimental sorting):
![image](https://github.com/user-attachments/
...
🤔 maflcko reviewed a pull request: "init: Lock blocksdir in addition to datadir"
(https://github.com/bitcoin/bitcoin/pull/31674#pullrequestreview-2558458605)
lgtm, but I left a question in the test commit and didn't check it further.
review ACK 2656a5658c14b43c32959db7235e9db55a17d4c8 🏼
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U
...
(https://github.com/bitcoin/bitcoin/pull/31674#pullrequestreview-2558458605)
lgtm, but I left a question in the test commit and didn't check it further.
review ACK 2656a5658c14b43c32959db7235e9db55a17d4c8 🏼
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U
...
💬 maflcko commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919876793)
nit in bdc0a68e676f237bcbb5195a60bb08bb34123e71: Could run clang-format on new code to remove the stray `\`?
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919876793)
nit in bdc0a68e676f237bcbb5195a60bb08bb34123e71: Could run clang-format on new code to remove the stray `\`?
💬 maflcko commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919867104)
style nit: Could use ranges, if you feel fancy :sweat_smile:
```suggestion
bool first_run = std::ranges::all_of(
fs::directory_iterator(opts.blocks_dir),
[](const auto& entry) {
const std::string path = fs::PathToString(entry.path().filename());
return entry.is_regular_file() && path.starts_with('.');
}
);
```
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919867104)
style nit: Could use ranges, if you feel fancy :sweat_smile:
```suggestion
bool first_run = std::ranges::all_of(
fs::directory_iterator(opts.blocks_dir),
[](const auto& entry) {
const std::string path = fs::PathToString(entry.path().filename());
return entry.is_regular_file() && path.starts_with('.');
}
);
```
💬 maflcko commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919883437)
2656a5658c14b43c32959db7235e9db55a17d4c8: Any reason to pass the datadir_path as the blocksdir?
(https://github.com/bitcoin/bitcoin/pull/31674#discussion_r1919883437)
2656a5658c14b43c32959db7235e9db55a17d4c8: Any reason to pass the datadir_path as the blocksdir?
👍 fanquake approved a pull request: "doc: Update dependency installation for Debian/Ubuntu"
(https://github.com/bitcoin/bitcoin/pull/31621#pullrequestreview-2558637286)
ACK 160c27ec078346fbf07f9b84dc10cef2b9809327 - seems correct for modern distro versions, and using pkgconf on older ones also seems to work fine.
(https://github.com/bitcoin/bitcoin/pull/31621#pullrequestreview-2558637286)
ACK 160c27ec078346fbf07f9b84dc10cef2b9809327 - seems correct for modern distro versions, and using pkgconf on older ones also seems to work fine.
🚀 fanquake merged a pull request: "doc: Update dependency installation for Debian/Ubuntu"
(https://github.com/bitcoin/bitcoin/pull/31621)
(https://github.com/bitcoin/bitcoin/pull/31621)
💬 hebasto commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2598133109)
> > I don't like the idea of overloading the WERROR build option with additional functionality, as this is not what users would expect.
>
> Can you elaborate on why this would be unexpected? This is an option to turn warnings into errors, and it'd just be turning more warnings into errors.
Other projects use the same or similar build option only to turn _compiler's_ warnings into errors.
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2598133109)
> > I don't like the idea of overloading the WERROR build option with additional functionality, as this is not what users would expect.
>
> Can you elaborate on why this would be unexpected? This is an option to turn warnings into errors, and it'd just be turning more warnings into errors.
Other projects use the same or similar build option only to turn _compiler's_ warnings into errors.
💬 fanquake commented on pull request "ci: Enable DEBUG=1 for one GCC-12+ build to catch 117966 regressions":
(https://github.com/bitcoin/bitcoin/pull/31522#issuecomment-2598134366)
One thought I have here is that not only are we no-longer running any unit tests for the Win release builds in the CI, but after this change, the build will also no-longer be a release build, so we are moving even further away from testing in the CI, what we are actually producing in Guix. I guess if this is temporary, maybe it's ok, but worth noting, and if it's possible to test this in a different way, without moving the Windows build away from being a release build, it might be worthwhile.
(https://github.com/bitcoin/bitcoin/pull/31522#issuecomment-2598134366)
One thought I have here is that not only are we no-longer running any unit tests for the Win release builds in the CI, but after this change, the build will also no-longer be a release build, so we are moving even further away from testing in the CI, what we are actually producing in Guix. I guess if this is temporary, maybe it's ok, but worth noting, and if it's possible to test this in a different way, without moving the Windows build away from being a release build, it might be worthwhile.
💬 hodlinator commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2598138418)
> (For reference, the commit that drops the unused casts ([f23d6a5](https://github.com/bitcoin/bitcoin/commit/f23d6a572d62ff640a28718ff0771c3f0f87f365)) has already been merged.)
Actual commit: https://github.com/bitcoin/bitcoin/commit/fad4032b219e58300f8d0a74bbcf38ef26fa89d0
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2598138418)
> (For reference, the commit that drops the unused casts ([f23d6a5](https://github.com/bitcoin/bitcoin/commit/f23d6a572d62ff640a28718ff0771c3f0f87f365)) has already been merged.)
Actual commit: https://github.com/bitcoin/bitcoin/commit/fad4032b219e58300f8d0a74bbcf38ef26fa89d0
💬 fanquake commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#issuecomment-2598148421)
https://cirrus-ci.com/task/6569144394448896?logs=ci#L3546:
```bash
[17:40:31.020] #29185 NEW cov: 1486 ft: 2800 corp: 248/5890b lim: 63 exec/s: 2653 rss: 125Mb L: 63/63 MS: 3 ShuffleBytes-InsertRepeatedBytes-InsertRepeatedBytes-
[17:40:31.020] /ci_container_base/src/common/pcp.cpp:331:13: runtime error: implicit conversion from type 'uint16_t' (aka 'unsigned short') of value 65535 (16-bit, unsigned) to type 'uint8_t' (aka 'unsigned char') changed the value to 255 (8-bit, unsigned)
[17:40:
...
(https://github.com/bitcoin/bitcoin/pull/31676#issuecomment-2598148421)
https://cirrus-ci.com/task/6569144394448896?logs=ci#L3546:
```bash
[17:40:31.020] #29185 NEW cov: 1486 ft: 2800 corp: 248/5890b lim: 63 exec/s: 2653 rss: 125Mb L: 63/63 MS: 3 ShuffleBytes-InsertRepeatedBytes-InsertRepeatedBytes-
[17:40:31.020] /ci_container_base/src/common/pcp.cpp:331:13: runtime error: implicit conversion from type 'uint16_t' (aka 'unsigned short') of value 65535 (16-bit, unsigned) to type 'uint8_t' (aka 'unsigned char') changed the value to 255 (8-bit, unsigned)
[17:40:
...
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1920059052)
Ah, so
`widen(i) << shift` => `0` for the left side even before `clamp()`, and
`SaturatingLeftShift(i, shift)` => `std::numeric_limits<uint16_t>::max()` for the right side.
Thanks!
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1920059052)
Ah, so
`widen(i) << shift` => `0` for the left side even before `clamp()`, and
`SaturatingLeftShift(i, shift)` => `std::numeric_limits<uint16_t>::max()` for the right side.
Thanks!
💬 fanquake commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2598176375)
Tested on aarch64.
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2598176375)
Tested on aarch64.
💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2598177746)
> Could create a new issue? Maybe it hit an automated filter?
I've just created a new issue. The "View full details" link I received via email shows:
> We were unable to get this feedback item. It could be because you don't have access to it or it doesn't exist
>
> Error details: ["Request failed with status code 404"]
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2598177746)
> Could create a new issue? Maybe it hit an automated filter?
I've just created a new issue. The "View full details" link I received via email shows:
> We were unable to get this feedback item. It could be because you don't have access to it or it doesn't exist
>
> Error details: ["Request failed with status code 404"]
🚀 fanquake merged a pull request: "ci: Turn CentOS task into native one"
(https://github.com/bitcoin/bitcoin/pull/31651)
(https://github.com/bitcoin/bitcoin/pull/31651)
💬 hebasto commented on issue "multiprocess: `ipc_tests` fail on NetBSD":
(https://github.com/bitcoin/bitcoin/issues/31618#issuecomment-2598215552)
Another failure [happened](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/12821765747/job/35753584191) on FreeBSD:
```
...
test/ipc_tests.cpp(11): Entering test suite "ipc_tests"
test/ipc_tests.cpp(12): Entering test case "ipc_tests"
2025-01-17T03:21:27.796591Z [unknown] [test/util/random.cpp:46] [SeedRandomStateForTest] Setting random seed for current tests to RANDOM_CTX_SEED=0140cbe5acaa3785e6c2a1897e6b52be00debbec3b124b182a626a06ad224950
2025-01-17T03:21:27.797051Z [test] [init/
...
(https://github.com/bitcoin/bitcoin/issues/31618#issuecomment-2598215552)
Another failure [happened](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/12821765747/job/35753584191) on FreeBSD:
```
...
test/ipc_tests.cpp(11): Entering test suite "ipc_tests"
test/ipc_tests.cpp(12): Entering test case "ipc_tests"
2025-01-17T03:21:27.796591Z [unknown] [test/util/random.cpp:46] [SeedRandomStateForTest] Setting random seed for current tests to RANDOM_CTX_SEED=0140cbe5acaa3785e6c2a1897e6b52be00debbec3b124b182a626a06ad224950
2025-01-17T03:21:27.797051Z [test] [init/
...
📝 maflcko opened a pull request: "ci: Skip read-write of default env vars"
(https://github.com/bitcoin/bitcoin/pull/31678)
If they remain unset, they use the default anyway. Except for `USER`, but this seems unused anyway.
Can be checked via:
```
sh-5.2# touch /tmp/empty_env
sh-5.2# podman run --rm --env-file /tmp/empty_env 'ubuntu:24.04' env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
container=podman
HOME=/root
HOSTNAME=19ece5c9e052
(https://github.com/bitcoin/bitcoin/pull/31678)
If they remain unset, they use the default anyway. Except for `USER`, but this seems unused anyway.
Can be checked via:
```
sh-5.2# touch /tmp/empty_env
sh-5.2# podman run --rm --env-file /tmp/empty_env 'ubuntu:24.04' env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
container=podman
HOME=/root
HOSTNAME=19ece5c9e052