💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136869429)
Yes, but I am not saying to do it (use `std::array` and `.at()`). It is just an option, in case you have not considered it. Up to you.
Btw, a difference between an `assert()` and `.at()` is that the former will call `abort()` and will inevitably cause the program to exit. OTOH `.at()` will throw `std::out_of_range` which the caller can catch and continue the execution. I don't have a strong opinion which one is more preferable to use here. Maybe either one is ok. Usually on such "programmer e
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136869429)
Yes, but I am not saying to do it (use `std::array` and `.at()`). It is just an option, in case you have not considered it. Up to you.
Btw, a difference between an `assert()` and `.at()` is that the former will call `abort()` and will inevitably cause the program to exit. OTOH `.at()` will throw `std::out_of_range` which the caller can catch and continue the execution. I don't have a strong opinion which one is more preferable to use here. Maybe either one is ok. Usually on such "programmer e
...
💬 fanquake commented on pull request "rpc: enhanced error message for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1136867843)
Not sure why this is being removed
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1136867843)
Not sure why this is being removed
💬 russeree commented on pull request "rpc: enhanced error message for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1136873703)
This was removed because the linter was complaining about the file permissions and that was the suggestion. I never deliberately changed any perms or ran anything as sudo.
This will be resolved
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1136873703)
This was removed because the linter was complaining about the file permissions and that was the suggestion. I never deliberately changed any perms or ran anything as sudo.
This will be resolved
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136880077)
Oh, yes, now it makes sense! I was confused. I should have read the comment like:
"ensure that the new table gets selected even if new_only is false _(because the tried table is empty)_"
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136880077)
Oh, yes, now it makes sense! I was confused. I should have read the comment like:
"ensure that the new table gets selected even if new_only is false _(because the tried table is empty)_"
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136890366)
Yes. Also std methods that take such "indexes" are usually `size_t`, e.g. https://en.cppreference.com/w/cpp/container/array/operator_at
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136890366)
Yes. Also std methods that take such "indexes" are usually `size_t`, e.g. https://en.cppreference.com/w/cpp/container/array/operator_at
💬 russeree commented on pull request "rpc: enhanced error message for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1469815375)
> Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?
Will do. No it was not intentional but was a side effect of moving 3 files over scp to a different host. This has been resolved and commits squashed.
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1469815375)
> Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?
Will do. No it was not intentional but was a side effect of moving 3 files over scp to a different host. This has been resolved and commits squashed.
💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469855782)
Updated dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 -> b22cf8d563a469e29c9942b4fd9f93351d8b9766 ([splitSystemFs_3](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_3) -> [splitSystemFs_4](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_3..splitSystemFs_4)) to fix a broken rename and include an additional header in `compat/assumptions.h`. Up until this point `compat/assumptions.h` relied on being i
...
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469855782)
Updated dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 -> b22cf8d563a469e29c9942b4fd9f93351d8b9766 ([splitSystemFs_3](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_3) -> [splitSystemFs_4](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_3..splitSystemFs_4)) to fix a broken rename and include an additional header in `compat/assumptions.h`. Up until this point `compat/assumptions.h` relied on being i
...
💬 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?