💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469898496)
Besides the remaining CI failure, it looks like the lint job does not support `clang-format-diff`. @MarcoFalke is this the case? I took the formatting line from one of your commits (fac5c373006a9e4bcbb56843bb85f1aca4d87599).
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469898496)
Besides the remaining CI failure, it looks like the lint job does not support `clang-format-diff`. @MarcoFalke is this the case? I took the formatting line from one of your commits (fac5c373006a9e4bcbb56843bb85f1aca4d87599).
💬 willcl-ark commented on issue "RFC: Replacing Boost Process":
(https://github.com/bitcoin/bitcoin/issues/24907#issuecomment-1469901431)
> FWIW, I started working on replacing boost::process with cpp-subprocess on the following branch:
Does cpp-subprocess ok work on OpenBSD?
(https://github.com/bitcoin/bitcoin/issues/24907#issuecomment-1469901431)
> FWIW, I started working on replacing boost::process with cpp-subprocess on the following branch:
Does cpp-subprocess ok work on OpenBSD?
👍 MarcoFalke approved a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
Nice. I left some non-blocking nits. Please advise maintainers if this should be merged and if you plan to address them.
review ACK 600ab02bf58e073a93936438a7e884b3a7110f1 💒
<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}"
RUTRmVTMeKV5npGrKx1
...
(https://github.com/bitcoin/bitcoin/pull/26177)
Nice. I left some non-blocking nits. Please advise maintainers if this should be merged and if you plan to address them.
review ACK 600ab02bf58e073a93936438a7e884b3a7110f1 💒
<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}"
RUTRmVTMeKV5npGrKx1
...
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136932363)
nit in https://github.com/bitcoin/bitcoin/commit/1a691358b7b4d9c8bd6286c2f38481155775f9d5: wrong section (see stdlib include section below)
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136932363)
nit in https://github.com/bitcoin/bitcoin/commit/1a691358b7b4d9c8bd6286c2f38481155775f9d5: wrong section (see stdlib include section below)
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136912251)
nit in https://github.com/bitcoin/bitcoin/commit/f3225c9090eaef8a2f9d711364ca5ff3fd799844: Use `{}` to document the (lack of a) default value, see also https://github.com/bitcoin/bitcoin/pull/26296
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136912251)
nit in https://github.com/bitcoin/bitcoin/commit/f3225c9090eaef8a2f9d711364ca5ff3fd799844: Use `{}` to document the (lack of a) default value, see also https://github.com/bitcoin/bitcoin/pull/26296
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136909353)
unrelated in f3225c9090eaef8a2f9d711364ca5ff3fd799844: Could use `TryParseHex`?
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136909353)
unrelated in f3225c9090eaef8a2f9d711364ca5ff3fd799844: Could use `TryParseHex`?
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136929048)
nit in https://github.com/bitcoin/bitcoin/commit/1a691358b7b4d9c8bd6286c2f38481155775f9d5: string_view already is a pointer, so no need to wrap it into another pointer. Also, clang-format. Also, if the name was kept `name`, more code would be move-only. :sweat_smile:
```cpp
std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(std::string_view name)
{
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136929048)
nit in https://github.com/bitcoin/bitcoin/commit/1a691358b7b4d9c8bd6286c2f38481155775f9d5: string_view already is a pointer, so no need to wrap it into another pointer. Also, clang-format. Also, if the name was kept `name`, more code would be move-only. :sweat_smile:
```cpp
std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(std::string_view name)
{
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136910536)
unrelated: Does `__func__` add any value?
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136910536)
unrelated: Does `__func__` add any value?
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136972083)
? in 6e5221c49ef0e259a3109b51cbc68474f587be58: Seems pretty useless to waste LOCs with the only purpose to add redundancy and manual maintenance burden to repeat the return type verbatim twice?
Seems fine to remove all those docs, unless there is something non-trivial and worthy to mention?
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136972083)
? in 6e5221c49ef0e259a3109b51cbc68474f587be58: Seems pretty useless to waste LOCs with the only purpose to add redundancy and manual maintenance burden to repeat the return type verbatim twice?
Seems fine to remove all those docs, unless there is something non-trivial and worthy to mention?
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136990916)
nit in 1157bcde4b2f05521a0c0d58eb790d4c62572ea7: I think the comment here can be removed? We have iwyu to produce these if anyone wants them, and all other includes in this file don't have them either.
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136990916)
nit in 1157bcde4b2f05521a0c0d58eb790d4c62572ea7: I think the comment here can be removed? We have iwyu to produce these if anyone wants them, and all other includes in this file don't have them either.
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136977126)
nit in 6e5221c49ef0e259a3109b51cbc68474f587be58: Might as well make this `make_unique` as well? ( I guess you wanted to keep this move-only, but the other `make_unique` are no longer move-only anyway, as you removed the `const`.)
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136977126)
nit in 6e5221c49ef0e259a3109b51cbc68474f587be58: Might as well make this `make_unique` as well? ( I guess you wanted to keep this move-only, but the other `make_unique` are no longer move-only anyway, as you removed the `const`.)
💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136991527)
nit in 1157bcde4b2f05521a0c0d58eb790d4c62572ea7: cassert
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1136991527)
nit in 1157bcde4b2f05521a0c0d58eb790d4c62572ea7: cassert
💬 MarcoFalke commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469950032)
I guess it can be added back? No strong opinion. Though, we might want to look into docker image caching to avoid the `apt` overhead.
> `sed -i '11 i #include <cstddef>' src/compat/assumptions.h`
Not sure if this should be hidden like this. A separate commit or pull request seems better for this bugfix. Also, could add it to ci iwyu for reviewers to check for other missing includes?
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469950032)
I guess it can be added back? No strong opinion. Though, we might want to look into docker image caching to avoid the `apt` overhead.
> `sed -i '11 i #include <cstddef>' src/compat/assumptions.h`
Not sure if this should be hidden like this. A separate commit or pull request seems better for this bugfix. Also, could add it to ci iwyu for reviewers to check for other missing includes?
💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#issuecomment-1469955838)
Didn't re-test. Please advise maintainers if you want this merged or work on coverage for `walletpassphrase` a bit more (https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1136502150)
review ACK 78cd77e17719d56366c483d5508cad7b0c5f1b5c 🏟
<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 -
...
(https://github.com/bitcoin/bitcoin/pull/27199#issuecomment-1469955838)
Didn't re-test. Please advise maintainers if you want this merged or work on coverage for `walletpassphrase` a bit more (https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1136502150)
review ACK 78cd77e17719d56366c483d5508cad7b0c5f1b5c 🏟
<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 -
...
💬 MarcoFalke commented on issue "feature_config_args.py failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27259#issuecomment-1469960716)
> (non-timeout) fashion
Why? What happens if you increase the timeout factor?
(https://github.com/bitcoin/bitcoin/issues/27259#issuecomment-1469960716)
> (non-timeout) fashion
Why? What happens if you increase the timeout factor?
💬 theStack commented on issue "RFC: Replacing Boost Process":
(https://github.com/bitcoin/bitcoin/issues/24907#issuecomment-1469975716)
> > FWIW, I started working on replacing boost::process with cpp-subprocess on the following branch:
>
> Does cpp-subprocess ok work on OpenBSD?
Yes, at least the corresponding unit tests and functional tests pass both on Linux (Ubuntu 22.04.1 LTS, Kernel 5.15.0) and OpenBSD (7.2). Would be nice if someone could test the branch (https://github.com/theStack/bitcoin/tree/nuke_boost_process) on MacOS.
(https://github.com/bitcoin/bitcoin/issues/24907#issuecomment-1469975716)
> > FWIW, I started working on replacing boost::process with cpp-subprocess on the following branch:
>
> Does cpp-subprocess ok work on OpenBSD?
Yes, at least the corresponding unit tests and functional tests pass both on Linux (Ubuntu 22.04.1 LTS, Kernel 5.15.0) and OpenBSD (7.2). Would be nice if someone could test the branch (https://github.com/theStack/bitcoin/tree/nuke_boost_process) on MacOS.
💬 desirepl commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043)
It's not the only GUI bug.
1. Start bitcoind -debuglogfile
2. It says it's default datadir is in %AppData%\Bitcoin and "using this data directory if no bitcoin.conf there (-qt.exe readr Registry value for default datadir in that case)
3. Place bitcoin.conf there and add two lines: datadir and blocksdir (out of sections, in global scope)
4. Start bitcoind -debuglogfile again
5. It says it's default datadir and using it the same as in 2; blocksdir is used as in .conf. Now it is ignoring datad
...
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043)
It's not the only GUI bug.
1. Start bitcoind -debuglogfile
2. It says it's default datadir is in %AppData%\Bitcoin and "using this data directory if no bitcoin.conf there (-qt.exe readr Registry value for default datadir in that case)
3. Place bitcoin.conf there and add two lines: datadir and blocksdir (out of sections, in global scope)
4. Start bitcoind -debuglogfile again
5. It says it's default datadir and using it the same as in 2; blocksdir is used as in .conf. Now it is ignoring datad
...
💬 stickies-v commented on issue "Feature request: alert PR author in case of CI failure":
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1470021680)
> Because notifying the author in case of a intermittent CI network error is entirely pointless if they can't re-run the task anyway.
I think having a "CI failed" label is not a bad idea, but I don't agree that notifying everyone is desirable. Afaik everyone can re-run the CI on _their own_ PRs, if they connect their github account to cirrus? At least I'm able to, and I don't think I have any special privileges. For example, on https://cirrus-ci.com/task/5819454748098560 I have this option:
...
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1470021680)
> Because notifying the author in case of a intermittent CI network error is entirely pointless if they can't re-run the task anyway.
I think having a "CI failed" label is not a bad idea, but I don't agree that notifying everyone is desirable. Afaik everyone can re-run the CI on _their own_ PRs, if they connect their github account to cirrus? At least I'm able to, and I don't think I have any special privileges. For example, on https://cirrus-ci.com/task/5819454748098560 I have this option:
...
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1137099484)
Where?
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1137099484)
Where?
📝 petertodd opened a pull request: "Ignore datacarrier limits for dataless OP_RETURN outputs"
(https://github.com/bitcoin/bitcoin/pull/27261)
They don't carry any data, so the -datacarrier/-datacarriersize options should not apply to them. Previously a transaction containing an OP_RETURN without data would be rejected if datacarrier was set to false, or the datacarriersize was set to zero.
(https://github.com/bitcoin/bitcoin/pull/27261)
They don't carry any data, so the -datacarrier/-datacarriersize options should not apply to them. Previously a transaction containing an OP_RETURN without data would be rejected if datacarrier was set to false, or the datacarriersize was set to zero.