💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1617052946)
Ah, good catch, I ran `clang-format-diff` over all commits and incorporated all suggestions, thanks.

🤔 dergoegge reviewed a pull request: "fuzz: increase `txorphan` harness stability"
(https://github.com/bitcoin/bitcoin/pull/30186#pullrequestreview-2083575630)
I suspect that the scripted diff is failing in CI but passing on your Mac because sed behaves differently on Mac vs Linux. Maybe [this](https://stackoverflow.com/questions/4247068/sed-command-with-i-option-failing-on-mac-but-works-on-linux) helps?

💬 dergoegge commented on pull request "fuzz: increase `txorphan` harness stability":
(https://github.com/bitcoin/bitcoin/pull/30186#discussion_r1617723588)
Should be `m_` not `n_`, see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

💬 maflcko commented on pull request "fuzz: increase `txorphan` harness stability":
(https://github.com/bitcoin/bitcoin/pull/30186#issuecomment-2135883451)
> but passing on your Mac

The script shouldn't be passing on macos. Otherwise, https://github.com/bitcoin/bitcoin/commit/b3122e167a023df18f67c4083bc72f78968874db doesn't work as intended and should be fixed.

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1617773662)
@laanwj One possibility is moving the code to a separate .c file, perhaps?

🤔 theuni reviewed a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2083365482)
Concept ACK and first-pass code review ACK excluding tests.

I don't feel qualified to review the rand_expo_duration changes.

I manually verified the randbits impls with some local tests as the shifts weren't intuitive to me at first glance.

💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1617727453)
Add a quick zero-case here too?

💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1617597271)
Neat :)

💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1617743561)
Why not do a rand32() if size() >= 4 first?

💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1617765084)
Unrelated change?

Edit: Or because of use of m_rng?

💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1617782150)
This is kinda sneaky and surprising (to me at least) to see with CRTP.

It would be nice to specify as part of the concept which functions are _not_ allowed to be overridden, but sadly
[std::experimental::detected_t](https://en.cppreference.com/w/cpp/experimental/is_detected) is.. well.. experimental.

💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1617786101)
Mmh, not sure what iwyu was reporting here before, but it seems correct now. Please resolve :)

💬 TheCharlatan commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1617787010)
I meant the forward-declaration itself. Is it used for something?

👍 theuni approved a pull request: "[25.x] windeploy: Renew certificate"
(https://github.com/bitcoin/bitcoin/pull/30184#pullrequestreview-2083681671)
ACK 7a4eff2c98eb41e304cfb03372bce401909bf3d0

`git diff origin/pr/30184..9f4ff1e9659597307f62510f1885ad8da3a1d9a3 -- contrib/windeploy/win-codesign.cert`
(empty)

👍 TheCharlatan approved a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2083683221)
Re-ACK 1a5ab1c27bfce12c46c1dcbb0922c8602798b20c

💬 maflcko commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2135959767)
> This PR doesn't change the libraries required to be installed on the user's system

Correct. I tested this in three different boxes and it worked.

![Screenshot from 2024-05-28 21-25-14](https://github.com/bitcoin/bitcoin/assets/6399679/d
...

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1617796685)
Well being able to ad-hoc override individual functions, while still being able to have non-overridden functions be able to refer to it, is sort of the point of CRTP (virtual functions have the same behavior, but CRTP gives it without runtime overhead). If we didn't want this ability, it'd be possible to just have a `template<typename R> class RNGUtility { R m_int; ...}`, where there are low-level `ChaCha20RNG` and `XoRoShiRo128PlusPlusRNG` classes, and high-level `using FastRandomContext = RNG
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1617802306)
That would probably work, but that'd be a really invasive workaround build system wise.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1617811698)
It appeared to be necessary, IIRC.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1617833166)
Done.