💬 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.
🚀 fanquake merged a pull request: "test: bump mocktime only after node has received and sent bytes"
(https://github.com/bitcoin/bitcoin/pull/30468)
(https://github.com/bitcoin/bitcoin/pull/30468)
💬 pablomartin4btc commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2233580171)
@hebasto, I couldn't reproduce the issue described in https://github.com/bitcoinknots/bitcoin/issues/83, trying both `master` and this PR, also checked the [fix in knots](https://github.com/bitcoinknots/bitcoin/releases/tag/v27.1.knots20240621) which is similar to this PR code change. It would be good to know what was the state of the "mask value" during the use case described in https://github.com/bitcoinknots/bitcoin/issues/83, the only thing I can think of it's that the issue is the same one
...
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2233580171)
@hebasto, I couldn't reproduce the issue described in https://github.com/bitcoinknots/bitcoin/issues/83, trying both `master` and this PR, also checked the [fix in knots](https://github.com/bitcoinknots/bitcoin/releases/tag/v27.1.knots20240621) which is similar to this PR code change. It would be good to know what was the state of the "mask value" during the use case described in https://github.com/bitcoinknots/bitcoin/issues/83, the only thing I can think of it's that the issue is the same one
...
🤔 hebasto reviewed a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2183191027)
Approach ACK f2712ed278ec44ad100bdbe765de1716191f238b.
I've reviewed all patches for now.
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2183191027)
Approach ACK f2712ed278ec44ad100bdbe765de1716191f238b.
I've reviewed all patches for now.
💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681210501)
The missed EOL causes "Hunk #1 succeeded at 234 with fuzz 1."
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681210501)
The missed EOL causes "Hunk #1 succeeded at 234 with fuzz 1."
💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681211287)
The missed EOL causes "Hunk #4 succeeded at 1540 with fuzz 1."
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681211287)
The missed EOL causes "Hunk #4 succeeded at 1540 with fuzz 1."
💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681214480)
```suggestion
@@ -599,7 +599,7 @@ if(NOT MSVC)
```
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1681214480)
```suggestion
@@ -599,7 +599,7 @@ if(NOT MSVC)
```