💬 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));`
💬 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.
(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`
(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
...
(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
(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.
(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
(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
💬 fanquake commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2233502884)
> you can see this variable only has meaning when cross compiling for windows and android
Given, for example, that if you cross-compile for macOS, with this PR, you end up with different linker flags, this clearly has effect outside of Windows and Android.
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2233502884)
> you can see this variable only has meaning when cross compiling for windows and android
Given, for example, that if you cross-compile for macOS, with this PR, you end up with different linker flags, this clearly has effect outside of Windows and Android.
🚀 fanquake merged a pull request: "refactor: Make m_last_notified_header private"
(https://github.com/bitcoin/bitcoin/pull/30466)
(https://github.com/bitcoin/bitcoin/pull/30466)
🤔 ryanofsky reviewed a pull request: "multiprocess: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2183161316)
re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233301504
> If you split the capnp additions out of 4e1a434 it's a bit easier for me to combine this PR with #29432 (by omitting the interface changes commit here or there).
Hmm, what specifically would you like me to change here? This PR will be pretty difficult for me maintain if it based on other PRs, so it is just based on master.
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2183161316)
re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2233301504
> If you split the capnp additions out of 4e1a434 it's a bit easier for me to combine this PR with #29432 (by omitting the interface changes commit here or there).
Hmm, what specifically would you like me to change here? This PR will be pretty difficult for me maintain if it based on other PRs, so it is just based on master.
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1681193702)
re: 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?
It's not really possible without making the PR bigger because mining code relies on createNewBlock returning CBlockTemplate not BlockTemplate, and I want mining code to work and CI to pass.
The purpose of createNewBlock2 is just to make sure serialization works when createNewBlock is changed to
...
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1681193702)
re: 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?
It's not really possible without making the PR bigger because mining code relies on createNewBlock returning CBlockTemplate not BlockTemplate, and I want mining code to work and CI to pass.
The purpose of createNewBlock2 is just to make sure serialization works when createNewBlock is changed to
...
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681187567)
nit: my understanding is that you've chosen this style to mimic the calls in `secp256k1_keypair_xonly_pub` and friends - but maybe it would make sense to migrate to `C++` style here, I couldn't find any other `bool` in `cpp` files where we're assigning ints, like we do in the `C` files.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681187567)
nit: my understanding is that you've chosen this style to mimic the calls in `secp256k1_keypair_xonly_pub` and friends - but maybe it would make sense to migrate to `C++` style here, I couldn't find any other `bool` in `cpp` files where we're assigning ints, like we do in the `C` files.
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681077911)
nit: it might be simplified to `ret =` - or even `bool ret =` if we narrow the scope of the previous one.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681077911)
nit: it might be simplified to `ret =` - or even `bool ret =` if we narrow the scope of the previous one.
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681222762)
nit: typos in commit message: `Accumlute` and `returning early on the function`
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681222762)
nit: typos in commit message: `Accumlute` and `returning early on the function`
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681219368)
off topic: I'm out of my depth here, but curious, what's the reason for not needing a cleanup of keypair in the merkle_root branch as well?
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681219368)
off topic: I'm out of my depth here, but curious, what's the reason for not needing a cleanup of keypair in the merkle_root branch as well?
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681189987)
Is there any advantage in reusing `ret`, or could we do `bool ret = secp256k1_keypair_xonly_pub` and return inside the if instead?
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681189987)
Is there any advantage in reusing `ret`, or could we do `bool ret = secp256k1_keypair_xonly_pub` and return inside the if instead?
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681216935)
off topic: we might need a [copy of pubkey_bytes](https://github.com/bitcoin/bitcoin/blob/b946f8a4c51be42e52d63a6d578158c0b2a6b7ed/src/pubkey.cpp#L199) in this case, but I guess that's unrelated to the PR
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681216935)
off topic: we might need a [copy of pubkey_bytes](https://github.com/bitcoin/bitcoin/blob/b946f8a4c51be42e52d63a6d578158c0b2a6b7ed/src/pubkey.cpp#L199) in this case, but I guess that's unrelated to the PR