🤔 janb84 reviewed a pull request: "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`"
(https://github.com/bitcoin/bitcoin/pull/33824#pullrequestreview-3460307847)
ACK fae83611b8ef358ea7aca7070fd7e82dc06f9755

The PR changes CI Tasks to use the dev-mode by default and disable options that are not applicable. This to enable kernel options from libbitcoinkernel

I like the approach of "enabling everything" and then turn of options explicitly , in this way the CI will build all the options without a lot of maintenance.

I have compared the resulting config with the old configuration , looks OK 2 non blocking questions:

ARM32 diff is only kernel stuff
...

💬 janb84 commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2523905099)
how about lines 208-212? it's windows so not sure if there is a dev-mode preset ?

```sh
- job-type: standard
generate-options: '-DBUILD_GUI=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DBUILD_KERNEL_LIB=ON -DBUILD_UTIL_CHAINSTATE=ON -DWERROR=ON'
job-name: 'Windows native, VS 2022'
- job-type: fuzz
generate-options: '-DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="wallet" -DBUILD_GUI=OFF -DBUILD_FOR_FUZZING=ON -DWERROR=ON'
...

💬 janb84 commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2523871348)
Do we still use this ? could not find something that uses this in the repo

💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523917297)
For reference, this is what a pure virtual `CCoinsView` would look like: [l0rinc/bitcoin@`d4bec8d` (#53)](https://github.com/l0rinc/bitcoin/pull/53/commits/d4bec8ded7a75dac9aead3730e73e92f656d6e18)

A lot cleaner, but needs further work and is outside the scope of this change.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2523928262)
> Is my understanding correct that we want to remove useless jitter that doesn't actually increase code coverage but confuses the fuzzer, such as the internal Random in leveldb's skiplist?

It depends on the specific fuzz engine as far as _exactly_ what happens (e.g. how it prioritizes or drops executed inputs). Speaking generally, if the fuzzer sees increased coverage (due to non-determinisim) it could prioritize a no-coverage-gain input and if it sees decreased coverage it could de-prioritiz
...

💬 maflcko commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523930442)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90299#c3:

> And for the avoidance of doubt, the problem is not that !exists(p) (although that is true) but that an empty path doesn't refer to any file system location. absolute("does not exist") is not an error, but absolute("") is.

> There is no absolute representation of the empty path, it's a category error.

💬 maflcko commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2523939377)
I think those are already using windows-specific presets. In any case, I am not familiar with Windows, so someone else would have to touch this.

💬 maflcko commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2523939409)
I run it externally :)

💬 maflcko commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#issuecomment-3528394646)
> "Valgrind, fuzz'" no diff , OK?

Yeah, all fuzz tasks only build the fuzz stuff. Maybe that could be a preset, but I won't be touching it here, to keep the scope small.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2523964995)
Nope, thanks.

💬 vasild commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#issuecomment-3528417778)
`e44e669378...110548c9ea`: take suggestions

💬 maflcko commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#issuecomment-3528420008)
review ACK 096924d39d644acc826cbffd39bb34038ecee6cd 📓

<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+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 096924d39d64
...

💬 vasild commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2523975283)
> What do you mean with "(without a help from the standard library)"?

I meant without using `shared_ptr`, but surely we do use the standard library for other things. Removed this sentence altogether.

@ajtowns I took that wording, thanks!

💬 vasild commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2523977301)
Changed.

💬 vasild commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2523981252)
Good idea! Added some brief explanation amongst those lines, thanks!

💬 m3dwards commented on pull request "ci: Lint follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33776#issuecomment-3528462663)
Looks good!

ACK fae3618fd6c82dfcea2f296caa16a79182b32059

💬 yancyribbens commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524024171)
This case wouldn't return early though, even though it's an invalid state.

💬 janb84 commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3528532539)
My Guix Build Output (matching)

**Host architecture:** `aarch64`
**Commit:** `96963b8`

```shell
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b81de17c51d03c328763270f14f3 guix-build-96963b888e5a/out
...

💬 darosior commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3528552716)
Sorry to be this guy again, but it seems to me unreasonable to touch such critical consensus code for such marginal benefits. Unless observable benefits can be shown, i lean toward Concept NACK.

💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524119124)
Hmm? It seems to me like we capture that in `if ((data_dir == nullptr && data_dir_len > 0) || ...) {`, after which we return early?