👍 brunoerg approved a pull request: "refactor: Use type-safe time in txorphanage"
(https://github.com/bitcoin/bitcoin/pull/30170#pullrequestreview-2083510013)
utACK fa6d4891c7815025ab1cd58d0906466af31bb648

💬 stickies-v commented on pull request "[27.x] Backports and rc1":
(https://github.com/bitcoin/bitcoin/pull/30092#issuecomment-2135801234)
re-ACK 22701a43464ab27ea83c8b49e2732ee647909a10

🤔 stickies-v reviewed a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2082493959)
Force-pushed to address @TheCharlatan's (cosmetic) feedback: incorporated the clang-format-diff suggestions and removed a couple of unnecessary includes.

💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1617310014)
Forward declaring this doesn't compile for me:

```
In file included from test/timeoffsets_tests.cpp:6:
In file included from ./node/timeoffsets.h:8:
In file included from ./sync.h:14:
In file included from ./threadsafety.h:9:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/mutex:191:
In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__memory/shared_ptr.h:30:
In file included from /Library/De
...

💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1617056982)
Because there's only one line in between? I agree it's very obvious now, but I think it doesn't hurt to have it and removes the arbitrariness of when it does become useful so I'd rather just be consistent and always have it?

💬 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