✅ Sjors closed a pull request: "subprocess: always check result of close()"
(https://github.com/bitcoin/bitcoin/pull/33061)

💬 Sjors commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#issuecomment-3118343686)
It appears that underlying issue is now understood to be signal safety, see #33063. So improving `close` error reporting and handling isn't a priority.

💬 maflcko commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#issuecomment-3118364788)
> it's just `throw` and `sysconf` that are the problem?

No, all of the C++ standard libary is problematic as well. (See the pull description and the bt)



> How do you verify a reverted merge commit? I guess this worked:

I typed `git revert -m1 31d3eebfb92ae0521e18225d69be95e78fb02672`, but `git` is very flexible and you can type many different things to arrive at the same result.

💬 darosior commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#issuecomment-3118381836)
Concept ACK...

👍 darosior approved a pull request: "util: Revert "common: Close non-std fds before exec in RunCommandJSON""
(https://github.com/bitcoin/bitcoin/pull/33063#pullrequestreview-3055837132)
ACK faa1c3e80d95552bdc2c0e717065ebf8d510138f

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2231370673)
Done

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2231372594)
Reworked it 👍

🤔 mzumsande reviewed a pull request: "p2p: rename GetAddresses -> GetAddressesUnsafe"
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3055853811)
Code review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

🚀 fanquake merged a pull request: "p2p: rename GetAddresses -> GetAddressesUnsafe"
(https://github.com/bitcoin/bitcoin/pull/32994)

💬 fanquake commented on pull request "[29.x] final changes for v29.1rc1":
(https://github.com/bitcoin/bitcoin/pull/33056#issuecomment-3118510216)
ACK 4c2d285b706058ece7092d03f0e5ad1112c2097f

🚀 fanquake merged a pull request: "[29.x] final changes for v29.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33056)

💬 pinheadmz commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3118790215)
I'm unable to reproduce so far building master at 6cdc5a90cffe9ced4ec50d2028c9896d25a9cb6a


on Debian:

`cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang-16' -DCMAKE_CXX_COMPILER='clang++-16' -DCMAKE_CXX_FLAGS='-std=c++20' -DBUILD_GUI=OFF -DBUILD_TESTS=OFF -DSANITIZERS=thread && cmake --build ./bld-cmake --parallel $( nproc ) && TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1" ./bld-cmake/test/functional/wallet_multiwallet.py`

on
...

💬 olegrok commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3119155140)
> [@olegrok](https://github.com/olegrok) If you want, you can test [#33002](https://github.com/bitcoin/bitcoin/pull/33002)

It looks a bit tricky (however I can't suggest solution that will be better) but this fixes an issue.

💬 ishaanam commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#issuecomment-3119410214)
re-utACK ea17a9423fb431a86d36927b02d3624f654fd867

I used range_diff to compare to my previous review. The changes were just to address review comments and rebase.

📝 theStack opened a pull request: "rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes`"
(https://github.com/bitcoin/bitcoin/pull/33065)
This PR takes use of the `FormatAllOutputTypes` helper (introduced in PR #32432, commit 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640) to get rid of the remaining hardcoded output types in wallet RPC documentation [1]. Note that it can't be used in the [`createmultisig` RPC](https://github.com/bitcoin/bitcoin/blob/fc162299f0cc5b61bbb55736fe85e27b705f78cc/src/rpc/output_script.cpp#L100), as this one is only for pre-taproot output types and hence doesn't contain "bech32m" in the list.

[1] instances
...

🤔 jonatack reviewed a pull request: "ci: Run unit test parallel with functional tests"
(https://github.com/bitcoin/bitcoin/pull/33000#pullrequestreview-3056247871)
I don't know how much variance we typically see in CI run times, but light lgtm ACK fab25c6f5803ae8699f068d089f1c6e4f1387743

```
$ /test_runner.py -h
--valgrind run nodes under the valgrind memory error detector: expect at least a ~10x slowdown. valgrind 3.14 or later required. Does not apply to previous release
binaries.
--bash-cmd-extra-tests BASH_CMD_EXTRA_TESTS
Run an arbitrary string as Bash command in parallel to the fu
...

🤔 LarryRuane reviewed a pull request: "test: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL"
(https://github.com/bitcoin/bitcoin/pull/33021#pullrequestreview-3056300082)
tACK b2d82668a57d4ab4eef310c25a2aebda18563677
I thoroughly reviewed and understand all code changes. This test perfectly tests `GetCoinsCacheSizeState()` and is much simpler and less fragile. (Also, I learned a few C++ tricks, very nice.)

I ran the modified test using the debugger (on Ubuntu), and it behaves as designed. The only very minor suggestion if you need to retouch is to consider adding a comment explaining why it's not necessary to verify that the loops didn't exceed `MAX_ATTEMPTS`
...

💬 glozow commented on pull request "p2p: stop special-casing witness-stripped error for unconfirmed transactions":
(https://github.com/bitcoin/bitcoin/pull/32379#issuecomment-3119690432)
I ran the above patch for a while and added logging to collect some stats and see how much more redownloading we’d do. TLDR it seems like we can definitely go ahead with it.

Caveats: Traffic has been very low so I set my mempool expiry to 1 hour to try to induce more orphan-fetching. I’m a default policy node so I don’t really expect to be relayed many transactions I’ll reject. There isn’t much opportunity to test reconsiderable rejections right now because of how low transaction traffic is.
...

💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2231703938)
This is better, will implement.

💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2231710505)
Will implement, the `ScopedScheduler` is better imo and `std::shared_ptr` makes more sense over `std::unique_ptr`.

> With a shared pointer, we can safely and explicitly manage these dependencies without introducing coupling between the logger and the scheduler.

I think there will always be coupling between the two since `Reset` may call `LogPrintLevel_`?