🚀 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!
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233469862)
Can you add support for `-loglevel`? Typically when testing Stratum v2 stuff I'll run with `-debug=sv2 -loglevel=sv2:trace`.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233469862)
Can you add support for `-loglevel`? Typically when testing Stratum v2 stuff I'll run with `-debug=sv2 -loglevel=sv2:trace`.
💬 hebasto commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2233469874)
> 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?
I agree that the documentation is not comprehensive.
As
...
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2233469874)
> 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?
I agree that the documentation is not comprehensive.
As
...
💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681165800)
I'm not particularly bothered by it for the reasons you mentioned. I'll let others weigh in.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681165800)
I'm not particularly bothered by it for the reasons you mentioned. I'll let others weigh in.
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233474486)
While trying to start the Template Provider from `bitoin-mine` it crashes as soon as it tried any libsecp operation, at this line: `ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));`
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233474486)
While trying to start the Template Provider from `bitoin-mine` it crashes as soon as it tried any libsecp operation, at this line: `ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));`