💬 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
🤔 paplorinc reviewed a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2182966578)
Started reviewing, but got a bit stuck at the first commit, will continue a bit later
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2182966578)
Started reviewing, but got a bit stuck at the first commit, will continue a bit later
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681189065)
> we do not change behavior by exiting early before attempting to sign if there is a failure with the merkle_root logic.
Previously, when an error occurred, subsequent calculations weren't performed.
Since `&=` doesn't short-circuit (and even if it did, `pubkey_bytes` would be allocated and `ComputeTapTweakHash` would run even if `secp256k1_keypair_xonly_pub` failed), the methods could be called even though a previous dependency failed.
Are you sure this is just a refactoring and not a beh
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681189065)
> we do not change behavior by exiting early before attempting to sign if there is a failure with the merkle_root logic.
Previously, when an error occurred, subsequent calculations weren't performed.
Since `&=` doesn't short-circuit (and even if it did, `pubkey_bytes` would be allocated and `ComputeTapTweakHash` would run even if `secp256k1_keypair_xonly_pub` failed), the methods could be called even though a previous dependency failed.
Are you sure this is just a refactoring and not a beh
...
📝 Sjors opened a pull request: "Stratum v2 Template Provider common functionality"
(https://github.com/bitcoin/bitcoin/pull/30475)
Based on #30332. Parent PR #29432.
This contains all Template Provider functionality that can be used by both #29432 and the IPC based alternative in [link to PR].
(https://github.com/bitcoin/bitcoin/pull/30475)
Based on #30332. Parent PR #29432.
This contains all Template Provider functionality that can be used by both #29432 and the IPC based alternative in [link to PR].
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2233560101)
I split off the Template Provider class into #30475. The non-base commits in this PR start the TP from `init.cpp` and add some other things that are useful for testing (e.g. testnet4).
A WIP for the IPC sidecar approach can be found [insert PR]. It's also based on #30475.
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2233560101)
I split off the Template Provider class into #30475. The non-base commits in this PR start the TP from `init.cpp` and add some other things that are useful for testing (e.g. testnet4).
A WIP for the IPC sidecar approach can be found [insert PR]. It's also based on #30475.