🚀 fanquake merged a pull request: "guix: pass `--enable-initfini-array` to release GCC"
(https://github.com/bitcoin/bitcoin/pull/27153)
(https://github.com/bitcoin/bitcoin/pull/27153)
💬 bigspider commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1136724397)
Perhaps `multi_a` for consistency, since `multi` is a valid fragment elsewhere?
Or `Tapscript multisig` if you want to explicitly _not_ refer to the fragment.
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1136724397)
Perhaps `multi_a` for consistency, since `multi` is a valid fragment elsewhere?
Or `Tapscript multisig` if you want to explicitly _not_ refer to the fragment.
💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1136746362)
> walletpassphrase
If you want to keep coverage for that, you can add it as well (before `walletlock`) with a timeout of `1`, which should override the previous timeout of `999999`?
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1136746362)
> walletpassphrase
If you want to keep coverage for that, you can add it as well (before `walletlock`) with a timeout of `1`, which should override the previous timeout of `999999`?
💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469699591)
Updated ea278094e9937bf96157161941d38a0c08a0aa4e -> c4f2b6d619f6c0faee5aa3118ade1040105fdd8b ([splitSystemFs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_1) -> [splitSystemFs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_1..splitSystemFs_2)) to rename `util/fs.*` to `util/fs_helpers.*`
Added commit dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 ([splitSystemFs_3](https://github.com/The
...
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1469699591)
Updated ea278094e9937bf96157161941d38a0c08a0aa4e -> c4f2b6d619f6c0faee5aa3118ade1040105fdd8b ([splitSystemFs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_1) -> [splitSystemFs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_1..splitSystemFs_2)) to rename `util/fs.*` to `util/fs_helpers.*`
Added commit dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 ([splitSystemFs_3](https://github.com/The
...
📝 russeree opened a pull request: "rpc: enhanced error message for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)
This commit addresses an issue in Bitcoin Core where an incorrect bech32 chain network hrp (prefix) would lead to the address being treated as entirely invalid for both base58 and bech32 decoding, without providing the user with an appropriate error message.
reference
https://github.com/bitcoin/bitcoin/issues/26290
**General Changes**
When a user inputs an incorrect address DecodeDestination now informs the user of the input and expected chain prefixes as well as the network name string.
...
(https://github.com/bitcoin/bitcoin/pull/27260)
This commit addresses an issue in Bitcoin Core where an incorrect bech32 chain network hrp (prefix) would lead to the address being treated as entirely invalid for both base58 and bech32 decoding, without providing the user with an appropriate error message.
reference
https://github.com/bitcoin/bitcoin/issues/26290
**General Changes**
When a user inputs an incorrect address DecodeDestination now informs the user of the input and expected chain prefixes as well as the network name string.
...
💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136853031)
@mzumsande,
> I'd expect performance to go down a bit for the case where we have many addresses to choose from. Do you see this in your benchmark?
Yes, that slows down a bit the fast `Select()` (any network is ok). I observed that and think it is ok because it is still super fast with or without the shuffling.
> Why does this need `ALREADY_VISITED_AND_BORING`?
The `for` loop could still repeat the whole array from the start if some address was found but was skipped. `ALREADY_VISITED_
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1136853031)
@mzumsande,
> I'd expect performance to go down a bit for the case where we have many addresses to choose from. Do you see this in your benchmark?
Yes, that slows down a bit the fast `Select()` (any network is ok). I observed that and think it is ok because it is still super fast with or without the shuffling.
> Why does this need `ALREADY_VISITED_AND_BORING`?
The `for` loop could still repeat the whole array from the start if some address was found but was skipped. `ALREADY_VISITED_
...
💬 fanquake commented on pull request "rpc: enhanced error message for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1469784160)
Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1469784160)
Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?
💬 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`?