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

💬 stratospher commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#discussion_r1680964397)
true, done.

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680966315)
> `uint256(uint64_t,uint64_t,uint64_t,uint64_t)` constructor. That way it would be the compiler parsing hexadecimal integer literals directly.

Not sure. That'd make it impossible to grep for a (let's say) block hash. Also, it would be harder to copy-paste one, if the developer has to manually split it into 4 parts and add `0x` prefixes. Finally, truncation checks can't be done, see https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680422462

> was to avoid changing all the call-sit
...

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680967973)
> 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?

💬 maflcko commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#issuecomment-2233211540)
reACK c322bddd08ed1f1f7f0c39768b659dd62d5e2dd5

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