💬 theuni commented on pull request "feefrac: 128-bit multiply support in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29758#discussion_r1617568664)
To help other reviewers:
```c++
__int64 _mul128(
__int64 Multiplier,
__int64 Multiplicand,
__int64 *HighProduct
);

Return value
The low 64 bits of the product.
```

So this should result in:
ret = <high, low>

Which matches the other impls.

💬 sipa commented on pull request "feefrac: 128-bit multiply support in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29758#discussion_r1617616315)
Note the weird return value `__int64` here, which IMHO makes no sense whatsoever. Semantically, the return value is 2⁶⁴`HighProduct` + (uint64_t)`ret`.

🤔 sr-gi reviewed a pull request: "test/BIP324: disconnection scenarios during v2 handshake"
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2083422330)
I love it now 🚀

ACK ae528cb9e29b75d36a8018954f45bcfb886a4557

🤔 mzumsande reviewed a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2083413620)
Concept ACK

💬 mzumsande commented on pull request "net: log connections failures via SOCKS5 with less severity":
(https://github.com/bitcoin/bitcoin/pull/30064#discussion_r1617626592)
I think that the change in this line should be explained better / maybe be its own commit. `LogConnectFailure` is not related to SOCKS5 but also reached through `ConnectDirectly()`, so it's not clear to me from the commit msg or PR description why the log is changed.

📝 marcofleon opened a pull request: "fuzz: increase `txorphan` harness stability"
(https://github.com/bitcoin/bitcoin/pull/30186)
This moves `nNextSweep` from being a static variable in `LimitOrphans` to being a member of the `TxOrphanage` class. This improves the stability of the `txorphan` fuzz harness, as each orphanage (created every iteration) now has its own value for `nNextSweep`.

This PR builds upon https://github.com/bitcoin/bitcoin/pull/30170 which is still open, so it's a draft for now.

💬 dergoegge commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2135762064)
rfm?

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1617665519)
Right, that's good to know. However the current problem is that we can't compile this for *any* FreeBSD version. Looks like the `MLMSG` etc macros don't work in C++, because `typeof` is used.

👍 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?