💬 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).
(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.
(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
...
(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)
(https://github.com/bitcoin/bitcoin/issues/30456)
🚀 fanquake merged a pull request: "doc: getaddressinfo[isscript] is optional"
(https://github.com/bitcoin/bitcoin/pull/30457)
(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).
(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.
(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?
(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.
...
(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?
(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.
(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?
(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
(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...
(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.
(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.
(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.
💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681140016)
Don't go out of your way for now, this may just be a mental block for myself only. I worked out some examples myself and it works, I just can't mentally model the skipping somehow.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681140016)
Don't go out of your way for now, this may just be a mental block for myself only. I worked out some examples myself and it works, I just can't mentally model the skipping somehow.
📝 maflcko opened a pull request: "fuzz: Limit parse_univalue input length"
(https://github.com/bitcoin/bitcoin/pull/30473)
The new limit should be more than enough, and hopefully avoids fuzz input bloat, such as `parse_univalue/0426365704e09ddd704a058cc2add1cbf104c1a9`. C.f. https://cirrus-ci.com/task/6178647134961664?logs=ci#L3805
```
Run parse_univalue with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/parse_univalue')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed:
...
(https://github.com/bitcoin/bitcoin/pull/30473)
The new limit should be more than enough, and hopefully avoids fuzz input bloat, such as `parse_univalue/0426365704e09ddd704a058cc2add1cbf104c1a9`. C.f. https://cirrus-ci.com/task/6178647134961664?logs=ci#L3805
```
Run parse_univalue with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/parse_univalue')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed:
...
💬 hebasto commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#discussion_r1681144357)
> Where/how does CMake translate this to `OSX_MIN_VERSION`?
https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2232957753:
> ... the `CMAKE_SYSTEM_VERSION` value is used to define `DARWIN_MAJOR_VERSION` and `DARWIN_MINOR_VERSION`...
It happens [here](https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Platform/Darwin.cmake?ref_type=heads#L18-27):
```cmake
# Darwin versions:
# 6.x == Mac OSX 10.2 (Jaguar)
# 7.x == Mac OSX 10.3 (Panther)
# 8.x == Mac OSX 10.4 (Ti
...
(https://github.com/bitcoin/bitcoin/pull/30465#discussion_r1681144357)
> Where/how does CMake translate this to `OSX_MIN_VERSION`?
https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2232957753:
> ... the `CMAKE_SYSTEM_VERSION` value is used to define `DARWIN_MAJOR_VERSION` and `DARWIN_MINOR_VERSION`...
It happens [here](https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/Platform/Darwin.cmake?ref_type=heads#L18-27):
```cmake
# Darwin versions:
# 6.x == Mac OSX 10.2 (Jaguar)
# 7.x == Mac OSX 10.3 (Panther)
# 8.x == Mac OSX 10.4 (Ti
...
💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681159878)
oh whoops, wrong one. I meant for the loop above!
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681159878)
oh whoops, wrong one. I meant for the loop above!