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

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

📝 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:
...

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

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

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

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

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

💬 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));`

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681170920)
> The way the bound was computed was inaccurate; I have fixed that

I'm unsure where the mis-estimate was or how it was fixed.

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2233483074)
reviewed through https://github.com/bitcoin/bitcoin/pull/30126/commits/2003bb8a279c8891e55bab190ca36f0c6c8697ea

via `git range-diff master 23496cb 2003bb8a279c8891e55bab190ca36f0c6c8697ea`

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681178728)
Let me try to explain here. If it helps you, that may be a reason to incorporate it as a comment.

Overall the serialization format consists of:
* For each transaction:
* VARINT(size)
* VARINT(fee)
* List of VARINTs that are the number of skipped options (see below), encoding dependencies and position.
* VARINT(0)

Let's say you have a 7-transaction cluster, consisting of transactions F,A,C,B,G,E,D, but serialized in order A,B,C,D,E,F,G, because that's a topological ordering. By t
...

💬 hebasto commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2233496140)
Here is the previous discussion on this topic: https://github.com/hebasto/bitcoin/pull/3#discussion_r892222352.

In particular, @ryanofsky [noted](https://github.com/hebasto/bitcoin/pull/3#discussion_r893226408):
> I think this doc is overgeneralizing. If you look at https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html you can see this variable only has meaning when cross compiling for windows and android

📝 maflcko opened a pull request: "fuzz: Speed up PickValue in txorphan"
(https://github.com/bitcoin/bitcoin/pull/30474)
`PickValue` will advance a begin iterator on the `outpoints` set, which is expensive, because it only has a `++` operator. As it is called in a loop of `num_in` (~`outpoints.size()`), the runtime is `O(outpoints.size() ^ 2)`.

Fix it by making the runtime linear.

💬 maflcko commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1681188649)
This is a bit hidden, but `PickValue` will advance a begin iterator on this set, which is expensive, because it only has a `++` operator.

Fixed in https://github.com/bitcoin/bitcoin/pull/30474