💬 rebroad commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2844242309)
I'm finding it hard to understand the nuance that is potentially being missed in the arguments in either direction. I think node operators ought to have a say in the transactions they want to support or not support, so as long as the filters are optional (and Bitcoin Core developers choose the defaults), then I'm in favour of including filters that have at least 33% support of node operators.

⚠️ laanwj opened an issue: "Use of deprecated CryptAcquireContext in random.cpp"
(https://github.com/bitcoin/bitcoin/issues/32391)
According to [Windows API documentation](https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptacquirecontextw), the `CryptAcquireContext*` function as used in `src/random.cpp` is deprecated:

> Important This API is deprecated. New and existing software should start using [Cryptography Next Generation APIs.](https://learn.microsoft.com/en-us/windows/desktop/SecCNG/cng-portal) Microsoft may remove this API in future releases.

💬 laanwj commented on issue "Use of deprecated CryptAcquireContext in random.cpp":
(https://github.com/bitcoin/bitcoin/issues/32391#issuecomment-2844260438)
OH this is a duplicate of #14089, which was closed at some point. Not sure whether to leave this open.

🤔 shahsb reviewed a pull request: "refactor: validation: mark CheckBlockIndex as const"
(https://github.com/bitcoin/bitcoin/pull/32364#pullrequestreview-2809300600)
Concept ACK

💬 shahsb commented on pull request "refactor: validation: mark CheckBlockIndex as const":
(https://github.com/bitcoin/bitcoin/pull/32364#issuecomment-2844282604)
ACK https://github.com/bitcoin/bitcoin/pull/32364/commits/657e58bcd739b52e7b9c9349cd14f05b6b79297c

💬 laanwj commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2069950447)
Could use `elif`, but as the effect is the same, better to roll this into one `if`, i think:
```python
if (low_s and s > secp256k1.GE.ORDER_HALF) or (not low_s and s < secp256k1.GE.ORDER_HALF):
s = ORDER - s
```

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844353635)
i'm going to approach this differently, turning this PR draft for now.

Going to remove https://github.com/bitcoin/bitcoin/pull/32343/commits/269c2a3dc5c3c85ce73f66c354895f768576ce51 here and try to upstream similar functionality optimizing `close_fds` (written more generally, not using bitcoin-core code) to cpp-subprocess. We can then use that code here.

📝 laanwj converted_to_draft a pull request: "common: Close non-std fds before exec in RunCommandJSON"
(https://github.com/bitcoin/bitcoin/pull/32343)
Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).

> Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and star
...

📝 laanwj converted_to_draft a pull request: "common: Close non-std fds before exec in RunCommandJSON"
(https://github.com/bitcoin/bitcoin/pull/32343)
Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).

> Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and star
...

💬 hebasto commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844357436)
FWIW, I'm running the current PR branch on some other OSes: https://github.com/hebasto/bitcoin-core-nightly/pull/51.

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844368951)
> FWIW, I'm running the current PR branch on some other OSes: https://github.com/hebasto/bitcoin-core-nightly/pull/51.

Testing is very welcome, the overall approach is going to stay the same.

💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844382194)
> (written more generally, not using bitcoin-core code)

Okay, after a bit of consideration, maybe this was a bad idea: the directory iteration and string-parsing is going to be horrible.

cpp-subprocess is C++11 only, while `std::filesystem` and `std::from_chars` are C++17. i really don't want to go back to using locale-dependent number parsing functions.

📝 AndreaLanfranchi opened a pull request: "Update .gitignore"
(https://github.com/bitcoin/bitcoin/pull/32392)
Build process on WIndows creates the "out" folder by default which is not excluded from changes tracked by git.
Also additional directories `.vs` and `.vscode` are created with files exclusively relevant to the local development environment: commits should not be dirtied by those.

This PR
* Excludes dirs used by VS family.
* Also exclude "out" build directory (default on WIndows)

💬 hebasto commented on pull request "Update .gitignore":
(https://github.com/bitcoin/bitcoin/pull/32392#issuecomment-2844423790)
We generally do not accept IDE-specific entries in the project’s `.gitignore`. Please adjust your build environment by other means.

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2844423967)
`cc8f239663...8ef769cc2d`: rebase and use the naming:

```cpp
bool IsConnectedToAddrPort(const std::string& addr_port);
bool IsConnectedToAddrPort(const CService& addr_port);
bool IsConnectedToAddr(const CNetAddr& addr);
```

✅ fanquake closed a pull request: "Update .gitignore"
(https://github.com/bitcoin/bitcoin/pull/32392)

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2070021226)
Done.

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2070024083)
#32338 was merged and these variables are not necessary now.

💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2844434880)
`8ef769cc2d...4f635d100d`: make the methods `const`

👋 vasild's pull request is ready for review: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326)

💬 hebasto commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844437349)
@laanwj

Before undrafting this PR. could you please rebase it to include the merged https://github.com/bitcoin/bitcoin/pull/32071. Otherwise, NetBSD [tests](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/14772257138) fail.