💬 hebasto commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#discussion_r1919777981)
style nit: 2 space indentation.
(https://github.com/bitcoin/bitcoin/pull/31665#discussion_r1919777981)
style nit: 2 space indentation.
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1919786698)
I'll add a check for `freebsd`.
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1919786698)
I'll add a check for `freebsd`.
💬 hebasto commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1919791861)
7ab858765d90470f941851d8cf55d616014ee2df:
Why are native packages being removed?
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1919791861)
7ab858765d90470f941851d8cf55d616014ee2df:
Why are native packages being removed?
💬 hebasto commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#issuecomment-2597778189)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31674#issuecomment-2597778189)
Concept ACK.
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1919798868)
Ah that's probably why CI broke.
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1919798868)
Ah that's probably why CI broke.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2597803878)
Updated 4dde75858a3b08f84d71176c7be14bae62020b1f -> 3eefbda3710aad3adc4aac3a6a36e381f9e9f555 ([kernelApi_16](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_16) -> [kernelApi_17](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_17), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_16..kernelApi_17))
* Get new method for computing cache sizes from https://github.com/bitcoin/bitcoin/pull/31483
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2597803878)
Updated 4dde75858a3b08f84d71176c7be14bae62020b1f -> 3eefbda3710aad3adc4aac3a6a36e381f9e9f555 ([kernelApi_16](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_16) -> [kernelApi_17](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_17), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_16..kernelApi_17))
* Get new method for computing cache sizes from https://github.com/bitcoin/bitcoin/pull/31483
💬 fanquake commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2597811603)
> 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. I'm assuming any user who want to use this locally is exactly the kind of user who'd rather see more warnings than less?
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2597811603)
> 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. I'm assuming any user who want to use this locally is exactly the kind of user who'd rather see more warnings than less?
🚀 fanquake merged a pull request: "[test] fix p2p_orphan_handling.py empty orphanage check"
(https://github.com/bitcoin/bitcoin/pull/31675)
(https://github.com/bitcoin/bitcoin/pull/31675)
💬 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.