💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607603259)
Changed. I'm assuming you propose the contemporary way to access the RPC.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607603259)
Changed. I'm assuming you propose the contemporary way to access the RPC.
📝 Raimo33 opened a pull request: "streams: replace `std::find` with `memchr` (5x improvement)"
(https://github.com/bitcoin/bitcoin/pull/34044)
## Summary
This PR optimizes the `FindByte` method by using `memchr` instead of `std::find`. This takes advantage of the underlying optimizations that come with `memchr`, primarily vectorized chunked reads. While `std::find` is more standard and modern, it is suboptimal for iterating single bytes as they're iterated 1 by 1 instead of exploiting SIMD.
One could argue that this is not a concern of Bitcoin Core but rather of libc++ mantainers, but since it shows 5x improvement in existing ben
...
(https://github.com/bitcoin/bitcoin/pull/34044)
## Summary
This PR optimizes the `FindByte` method by using `memchr` instead of `std::find`. This takes advantage of the underlying optimizations that come with `memchr`, primarily vectorized chunked reads. While `std::find` is more standard and modern, it is suboptimal for iterating single bytes as they're iterated 1 by 1 instead of exploiting SIMD.
One could argue that this is not a concern of Bitcoin Core but rather of libc++ mantainers, but since it shows 5x improvement in existing ben
...
💬 ANtutov commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#issuecomment-3638217563)
> lgtm ACK [e71c4df](https://github.com/bitcoin/bitcoin/commit/e71c4df1685131f5ab48aac6ccb07ac944e91e9f)
>
> Sorry for causing some confusion earlier.
I'm grateful to you cause i was confused a bit
(https://github.com/bitcoin/bitcoin/pull/33962#issuecomment-3638217563)
> lgtm ACK [e71c4df](https://github.com/bitcoin/bitcoin/commit/e71c4df1685131f5ab48aac6ccb07ac944e91e9f)
>
> Sorry for causing some confusion earlier.
I'm grateful to you cause i was confused a bit
💬 brunoerg commented on pull request "p2p: saturate LocalServiceInfo::nScore to prevent overflow":
(https://github.com/bitcoin/bitcoin/pull/34028#discussion_r2607638920)
I just ran a mutation test for this PR, here's a mutant that wasn't killed and might be worth addressing (writing tests that can detect this change):
```diff
diff --git a/src/net.cpp b/src/net.cpp
index bbe24c8d3e..ab159e0123 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -321,7 +321,7 @@ bool SeenLocal(const CService& addr)
const auto it = mapLocalHost.find(addr);
if (it == mapLocalHost.end()) return false;
- if (it->second.nScore < std::numeric_limits<int>::max()) {
+
...
(https://github.com/bitcoin/bitcoin/pull/34028#discussion_r2607638920)
I just ran a mutation test for this PR, here's a mutant that wasn't killed and might be worth addressing (writing tests that can detect this change):
```diff
diff --git a/src/net.cpp b/src/net.cpp
index bbe24c8d3e..ab159e0123 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -321,7 +321,7 @@ bool SeenLocal(const CService& addr)
const auto it = mapLocalHost.find(addr);
if (it == mapLocalHost.end()) return false;
- if (it->second.nScore < std::numeric_limits<int>::max()) {
+
...
💬 davidgumberg commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2607756460)
Thanks, I've taken this in 15cefcd7d3729b3af59f63b1ba406b5dfb405e99 https://github.com/bitcoin/bitcoin/blob/15cefcd7d3729b3af59f63b1ba406b5dfb405e99/src/test/sock_tests.cpp#L108-L112
(https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2607756460)
Thanks, I've taken this in 15cefcd7d3729b3af59f63b1ba406b5dfb405e99 https://github.com/bitcoin/bitcoin/blob/15cefcd7d3729b3af59f63b1ba406b5dfb405e99/src/test/sock_tests.cpp#L108-L112
💬 Bicaru20 commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2607756886)
Since increasing the 50 in the loop will not contribute anything to the test, maybe the `>> 8` and `% 256` could be erased.
```suggestion
first_octet = 1 + i
second_octet = i
```
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2607756886)
Since increasing the 50 in the loop will not contribute anything to the test, maybe the `>> 8` and `% 256` could be erased.
```suggestion
first_octet = 1 + i
second_octet = i
```
💬 Bicaru20 commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#issuecomment-3638411625)
Code review ACK fb9c393564ef098267ad2a921b75e0716f38b047. I have run the test and everything checks out.
I left a small comment in the code, also fine with merging as-is
(https://github.com/bitcoin/bitcoin/pull/34039#issuecomment-3638411625)
Code review ACK fb9c393564ef098267ad2a921b75e0716f38b047. I have run the test and everything checks out.
I left a small comment in the code, also fine with merging as-is
💬 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_r2607768660)
> because we could have duplicates in pruned_blocks by pruning twice
Ah, nice catch.
> So I also added a check to the previous commit that we don't attempt to prune an already pruned block - maybe that was also the reason for the non-determinism?
The non-determinism is gone now, it was either because I had a small local patch to change `m_block_index` to `std::map` that I forgot about or because of the lack of this new check.
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2607768660)
> because we could have duplicates in pruned_blocks by pruning twice
Ah, nice catch.
> So I also added a check to the previous commit that we don't attempt to prune an already pruned block - maybe that was also the reason for the non-determinism?
The non-determinism is gone now, it was either because I had a small local patch to change `m_block_index` to `std::map` that I forgot about or because of the lack of this new check.
💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3638417186)
> > I expected that someone would squash upon integrating the work.
>
> Maintainers don't touch the original commits, which is especially nice if you GPG sign them.
That is a fair point. I could sign, but it feels like unnecessarily blowing up the size of a minor contribution.
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3638417186)
> > I expected that someone would squash upon integrating the work.
>
> Maintainers don't touch the original commits, which is especially nice if you GPG sign them.
That is a fair point. I could sign, but it feels like unnecessarily blowing up the size of a minor contribution.
💬 Ataraxia009 commented on pull request "streams: replace `std::find` with `memchr` (5x improvement)":
(https://github.com/bitcoin/bitcoin/pull/34044#issuecomment-3638423961)
Concept ACK
'memchr' seems like a better alternative here
(https://github.com/bitcoin/bitcoin/pull/34044#issuecomment-3638423961)
Concept ACK
'memchr' seems like a better alternative here
👍 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?