👍 ryanofsky approved a pull request: "util: Add some more Unexpected and Expected methods"
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3563848843)
Code review ACK fa0a1a6a4986c1a977bbcd5f48834bedeac5dffb. Left some suggestions which I think would make this better in significant ways, but I do this is already an improvement in its current form.
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3563848843)
Code review ACK fa0a1a6a4986c1a977bbcd5f48834bedeac5dffb. Left some suggestions which I think would make this better in significant ways, but I do this is already an improvement in its current form.
💬 ryanofsky commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607689549)
In commit "util: Make Expected::value() throw" (fa0297540e29f077a76133d1b60d3fd35f03b4e1)
In the standard `std::expected` class there is a clear difference between the high-level `value()` method which throws an exception when the value is unset, and lower level pointer operators which are `noexcept` and can't be called without a value. This makes `std::expected` consistent with `std::optional`, which also has `noexcept` pointer operators and a throwing `value()` method. It's also similar t
...
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607689549)
In commit "util: Make Expected::value() throw" (fa0297540e29f077a76133d1b60d3fd35f03b4e1)
In the standard `std::expected` class there is a clear difference between the high-level `value()` method which throws an exception when the value is unset, and lower level pointer operators which are `noexcept` and can't be called without a value. This makes `std::expected` consistent with `std::optional`, which also has `noexcept` pointer operators and a throwing `value()` method. It's also similar t
...
💬 ryanofsky commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607767971)
In commit "util: Add Expected<void, E> specialization" (fa3d6c760db3dafbaa530401893a40fc5f6575f9)
This specialization is more complicated than it needs to be. Would suggest simplifying with inheritance:
```c++
//! Specialization for void returning void from value methods.
template <class E>
class Expected<void, E> : public Expected<std::monostate, E>
{
using Base = Expected<std::monostate, E>;
public:
using Expected<std::monostate, E>::Expected;
constexpr void operator*
...
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607767971)
In commit "util: Add Expected<void, E> specialization" (fa3d6c760db3dafbaa530401893a40fc5f6575f9)
This specialization is more complicated than it needs to be. Would suggest simplifying with inheritance:
```c++
//! Specialization for void returning void from value methods.
template <class E>
class Expected<void, E> : public Expected<std::monostate, E>
{
using Base = Expected<std::monostate, E>;
public:
using Expected<std::monostate, E>::Expected;
constexpr void operator*
...
💬 ryanofsky commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607703403)
In commit "util: Add Unexpected::error()" (faa0d23ee75aac289c3766ce6ee27948b0d982d9)
Calling this `m_error` would seem more consistent.
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2607703403)
In commit "util: Add Unexpected::error()" (faa0d23ee75aac289c3766ce6ee27948b0d982d9)
Calling this `m_error` would seem more consistent.
🤔 Crypt-iQ reviewed a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3563976228)
crACK ac67ab76470441ed7539eb780efa3912f1281faa
Will continue to fuzz and see if anything pops up.
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3563976228)
crACK ac67ab76470441ed7539eb780efa3912f1281faa
Will continue to fuzz and see if anything pops up.
💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2607792781)
nit: (if you push up again) this could be named `pindexNew` to match the header?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2607792781)
nit: (if you push up again) this could be named `pindexNew` to match the header?
💬 davidgumberg commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3638472464)
[[9577508..15cefcd](https://github.com/bitcoin/bitcoin/compare/95775080da6a2c64a023623a060cd89760d6b762..15cefcd7d3729b3af59f63b1ba406b5dfb405e99)] Pushed to address @laanwj's [suggestion.](https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2455303840).
> Should we keep using socketpair on platforms that support it?
(One reason to do this is that TCP ports might be heavily contended resource on CI systems, though it should be okay)
My gut reaction is that it would not be worth the
...
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3638472464)
[[9577508..15cefcd](https://github.com/bitcoin/bitcoin/compare/95775080da6a2c64a023623a060cd89760d6b762..15cefcd7d3729b3af59f63b1ba406b5dfb405e99)] Pushed to address @laanwj's [suggestion.](https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2455303840).
> Should we keep using socketpair on platforms that support it?
(One reason to do this is that TCP ports might be heavily contended resource on CI systems, though it should be okay)
My gut reaction is that it would not be worth the
...
💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#discussion_r2607868168)
re: https://github.com/bitcoin/bitcoin/pull/33847#discussion_r2607414440
> my earlier example about chainman owning context: it is [passed](https://github.com/bitcoin/bitcoin/blob/56ce78d5f62c9eb9353d33eedd15495c1205465f/src/kernel/bitcoinkernel.h#L967) as a non-owning (`const` ptr) view, and lifetime then ensured via [`std::shared_ptr`](https://github.com/bitcoin/bitcoin/blob/56ce78d5f62c9eb9353d33eedd15495c1205465f/src/kernel/bitcoinkernel.cpp#L472).
That's pretty interesting. I can see
...
(https://github.com/bitcoin/bitcoin/pull/33847#discussion_r2607868168)
re: https://github.com/bitcoin/bitcoin/pull/33847#discussion_r2607414440
> my earlier example about chainman owning context: it is [passed](https://github.com/bitcoin/bitcoin/blob/56ce78d5f62c9eb9353d33eedd15495c1205465f/src/kernel/bitcoinkernel.h#L967) as a non-owning (`const` ptr) view, and lifetime then ensured via [`std::shared_ptr`](https://github.com/bitcoin/bitcoin/blob/56ce78d5f62c9eb9353d33eedd15495c1205465f/src/kernel/bitcoinkernel.cpp#L472).
That's pretty interesting. I can see
...
💬 davidgumberg commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2607898217)
Sure, you can reproduce this issue as follows with `podman`:
### Ubuntu container
```Bash
# The reason a Dockerfile is needed is to get systemd installed and running in
# the container.
mkdir -p /tmp/ubuntu-systemd-container && cd /tmp/ubuntu-systemd-container
cat > ./Dockerfile << 'EOF'
FROM ubuntu:24.04
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update
RUN apt-get install -y curl git gpg systemd make uidmap wget xz-utils
CMD ["/lib/systemd/systemd"]
EOF
# I haven't con
...
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2607898217)
Sure, you can reproduce this issue as follows with `podman`:
### Ubuntu container
```Bash
# The reason a Dockerfile is needed is to get systemd installed and running in
# the container.
mkdir -p /tmp/ubuntu-systemd-container && cd /tmp/ubuntu-systemd-container
cat > ./Dockerfile << 'EOF'
FROM ubuntu:24.04
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update
RUN apt-get install -y curl git gpg systemd make uidmap wget xz-utils
CMD ["/lib/systemd/systemd"]
EOF
# I haven't con
...
💬 stringintech commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2607898828)
Makes sense, thanks! It seems I can cherry-pick your commit as is to replace bfceb4c?
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2607898828)
Makes sense, thanks! It seems I can cherry-pick your commit as is to replace bfceb4c?
📝 achow101 opened a pull request: "test: Log IP of download server in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/34045)
In order to help debug issues with previous release downloads from our web server, we need to know which IP the downloader connected to.
(https://github.com/bitcoin/bitcoin/pull/34045)
In order to help debug issues with previous release downloads from our web server, we need to know which IP the downloader connected to.
💬 davidgumberg commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2607905906)
Thanks for testing this, I've dropped this commit.
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2607905906)
Thanks for testing this, I've dropped this commit.
💬 davidgumberg commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#issuecomment-3638588033)
Pushed to drop the SELinux note since the issue seems to have been resolved upstream.
(https://github.com/bitcoin/bitcoin/pull/32442#issuecomment-3638588033)
Pushed to drop the SELinux note since the issue seems to have been resolved upstream.
💬 w0xlt commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2607926938)
nit: Can `OpenNetworkConnection` consistently fail even for valid addresses—for example, due to local networking issues or a misconfigured proxy that rejects connections?
Also, is it possible for this thread to loop and consume CPU until `NumToOpen` is satisfied or addrman is exhausted?
```suggestion
LogDebug(BCLog::PRIVBROADCAST, "Failed to connect to %s, will retry to a different address; remaining connections to open: %d", target_str, remaining);
// Preve
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2607926938)
nit: Can `OpenNetworkConnection` consistently fail even for valid addresses—for example, due to local networking issues or a misconfigured proxy that rejects connections?
Also, is it possible for this thread to loop and consume CPU until `NumToOpen` is satisfied or addrman is exhausted?
```suggestion
LogDebug(BCLog::PRIVBROADCAST, "Failed to connect to %s, will retry to a different address; remaining connections to open: %d", target_str, remaining);
// Preve
...
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2607935205)
`ConstPubkeyProvider` can self expand. Self expand in the context of PubkeyProviders means that pubkeys can always be retrieved, and this is always true for `ConstPubkeyProvider`. `ConstPubkeyProvider` cannot be ranged, nor can it be something that requires a private key in order to get the pubkey.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2607935205)
`ConstPubkeyProvider` can self expand. Self expand in the context of PubkeyProviders means that pubkeys can always be retrieved, and this is always true for `ConstPubkeyProvider`. `ConstPubkeyProvider` cannot be ranged, nor can it be something that requires a private key in order to get the pubkey.
💬 hebasto commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2607973542)
The default URL of repository has been updated recently:
```suggestion
Updating channel 'guix' from Git repository at 'https://git.guix.gnu.org/guix.git'...
```
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2607973542)
The default URL of repository has been updated recently:
```suggestion
Updating channel 'guix' from Git repository at 'https://git.guix.gnu.org/guix.git'...
```
💬 janb84 commented on pull request "test: Log IP of download server in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2607979328)
Are the server IP addresses always IPv4 ? `Socket.AF_INET` is ipv4 only
(https://github.com/bitcoin/bitcoin/pull/34045#discussion_r2607979328)
Are the server IP addresses always IPv4 ? `Socket.AF_INET` is ipv4 only
👍 hebasto approved a pull request: "depends: Boost 1.90.0"
(https://github.com/bitcoin/bitcoin/pull/33428#pullrequestreview-3564225158)
ACK ca4a844eed481afa0ca7eab8b972ccbda8dfe168.
My Guix build:
```
x86_64
c88ca4eda058c02cf8cd6d8e0d547ae83e52ec302187ddc1a1302ec1701d757a guix-build-ca4a844eed48/output/aarch64-linux-gnu/SHA256SUMS.part
a6853e0e507970ee3bca72bf00c9afcb979e7e6e28c8463836edf81f8aaa1958 guix-build-ca4a844eed48/output/aarch64-linux-gnu/bitcoin-ca4a844eed48-aarch64-linux-gnu-debug.tar.gz
1bf0497f2a88ace485f71c224cb0ace6e0d0ee3109057f8b0fb9813a9d8f2ddf guix-build-ca4a844eed48/output/aarch64-linux-gnu/bitcoin-ca4a84
...
(https://github.com/bitcoin/bitcoin/pull/33428#pullrequestreview-3564225158)
ACK ca4a844eed481afa0ca7eab8b972ccbda8dfe168.
My Guix build:
```
x86_64
c88ca4eda058c02cf8cd6d8e0d547ae83e52ec302187ddc1a1302ec1701d757a guix-build-ca4a844eed48/output/aarch64-linux-gnu/SHA256SUMS.part
a6853e0e507970ee3bca72bf00c9afcb979e7e6e28c8463836edf81f8aaa1958 guix-build-ca4a844eed48/output/aarch64-linux-gnu/bitcoin-ca4a844eed48-aarch64-linux-gnu-debug.tar.gz
1bf0497f2a88ace485f71c224cb0ace6e0d0ee3109057f8b0fb9813a9d8f2ddf guix-build-ca4a844eed48/output/aarch64-linux-gnu/bitcoin-ca4a84
...
👍 hodlinator approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3563299997)
re-ACK 8b417087aec4671a1ce58f2331d1688e665d9935
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3563299997)
re-ACK 8b417087aec4671a1ce58f2331d1688e665d9935
💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607934791)
Having a `default` prevents the compiler warning for missing `enum` values as it is assumed the developer meant for all missing `enum` values to go to the `default`.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2607934791)
Having a `default` prevents the compiler warning for missing `enum` values as it is assumed the developer meant for all missing `enum` values to go to the `default`.