🚀 fanquake merged a pull request: "ci: Enable more shellcheck"
(https://github.com/bitcoin/bitcoin/pull/32970)

💬 maflcko commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#discussion_r2231277189)
> Maybe only change this and don't undo the already upstreamed change (yet)?

I don't understand what you are referring to. Can you provide a link?

💬 maflcko commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#issuecomment-3118175723)
> Is this related? [arun11299/cpp-subprocess#53](https://github.com/arun11299/cpp-subprocess/issues/53)

Yes. My recommendation would be to fix this upstream. I currently don't plan to do it, so anyone else is free to pick it up.

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

I agree it makes sense to fix upstream first.

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

```sh
maflcko/2507-tempoary-revert
git cherry-pick 4f5e04da135080291853f71e6f81dd0302224c3a^..a0eed55398f882d9390e50582b10272d18f2b836
git diff maflcko/2507-tempoary-revert~1
```

So do I understand correctly from the man page you linked to that `close()` and `getpid` and are already signal safe, and it's just `throw` and `sysconf` that are the problem?

💬 fanquake commented on issue "ci: Lower and unify default stack size":
(https://github.com/bitcoin/bitcoin/issues/29840#issuecomment-3118322437)
Any opinions on doing this via link flags? We can instruct the linker to set stack size limits on the executable during linking. One advantage might be that we can set varying limits, per build, but that might not be useful.

If not, I'll pickup the ulimit approach.

👍 darosior approved a pull request: "[29.x] final changes for v29.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33056#pullrequestreview-3055779227)
ACK 4c2d285b706058ece7092d03f0e5ad1112c2097f

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