💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1995687071)
Correct, especially now after maflcko's change in fae3bf6b870eb0f9cddd1adac82ba72890806ae3 to not call `stop()` on test failure, I agree it makes sense to assert instead, good suggestion! Fixed.

> were there actually any cases where CannotSendRequest exceptions were caught and suppressed? If so what were those cases? Commit description should say what goals of this change are and if CannotSendRequest is an actual or theoretical change in behavior.

It was a common knock-on issue from attemp
...

💬 maflcko commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724927316)
Would be nice to have steps to test this with gcc and clang. Otherwise, it will be harder to test and the real benefits are harder to see and touch. Also the pull title could be adjusted to mention the Debug restriction.

💬 maflcko commented on pull request "test: fix intermittent failure in wallet_reorgsrestore.py":
(https://github.com/bitcoin/bitcoin/pull/32069#issuecomment-2724938439)
introduced in 11f8ab140fe63857f6a93b81021efda8f90ceeda, so added 30.0 milestone

💬 maflcko commented on pull request "test: fix intermittent failure in wallet_reorgsrestore.py":
(https://github.com/bitcoin/bitcoin/pull/32069#issuecomment-2724943277)
lgtm ACK 0f377b4ef959d5722c02d39d63916233f6e30c64

Maybe there could be a method on test_node, similar to stop_node to kill the node ?

💬 hodlinator commented on pull request "RFC: Add `operator""_uint256` compile-time user-defined literal":
(https://github.com/bitcoin/bitcoin/pull/31991#issuecomment-2724946307)
> Would be happy to ack a doc fix of the upstream msvc bug report

Yes, it seems like our issue was closer this "sub-issue" in the currently linked issue: https://developercommunity.visualstudio.com/t/consteval-conversion-function-fails/1579014#T-N10550785 and they apparently only fixed the primary issue. :\

💬 Sjors commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724958506)
That did the trick.

💬 furszy commented on pull request "test: fix intermittent failure in wallet_reorgsrestore.py":
(https://github.com/bitcoin/bitcoin/pull/32069#issuecomment-2724965232)
> Maybe there could be a method on test_node, similar to stop_node to kill the node ?

Done as suggested.

💬 maflcko commented on pull request "build: align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2724974512)
> So my question is.. what does depends built with `-O0` actually get us?

It can't hurt to have the option to have a stronger msan build for depends (see https://github.com/bitcoin/bitcoin/issues/22064#issuecomment-2607120689), but I don't know if it will be beneficial by default or in other scenarios.

💬 maflcko commented on pull request "test: fix intermittent failure in wallet_reorgsrestore.py":
(https://github.com/bitcoin/bitcoin/pull/32069#issuecomment-2724997907)
lgtm ACK 36b0713edc4655f6e0c291975d6d280fbc89cf2e

📝 Sjors opened a pull request: "build: use make < 3.82 syntax for define directive"
(https://github.com/bitcoin/bitcoin/pull/32070)
From the GNU 3.82 [release notes](https://www.cloudbees.com/blog/whats-new-gnu-make-382):

> The define directive now supports the same assignment operators that regular variable assignment alows: := , ?= and += , for simple, conditional and appending assignments.

macOS ships with 3.81. This caused the multiprocess config options to be ignored.

Fixes #32068

💬 Sjors commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2725004999)
Opened #32070 to fix it.

💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725007393)
We don't seem to specify a minimum make version?

💬 ryanofsky commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2725048933)
No strong opinion. I guess I'd be a little disappointed if you initially had a general solution that made it easy to enable buffering any place and then were told that instead of using that, you should add kludges to this code and implement ad-hoc buffering here. I don't think buffering is difficult to reason about and don't think buffering is the type of thing that needs to be deployed on a case-by-case basis, especially since performance of buffering should mostly depend on *where* data is bei
...

💬 darosior commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1995800292)
BIP9 does not define the period as configurable, it's always the retarget period. No strong opinion, but i find it surprising that the BIP9 warning mechanism should support (a specific type of) non-BIP9 deployments.

👍 ryanofsky approved a pull request: "build: use make < 3.82 syntax for define directive"
(https://github.com/bitcoin/bitcoin/pull/32070#pullrequestreview-2685973287)
Code review ACK 9157d9e449870851ef455e077249ac46fc2df24c. This is a pretty unusual bug and I don't understand how it wasn't causing any errors with make 3.81, just causing the flags to be ignored.

(Another case where there was a problem with make 3.81 was https://github.com/bitcoin-core/libmultiprocess/issues/139 / https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2621793644)

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2725084083)
Further idea around the second sentence of this:

https://github.com/bitcoin/bitcoin/pull/10738#issue-240283546:
> Special care must be taken, though, to only delete CNodes from a single thread, and to control which thread that is. Eventually, this should move to the message processing thread, so that it's not possible to hit cs_main from the main network thread.

It is possible to decouple `CNode` deletion from the `FinalizeNode()` call and do the latter in the `msghand` thread regardless
...

💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725088984)
> I don't understand how it wasn't causing any errors with make 3.81

I probably haven't recently used the depends build to test multiprocess on my development machine. I would either use my locally install `libmultiprocess` or a (cross-compiled) Guix build.

💬 maflcko commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725091163)
> This is a pretty unusual bug and I don't understand how it wasn't causing any errors with make 3.81, just causing the flags to be ignored.

In light of silent failures, it would be better to rule out this class of bug completely. Otherwise, a new instance will be added in the future, or will already exist somewhere in the code.

I am not sure why reviewers should spend time on supporting build tools that were released about 20 years ago. So rejecting such a build tool early with a failure
...

💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725097671)
@maflcko I'm fine with introducing a minimum GNU make version, as long as we actually check the version (otherwise we're back to silent failures).

But note that I run the stock macOS GNU make all the time, as do many other devs. So new issues are likely to be caught during review, though they'll still waste time debugging.

💬 maflcko commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725105639)
(unrelated to the make issue, the same conceptual problem exists for the macOS bash version, but I guess the bash version check would be even harder to achieve and is only reliably avoidable by not writing bash code)