💬 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

💬 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:
...

💬 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!

💬 fanquake commented on pull request "ci: Turn CentOS task into native one":
(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"]

🚀 fanquake merged a pull request: "ci: Turn CentOS task into native one"
(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/
...

📝 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

💬 maflcko commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1920105439)
Fixed both in https://github.com/bitcoin/bitcoin/pull/31678

💬 0xB10C commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2598275880)
Concept ACK

I want to check if this helps https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2558198430 when used with https://github.com/bitcoin/bitcoin/pull/31545 by not having to keep the base images around anymore.

💬 hodlinator commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2598305057)
Excuse me from being [brief](https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2580812651) earlier.

Taking the time do document changes in latest push - `git range-diff master 608b9ff 8148e4d`

- [Inlined comments](https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1909144554) about different `errno`-codes instead of duplicating the identifiers in the comment.
- Made missing cookie file comment [slightly more informative](https://github.com/bitcoin/bitcoin/pull/30660#discus
...

💬 fanquake commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2598328420)
Seems lihe there was some very brief discussion in #31033, and the conclusion ["I guess this should start with planting some metrics"](https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-2394932171), but I'm not sure putting changes into Bitcoin Core is the right first step.

Before changing our API, it'd be good to atleast show some usage that indicates that the changes here are useful. I assume you've already been running this locally, and collecting the data, so it'd be good to kno
...

💬 bigspider commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2598333028)
> @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.

I suppose so, although it feels somewhat circular as bitcoin-core was always the reference for the tests of the Ledger Bitcoin app!

💬 hebasto commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2598357568)
Rebased due to the conflict with the merged #31651.

💬 fanquake commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2598358694)
NACK - based on the reasoning above.

💬 hebasto commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2598364343)
> NACK - based on the reasoning above.

> Not sure. Why does this need another build option? Given that this is just for working around silent CMake warnings in the CI, couldn't this just be fixed by using explicit `*_TEST` options that otherwise fail if Python is missing?

What "explicit `*_TEST`" do you mean?

💬 Sjors commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2598371523)
ACK 4185ad1cb12060337c2d66a9a66e37be1d10f4ce

My cmake knowledge is also limited, but I like that this removes duplication. E.g instead of `base58_encode_decode.json.h` and `data/base58_encode_decode.json` now `test/CMakeLists.txt` only contains the latter.

The build log looks the same, before and after:

```
[ 76%] Generating data/base58_encode_decode.json.h
```

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2598377454)
Rebased due to the conflicts with the merged #31621 and #31651.

💬 Sjors commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#issuecomment-2598382415)
> This caused an incompatibility with blocksdirs from previous versions of Core. Setting to draft whlie I play around with a fix.

Once you figure that out, you can use a previous release to demonstrate that this works. See e.g. `mempool_compatibility.py`.

💬 fanquake commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2598383717)
> What "explicit *_TEST" do you mean?

(Introducing) An option that does anything test related, that also requires Python, but I don't think this solution is better than #31665, because it forces builders to start opting out of things, and doesn't solve #31476 generically, compared to #31665.