💬 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
(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.
(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
...
(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.
(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.
(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 :)
(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.
(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.
(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
(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
...
(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!
(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.
(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!
(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
(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.
(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
...
(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.
(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?
(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?
👍 TheCharlatan approved a pull request: "depends: drop Qt patches"
(https://github.com/bitcoin/bitcoin/pull/33860#pullrequestreview-3460632290)
ACK 3e9aca6f1b52169eed386849900a400e8cea431e
Guix build:
```
6eb3c63d243ad8b9e10bed1aa9ec545d4599a10e2d333a69b346c8aaab941fb2 guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/SHA256SUMS.part
b7bf29e4e64a207eca6ec785f2a8944d0051c177636b6f8db78da5836404ae89 guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/bitcoin-3e9aca6f1b52-aarch64-linux-gnu-debug.tar.gz
dea243b0ce4f6a389ad1612d4f27f0bca816f5f11fe77c2ff4ca703a1db607ee guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/bitcoin-3e9aca6f1b5
...
(https://github.com/bitcoin/bitcoin/pull/33860#pullrequestreview-3460632290)
ACK 3e9aca6f1b52169eed386849900a400e8cea431e
Guix build:
```
6eb3c63d243ad8b9e10bed1aa9ec545d4599a10e2d333a69b346c8aaab941fb2 guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/SHA256SUMS.part
b7bf29e4e64a207eca6ec785f2a8944d0051c177636b6f8db78da5836404ae89 guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/bitcoin-3e9aca6f1b52-aarch64-linux-gnu-debug.tar.gz
dea243b0ce4f6a389ad1612d4f27f0bca816f5f11fe77c2ff4ca703a1db607ee guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/bitcoin-3e9aca6f1b5
...
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524170909)
> Do we still need this after https://github.com/bitcoin/bitcoin/pull/33550?
I think it's still needed, I added it to get rid of linter warnings if `std::filesystem::copy` is instead called directly from the test code.
> Are we adding dead code in this commit? It's hard to judge if a solution is accurate if we see it before the problem is presented - can we make the question obvious before we provide the answer?
Fair point, I'll rework the commits so it's a bit easier to tell where thin
...
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2524170909)
> Do we still need this after https://github.com/bitcoin/bitcoin/pull/33550?
I think it's still needed, I added it to get rid of linter warnings if `std::filesystem::copy` is instead called directly from the test code.
> Are we adding dead code in this commit? It's hard to judge if a solution is accurate if we see it before the problem is presented - can we make the question obvious before we provide the answer?
Fair point, I'll rework the commits so it's a bit easier to tell where thin
...