🤔 w0xlt reviewed a pull request: "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key"
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3364117332)
ACK https://github.com/bitcoin/bitcoin/pull/32471/commits/2b16014dfbe7e249e4929beafb4ce26415206e7a

💬 Sjors commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-3430904842)
I think it was a cryptic reference to the fact that v27 is end-of-maintenance, but these type of errors still happen, at least until the next time-machine bump.

💬 maflcko commented on pull request "fuzz: don't pass (maybe) nullptr to memcpy()":
(https://github.com/bitcoin/bitcoin/pull/33644#discussion_r2450783853)
Instead of using C functions in a c++ codebase and then working around C quirks temporarily, it seems better to just use the c++ algorithms. The have a few benefits:

* The compiler will optimize them to the same asm, but performance doesn't really matter here anyway
* They work around the uban bug
* they have the additional benefit of being a bit more type safe and document the byte cast explicitly
* The resulting code and logic is shorter

```
std::ranges::copy(sin_addr_bytes, reinterp
...

💬 ryanofsky commented on pull request "Fix windows libc++ `fs::path` `fstream` compile errors":
(https://github.com/bitcoin/bitcoin/pull/33550#discussion_r2450789739)
It was intentional just to minimize verbosity of the code and size of the diff, but it's true this goes in direction of using std::filesystem::path instead of fs::path, and maybe we don't want that.

To me, it seems good to prefer using fs::path instead of std::filesystem::path, but using std::filesystem::path is not always avoidable anyway, and it is at least safe to use as long as not doing conversions to or from arbitrary `char` strings, so it's ok to tolerate in some places

💬 maflcko commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-3430964240)
Just to give another data point: 60000 falls into the ephemeral port range, which normally is avoided by the functional tests, as they start using ports starting from around 11000. However, I can't explain how and if the ephemeral port range itself was related to the issue here.

To check it, you can run:

```
cat /proc/sys/net/ipv4/ip_local_port_range

32768 60999
```


So a more trivial fix could be to change 60000 to 61000, but https://github.com/bitcoin/bitcoin/pull/33670 seems more natural.
...

💬 Sjors commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3430978163)
Two arguments against this change that I find worth pondering a bit about:

@fanquake wrote:

> Would conflict with anyone using a .gitignored `agents.md` locally?

I haven't checked if there's an existing solution for that.

@kanzure wrote:

> One other concern I have is that this is essentially a change that is meant
to be subversive to a developer's tools or development environment.

I could take that argument even further: let's say an anonymous developer uses the LLM to slight
...

🚀 hebasto merged a pull request: "Fix windows libc++ `fs::path` `fstream` compile errors"
(https://github.com/bitcoin/bitcoin/pull/33550)

💬 fanquake commented on pull request "ci: add Valgrind fuzz":
(https://github.com/bitcoin/bitcoin/pull/33461#discussion_r2450847591)
Dropped to `md` for a look.

🤔 mzumsande reviewed a pull request: "addrman, net: filter during address selection via AddrPolicy to avoid underfill"
(https://github.com/bitcoin/bitcoin/pull/33663#pullrequestreview-3360827654)
Concept ACK

The main function of this is p2p (answering GetAddr requests from peers), RPCs like `getnodeaddresses` are of secondary importance.
Since the p2p use is also affected by this, the PR description should be changed to reflect this.

That said, it seems good to always try to return 1000 addresses in a GetAddr answer, so that peers don't know whether we have a long list of banned addresses or not.

💬 mzumsande commented on pull request "addrman, net: filter during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2448349379)
here and below: would be good to expand this over multiple lines (clang-format)

✅ fanquake closed an issue: "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:"
(https://github.com/bitcoin/bitcoin/issues/30030)

🚀 fanquake merged a pull request: "test: Use unassigned p2p_port instead of hardcoded 60000 in p2p_i2p_ports.py"
(https://github.com/bitcoin/bitcoin/pull/33670)

💬 ajtowns commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3431050727)
Concept ACK here too; haven't looked at the code yet

🤔 stickies-v reviewed a pull request: "Add libbitcoinkernel example files"
(https://github.com/bitcoin/bitcoin/pull/33669#pullrequestreview-3364281398)
Concept ACK, but I agree with @TheCharlatan that this repo is not be the most productive place to put them.

💬 m3dwards commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#issuecomment-3431070343)
ACK fabe0e07de1ad2f26da62f3ebe0e9be3f939b1f8

This will save a lot of space and help to prevent the other caches from getting evicted which are much more important than the docker build layer caches. Should only slow down the jobs by a few minutes at most and hopefully be a net positive as the other more important caches (like ccache) will not have been evicted.

Ran on master on my own fork.

💬 Raimo33 commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#issuecomment-3431076499)
Concept ACK

💬 hebasto commented on pull request "randomenv: Fix MinGW dllimport warning for `environ`":
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3431122616)
Friendly ping people who reviewed the previous [change](https://github.com/bitcoin/bitcoin/pull/30491):

@sipa @sipsorcery @theuni

💬 laanwj commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2450954020)
nit: please capitalize P2P (another one below in `btck_block_to_bytes`)

💬 sipa commented on pull request "randomenv: Fix MinGW dllimport warning for `environ`":
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3431135242)
utACK 9610b0d1e28aeda02a2ddcf1f0591ae577c3e88e if this makes the compilers happy

👍 laanwj approved a pull request: "refactor: optimize block index comparisons (1.4-6.8x faster)"
(https://github.com/bitcoin/bitcoin/pull/33637#pullrequestreview-3364368958)
Code review ACK c15d839aefba017e64f14cd9cd2779655352f4b6
i did not run the benchmarks but the code changes look good, using `<=>` makes sense.