💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680976118)
There is no `uint160S()` outside of **uint256_tests.cpp** and only `uint256` and `uint160` use `base_blob` so it felt alright having it here. If we abandon the `uint256S()`-overload approach I agree to change this.

📝 Igor258 opened a pull request: "btc"
(https://github.com/bitcoin/bitcoin/pull/30472)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ fanquake closed a pull request: "btc"
(https://github.com/bitcoin/bitcoin/pull/30472)

📝 fanquake locked a pull request: "btc"
(https://github.com/bitcoin/bitcoin/pull/30472)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680983277)
Generally agree https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680963050

(Some call-sites do use "0x"-prefix now that I think of it, while others do not, but aligning that would probably be an easy diff to accept).

💬 hodlinator commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680986937)
> > I want to enforce `consteval` for existing string literals. Ideally the `string_view` overload should be `constexpr` but from my research on MSVC assembly output
>
> I think `string_view` can be used in a consteval context, no?

Yes, it's the rest of that paragraph that poses some concern.

💬 fanquake commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680995968)
> Ideally the string_view overload should be constexpr but from my research on MSVC assembly output

Please don't be overly concerned about MSVC. We've had issues with it in the past, where it's failed to optimize code properly, or we've had to write workarounds for it, when code was otherwise fine in Clang or GCC, i.e (https://github.com/bitcoin/bitcoin/pull/28657#discussion_r1360780446). We don't use it as a release compiler (and never will), so if it's failing to do X, that isn't necessaril
...

✅ fanquake closed an issue: "getaddressinfo: complains missing `isscript` when called on unknown witness version"
(https://github.com/bitcoin/bitcoin/issues/30456)

🚀 fanquake merged a pull request: "doc: getaddressinfo[isscript] is optional"
(https://github.com/bitcoin/bitcoin/pull/30457)

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233301504)
If you split the capnp additions out of 4e1a4342f3b2a857e3211587cd14e36704197483 it's a bit easier for me to combine this PR with #29432 (by omitting the interface changes commit here or there).

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1681052659)
Ok, I was having NAT in mind when I wrote "is going to result in multiple mappings on the same external IPv6 address". Sorry for the noise.

💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1681055963)
Is it possible to rename this to `createNewBlock` and drop the one above? Or is there an inconsistency between my PRs?

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1681059488)
Please disregard my previous comments if they contradict this one. My apologies for any confusion.

On Windows, creating symbolic links requires a [permission](https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-10/security/threat-protection/security-policy-settings/create-symbolic-links), which is disabled by default. Therefore, CMake creates hard links (or copies as a fallback). In that case, `os.path.realpath(__file__)` will point at a file in the build directory.


...

🤔 theuni reviewed a pull request: "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds"
(https://github.com/bitcoin/bitcoin/pull/30465#pullrequestreview-2182947860)
Concept ACK, but we need more docs here.

- I can't tell (even from looking at the CMake docs) what macOS should be set to and why you've chosen this value
- I can only assume that the Linux value means "minimum kernel version" for the sake of kernel headers, but I don't see that in the docs either
- Agree with @fanquake that it's not clear how this affects our other version vars. Which take precedence?

💬 theuni commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#discussion_r1681066373)
Where/how does CMake translate this to `OSX_MIN_VERSION`? Ideally we'd compose these somehow.

💬 maflcko commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#issuecomment-2233404249)
Should the check be applied to miniscript_string, for example the `miniscript_string/ae395bdc087e233d7f8e1844d4814b2c00cc9d21` input, as well?

💬 danielabrozzoni commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1681109056)
nit: you should change this comment to reflect the fact that you have three nodes now

💬 instagibbs commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1681129115)
yeah I was having a real hard time understanding what a lot of these fields actually mean...

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681134991)
> returning subset in this case would not be correct, as it's possible that a bigger intersection still has a higher feerate.

Ah I had forgotten this fact, makes total sense because it doesn't hurt to just stop here.

💬 maflcko commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1681137919)
I think it means "some kind of pay to script hash", that is SH or WSH.

I guess it would be clearer to just return an enum as string, rather than a collection of boolean fields.