💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1617508249)
Updated

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1617509629)
13.2 is from April 2023: https://www.freebsd.org/releases/13.2R/announce/
On FreeBSD it is easy and smooth to upgrade the OS (at least my experience since FreeBSD 4.x) even across major versions (e.g. 13 -> 14). Seems fine to only support >= 13.2.

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1617510788)
I've fixed that, plus increased the `min_required_fds` to also account for automatic outbounds in the last commit. I'm open to discuss about the latter, but I think it's something we should also account for

📝 fanquake opened a pull request: "guix: show `*_FLAGS` variables in pre-build output"
(https://github.com/bitcoin/bitcoin/pull/30185)
For example:
```bash
# ADDITIONAL_GUIX_COMMON_FLAGS set in the ENV
ADDITIONAL_GUIX_ENVIRONMENT_FLAGS="--emulate-fhs" ./contrib/guix/guix-build
<snip>
INFO: Building f751991 for platform triple x86_64-linux-gnu:
...using reference timestamp: 1716905119
...running at most 10 jobs
...from worktree directory: '/bitcoin'
...bind-mounted in container to: '/bitcoin'
...in build directory: '/bitcoin/guix-build-f75199182133/distsrc-f75199182133-x86_64-linux-gnu
...

✅ BenWestgate closed a pull request: "doc: Change GiB to GB in release-process.md"
(https://github.com/bitcoin/bitcoin/pull/30171)

👍 theuni approved a pull request: "feefrac: 128-bit multiply support in MSVC"
(https://github.com/bitcoin/bitcoin/pull/29758#pullrequestreview-2083318452)
utACK 5fb70b5b72c96a7e640900de56393b0bdb9df37a

(I didn't look into the additional unit tests)

Godbolt output here:
https://godbolt.org/z/8Msz984bv

Compiles down to roughly the same.

💬 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.