🤔 marcofleon reviewed a pull request: "addrman: cap the `max_pct` to not exceed the maximum number of addresses"
(https://github.com/bitcoin/bitcoin/pull/31235#pullrequestreview-2429970052)
Tested ACK 9c5775c331e02dab06c78ecbb58488542d16dda7. Reproduced the crash on master and checked that this fixed it. The checks added to `GetAddr_` look reasonable.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2470893774)
addrman test failure for win64, assuming unrelated: https://github.com/bitcoin/bitcoin/pull/30239/checks?check_run_id=32868463159

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838351237)
will take a crack at it in follow-up

💬 fanquake commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2470949674)
> addrman test failure for win64, assuming unrelated:

Yea this is a Wine issue (see #31071 & the linked issues). I restarted the job.

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838145180)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834309312

> Looking at the usage of this endpoint I am wondering if it is even necessary? I.e. instead of getting it first and using it for `chainStateFlushed`, the `chainStateFlushed` implementation could just do that internally.

Current usages are a little unusual because chainStateFlushed() is used in the wallet both to receive notifications from the node about when to flush, but also internally in the wallet to flush its ow
...

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838364191)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834365825

> Why is the `role` typed as `UInt32` when the other enums are typed as `Int32`?

Changes this to `Int32` to be consistent. Probably `UInt32` was used because this parameter was added at a later time than all the other code was written. The exact integer type used to represent the enum doesn't actually matter as long as it is wide enough to hold all the enum values, and as long as round-trip static casts from the enum
...

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838369167)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834376651

> Should this include the version as well at this point?

Yes, good catch. Added the version field and added a comment to JSONRPCRequest struct to prevent this type of bug in the future. I think bug this did not matter too much in practice because this capnp struct is only used to forward RPC requests from the node to the wallet and wallet shouldn't respond to requests differently based on JSONRPC version, but it is st
...

🤔 ryanofsky reviewed a pull request: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-2429667905)
Updated 968f14fc7a634f65f4399dc042a37d4a39f2c703 -> 34d3e2a6eaaab9de5328c3e64739f1392696c7db ([`pr/ipc-chain.9`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.9) -> [`pr/ipc-chain.10`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-chain.9..pr/ipc-chain.10)) with review suggestions and fixes.

Thanks for the review!

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838152072)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834348410

> Nit: Shouldn't this be called `txid`?

Nice catch, fixed! Might be a good idea to make sure these names are correct, even though they are just documentation and don't affect behavior.

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838357818)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834362068

> How does this `write` argument here map to the `action` argument in the c++ interface? They seem to have different types.

This is a bug, and it should have been caught by the libmultiprocess library. https://github.com/chaincodelabs/libmultiprocess/pull/120 adds a static_assert to ensure that it would result in a compile error if a specified capnproto type is ever not compatible with a c++ enum type.

🤔 mzumsande reviewed a pull request: "addrman: cap the `max_pct` to not exceed the maximum number of addresses"
(https://github.com/bitcoin/bitcoin/pull/31235#pullrequestreview-2430152067)
Code Review ACK 9c5775c331e02dab06c78ecbb58488542d16dda7

💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2471040172)
> > Which I think can be removed now, no?
>
> No, we can't. Only for Windows builds `*.exe` and `*.dll` files reside in a single `bin` subdirectory. On other platforms, shared libraries still reside in their own `lib` subdirectory, which requires the continued use of RPATH.
>
> > Edit: Also, I think this obsoletes the `CMAKE_SKIP_BUILD_RPATH` comment in `CMakeLists.txt`.
>
> I believe this comment is still relevant.

Yes, you're right on both counts. I forgot about the different lib p
...

✅ maflcko closed a pull request: "ci: Bump valgrind tasks to clang-18"
(https://github.com/bitcoin/bitcoin/pull/31273)

💬 maflcko commented on pull request "ci: Bump valgrind tasks to clang-18":
(https://github.com/bitcoin/bitcoin/pull/31273#issuecomment-2471093188)
Closing as duplicate of https://github.com/bitcoin/bitcoin/issues/29635. Lets keep the discussion in one place.

✅ Sjors closed an issue: "v27.2 guix build fails with hash mismatch"
(https://github.com/bitcoin/bitcoin/issues/31266)

💬 Sjors commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-2471100700)
I was able to do a complete guix build for v27.2: https://github.com/bitcoin-core/guix.sigs/pull/1433

💬 marcofleon commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#discussion_r1838474496)
Good call, done.

👍 danielabrozzoni approved a pull request: "doc: mention `descriptorprocesspsbt` in psbt.md"
(https://github.com/bitcoin/bitcoin/pull/31277#pullrequestreview-2430251087)
ACK ebb6cd82baf8406454b18afcb8ccb4e1dde0d43e

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1838505227)
> Yeah, looking at it again, this doesn't make sense because the test may fail if someone actually scales the wait time down. I changed it back and will leave the mocktime approach for a follow-up.


Source: https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1780172239

🚀 glozow merged a pull request: "test: enhance p2p_orphan_handling"
(https://github.com/bitcoin/bitcoin/pull/31037)