💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523797916)
How does this change affect the error catchers, e.g. https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.h#L507?
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523797916)
How does this change affect the error catchers, e.g. https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.h#L507?
💬 alexanderwiederin commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2523835941)
I'd prefer 1. The benefits are:
a) Self-enforcing contract - the implementation maintains the 'same block' semantics regardless of internal changes.
b) Simpler reasoning - equality semantics are clear without needing much further context.
Regarding the pointer based approach: I don't think the performance improvement justifies coupling the API to Core's current internal architecture.
I can work with either approach - thought it was worth discussing the trade-offs.
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2523835941)
I'd prefer 1. The benefits are:
a) Self-enforcing contract - the implementation maintains the 'same block' semantics regardless of internal changes.
b) Simpler reasoning - equality semantics are clear without needing much further context.
Regarding the pointer based approach: I don't think the performance improvement justifies coupling the API to Core's current internal architecture.
I can work with either approach - thought it was worth discussing the trade-offs.
💬 maflcko commented on pull request "Remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3528262336)
missing refactor prefix in title?
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3528262336)
missing refactor prefix in title?
💬 maflcko commented on pull request "Remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3528289276)
review ACK 99d012ec80a4415e1a37218fb4933550276b9a0a 💧
<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 99d012ec80a4
...
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3528289276)
review ACK 99d012ec80a4415e1a37218fb4933550276b9a0a 💧
<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 99d012ec80a4
...
🤔 hebasto reviewed a pull request: "depends: Boost 1.89.0"
(https://github.com/bitcoin/bitcoin/pull/33428#pullrequestreview-3460291898)
Concept ACK on updating to the upcoming 1.90 instead of 1.89. The newer version allows dropping `skip_compiled_targets.patch`: https://github.com/hebasto/bitcoin/commit/b39ff621195238793b2ab3618291e99096fe04d3.
(https://github.com/bitcoin/bitcoin/pull/33428#pullrequestreview-3460291898)
Concept ACK on updating to the upcoming 1.90 instead of 1.89. The newer version allows dropping `skip_compiled_targets.patch`: https://github.com/hebasto/bitcoin/commit/b39ff621195238793b2ab3618291e99096fe04d3.
💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523866680)
> Did you actually call the full kernel function here, including:
Yeah, these pass for me too. And I can't find any documentation that any of this would be UB or lead to errors. Hence my confusion about how you got your error message.
```cpp
fs::path path{fs::absolute(fs::PathFromString({nullptr, 0}))};
fs::create_directories(path);
path = fs::absolute(fs::PathFromString(""));
fs::create_directories(path);
```
> I wonder what the valid meaning is here, of passing an
...
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523866680)
> Did you actually call the full kernel function here, including:
Yeah, these pass for me too. And I can't find any documentation that any of this would be UB or lead to errors. Hence my confusion about how you got your error message.
```cpp
fs::path path{fs::absolute(fs::PathFromString({nullptr, 0}))};
fs::create_directories(path);
path = fs::absolute(fs::PathFromString(""));
fs::create_directories(path);
```
> I wonder what the valid meaning is here, of passing an
...
💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523872088)
> In what situation would data_dir == nullptr and yet data_dir_len does not equal zero?
This is an invalid state and would clearly be a logic/programmer error by the caller, hence the check to catch it and return early instead of continuing.
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523872088)
> In what situation would data_dir == nullptr and yet data_dir_len does not equal zero?
This is an invalid state and would clearly be a logic/programmer error by the caller, hence the check to catch it and return early instead of continuing.
💬 maflcko commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523891180)
> Yeah, these pass for me too. And I can't find any documentation that any of this would be UB or lead to errors.
Ah, I see you are using libc++, which shows a different behavior:
https://godbolt.org/z/97aer9jde
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523891180)
> Yeah, these pass for me too. And I can't find any documentation that any of this would be UB or lead to errors.
Ah, I see you are using libc++, which shows a different behavior:
https://godbolt.org/z/97aer9jde
💬 stickies-v commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2523909255)
> coupling the API to Core's current internal architecture.
`CBlockIndex` is very much a kernel/validation class, not a node class (which I assume is what you mean with Core?). We're also not coupling the API to any internals, the API has no knowledge of the pointer comparison.
> b) Simpler reasoning - equality semantics are clear without needing much further context.
I feel like the current implementation is simple enough and very consistent with how validation works internally, but ye
...
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2523909255)
> coupling the API to Core's current internal architecture.
`CBlockIndex` is very much a kernel/validation class, not a node class (which I assume is what you mean with Core?). We're also not coupling the API to any internals, the API has no knowledge of the pointer comparison.
> b) Simpler reasoning - equality semantics are clear without needing much further context.
I feel like the current implementation is simple enough and very consistent with how validation works internally, but ye
...
🤔 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
...
(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'
...
(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
(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