💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2657139983)
Ah, I didn't test with libFuzzer. I guess to support that I'll have to port https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/test/fuzz/test_runner.py#L170

I'll do that tomorrow.

✅ fanquake closed a pull request: "cmake: Install man pages for configured targets only"
(https://github.com/bitcoin/bitcoin/pull/31765)

💬 fanquake commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2657151761)
Closing given the concept ACK in the other thread.

💬 fanquake commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#issuecomment-2657172457)
https://github.com/bitcoin/bitcoin/pull/31662/checks?check_run_id=37171033299:
```bash
[10:53:16.049] gmake[2]: Entering directory '/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu'
[10:53:16.093] [ 72%] Linking CXX executable fuzz
[10:53:16.093] cd /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz && /usr/bin/cmake -E cmake_link_script CMakeFiles/fuzz.dir/link.txt --verbose=1
[10:53:16.117] /usr/bin/clang++-20 -ftrivial-auto-var-init=pattern -O2 -g -fsanitize=fuz
...

💬 sipa commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2657179844)
Mild concept ACK. I think having all binaries in one place is a nice, but not critical improvement. If we're doing it, we should do it now, though.

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1954865233)
In commit "rpc: handle shutdown during long poll and wait methods" (0c06c8108fb43be317d86199750ecd3b7db9a5d2)

I think this code is not quite right because it is assuming that if `getTip()` returns null it means the node is shutting down, when in reality it is starting up. Also the IsRPCRunning() call unnecessarily complicates things and is incorrect as we found previously in #30635. Would also be nice to reduce number of variables used in this code and remove CHECK_NONFATAL calls that can't t
...

💬 glozow commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2657198949)
@hebasto @theuni can you open a PR to prune the gcc stuff for v29?

💬 sipa commented on pull request "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports":
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2657199198)
I suggest treating the broken GCC-based coverage as a 29.0 blocker, as it seems it's been effectively unusable anyway.

Adding an LLVM-based coverage mechanism then seems like an independent improvement.

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1954895162)
I'll defer to you, but IIUC we'll do ~1 orphan processing per message processing step, so it might take a bit more to process all 24 from orphanage? Alternatively e could query the *first* tx and wait until the descendant count hits DEFAULT_ANCESTOR_LIMIT

👍 stickies-v approved a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2615433035)
tACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90

Being able to specify install components for all targets is helpful, and this implementation seems very clean. Part of me annoyed is that the consistency isn't there for kernel (`bitcoinkernel` target and `libbitcoinkernel` component), but it seems like that's conventional for libraries so I'll suck it up.

💬 stickies-v commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#discussion_r1954741365)
I think this is now duplicated and can be removed?

🤔 danielabrozzoni reviewed a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758#pullrequestreview-2615727315)
Whoops, I had forgotten about this one :)

re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53

💬 hodlinator commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2657262496)
Would probably have leaned towards just updating the comment and leaving the test just out of paranoia, even though so much is different with the `std::chrono` implementation that anything similar is unlikely to re-occur. Getting rid of lines to maintain has value too.

💬 theuni commented on pull request "depends: add missing Darwin objcopy":
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2657264201)
@hebasto Agreed as long as it's trivial. From what I can tell `llvm-install-name-tool` is just as available as the rest of the tools, so even though it's annoying because it's not actually necessary, I don't see much harm in setting it. @fanquake ?

This required a similar hack for https://github.com/bitcoin/bitcoin/pull/30434 as well.

💬 brunoerg commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657278221)
> q: isn't the test just wrong?. `min_viable_change` is initialized to the future input fee without increasing the window by one as we do in the actual code. Thats why there is an overlap here. See the wallet code:
>
> [bitcoin/src/wallet/spend.cpp](https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/src/wallet/spend.cpp#L1131-L1132)
>
> Lines 1131 to 1132 in [c242fa5](/bitcoin/bitcoin/commit/c242fa5be358150d83c2446896b6f4c45c6365e9)
>
> const auto change_spend_f
...

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2657285774)
Rebased c6658d675d0bb945f53dc62f7e9b95c7bba973bb -> b2108240dec5d858d17af798fdc79037f894786d ([`pr/subtree.15`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.15) -> [`pr/subtree.16`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.16), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.15-rebase..pr/subtree.16)) to fix conflict with #31834. Also added another fix for #30975 and documented & unhid CAPNP_EXECUTABLE variable as suggested

re: https://github.com/bi
...

🚀 glozow merged a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495)

💬 ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2657295215)
@Sjors Note if you rebase this on #31741 you should be able to drop ab831be2a93d9c30408192ca4eaa1552ac6bc3dc and 19f693e6b81e5e0e75b820be5ba7a53911b4918c workarounds since a more direct fix for the fuzz build issues was added (96d3b2a0bb0c267569162b055ca306f709ec0b4e). Also of course can drop be8abf9c55c5279ca36cd080626b8071733ac6c1 since #31834 was merged

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657299519)
@stickies-v Thanks for pointing that out, that was accidental! I actually didn't notice the difference. I think we should fix it so that they're aligned instead of having a single outlier.

Though.. would it make sense to rename the target to `libbitcoinkernel` instead of changing the component to `bitcoinkernel`? I think the former would be more obvious?

💬 brunoerg commented on pull request "test: check `scanning` field from `getwalletinfo`":
(https://github.com/bitcoin/bitcoin/pull/31768#discussion_r1954941746)
Good suggestion, but I will leave as is for now to not invalidate the reviews. Thanks.