💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2061620107)
Switched to use `os.linesep` which I think is more idiomatic... But I'm not entirely happy with how I ended up having to escape:
https://github.com/bitcoin/bitcoin/blob/b19439b1eee57041fc37dcf509d5ee9f248263bc/test/functional/feature_framework_startup_failures.py#L76-L79

It's a side-effect of using `repr(e)` in 28e282ef9ae94ede4aace6b97ff18c66cb72a001 which gives the escaped form.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2061626467)
Thanks for catching that. I would have leaned towards `--tm(p(d(i(r?)?)?)?)?=` but I like the non-capturing groups, don't recall coming across them before. Taken.

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2832642819)
ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c

👍 hodlinator approved a pull request: "test: avoid stack overflow in `FindChallenges` via manual iteration"
(https://github.com/bitcoin/bitcoin/pull/32351#pullrequestreview-2796453342)
ACK 2e533cf86a42b3416c1164d04089e80ffe622dd6

Prefer this alternative of switching to an iterative algorithm (instead of bumping the stack space #32349).

On master we do a depth-first gathering of challenges, the new stack based version does breadth-first, but result is put in a `std::set` so traversal order does not matter.

💬 hodlinator commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#discussion_r2061627575)
In bd89f7eff0dc6c44c6a0b96883c3816fe0919f95, why not just:
```C++
BOOST_CHECK(challenges_recursive == challenges);
```
?
https://en.cppreference.com/w/cpp/container/set/operator_cmp claims the complexity is linear with the size.

💬 hodlinator commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2832650664)
Was curious how this issue was introduced. Seems like it's existed for some time but was found by #32339 momentarily switching some Windows CI to use Debug configs. Would it be worth adding to the description instead of just "ref: #32339"?

💬 hebasto commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2832694789)
Concept ACK.

💬 maflcko commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2833289993)
The erroneous format specs still remain: https://github.com/bitcoin/bitcoin/issues/32295#issuecomment-2827238571

💬 maflcko commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2833334943)
The error seems to be intermittent, because now it looks like the centos task passed. (For reference, the failure three days ago was https://github.com/bitcoin/bitcoin/runs/41112451543)

👍 hebasto approved a pull request: "test: avoid stack overflow in `FindChallenges` via manual iteration"
(https://github.com/bitcoin/bitcoin/pull/32351#pullrequestreview-2797575186)
ACK 2e533cf86a42b3416c1164d04089e80ffe622dd6, I have reviewed the code and it looks OK.

If we put refactoring aside, the changes in this PR could be much smaller and easier to review with `git diff --ignore-all-space`:
```diff
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -297,9 +297,13 @@ using miniscript::operator""_mst;
using Node = miniscript::Node<CPubKey>;

/** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */
...

✅ hebasto closed a pull request: "test: Increase stack size for "Debug" builds with MSVC"
(https://github.com/bitcoin/bitcoin/pull/32349)

💬 hebasto commented on pull request "test: Increase stack size for "Debug" builds with MSVC":
(https://github.com/bitcoin/bitcoin/pull/32349#issuecomment-2833346711)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/32351.

💬 l0rinc commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#issuecomment-2833349523)
> On master we do a depth-first gathering of challenges, the new stack based version does breadth-first, but result is put in a std::set so traversal order does not matter.

I've reworded the commit message to make this clearer.
The original method is pre-order depth-first, where the children are processed in the same order they appear (Left-to-right). The new method is still pre-order depth-first, but children are inserted in the reverse order (Right-to-left). But as you mention, it's in a s
...

💬 l0rinc commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#discussion_r2062574904)
This helped me debug the mentioned vector conversion, but it's not important anymore, so your suggestion also works.

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2062566402)
nit: maybe it's better to return a `std::span` from `m_recv_buffer` to avoid a copy here.

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2062570060)
Maybe use a list of buffers (instead a single FIFO buffer)?

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2062572600)
nit: can be simplified using
```c++
void WriteReply(HTTPStatusCode status, std::string_view reply_body_view)
{
WriteReply(status, std::as_bytes(std::span{reply_body_view}));
}
```

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2062569706)
Would it be possible to allow writing a reply composed of several `span<byte>`?
It should allow sending while serializing a response object.

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2062573530)
Can be removed if we'll add `WriteReply(HTTPStatusCode status, std::string_view reply_body_view)` (see above).

💬 maflcko commented on pull request "doc: Fix fuzz test_runner.py path":
(https://github.com/bitcoin/bitcoin/pull/32353#issuecomment-2833354624)
lgtm ACK 61f238e84ac6d24d8f420c2eabcbb2980d7fcb1e