💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607517231)
```suggestion
if (!IsDirty()) return;
```

💬 l0rinc commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#discussion_r2607512445)
nit: if you think it makes it more readable
```suggestion
bool IsDirty() const noexcept { return !!m_next; }
```
or
```suggestion
bool IsDirty() const noexcept { return m_next != nullptr; }
```

💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607533987)
Not so. Not all commands are expected to interact via stdin. `signtx` does interact via stdin. I removed another sections, but left on the "On success ..". Based on what is it verbose, because I am not otherwise familiar with Bitcoin Core RPC and I find it useful to have these bits of behavior made explicit.

💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607537587)
My involvement is not through HWI. I want to emphasize that presence of `error` in a successful result is not an issue if `"error": null`.

💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607540719)
Dropped this, and cleaning up other parts. This is messy because the documentation describes both the commandline interface and the stdin at the same time.

👍 brunoerg approved a pull request: "fuzz: Add tests for `CCoinControl` methods"
(https://github.com/bitcoin/bitcoin/pull/34026#pullrequestreview-3563662802)
code review ACK 31904f1ba1de49890f605f1d7c907790780831ec

It's fine as is, but I think we could improve this harness a little bit by adding some assertions. Some quite simple examples:
```diff
@@ -87,9 +87,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol)
[&] {
uint32_t sequence{fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
(void)coin_control.Select(out_point).SetSequence(sequence);
- },
- [&] {
-
...

💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607546750)
Dropping.

💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607553947)
Nope. I do not have sufficient knowledge of Bitcoin Core to state this. Do you have an idea? My goal is to emphasize that older versions behaved differently and less refined, such that there is benefit to start using external-signers with recent Bitcoin versions.

Maybe we should just leave it out?

💬 cobratbq commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2607555778)
Copied from other PR, to include their improvements. I'll tweak it.

💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2607580627)
It shouldn't make any difference, for building host tools. It also seems non-trivial to swap out the version in `gcc-toolchain-14`.

💬 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.

📝 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
...

💬 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

💬 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()) {
+
...

💬 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

💬 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
```

💬 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

💬 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.

💬 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.

💬 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