🤔 caesrcd reviewed a pull request: "config: allow setting -proxy per network"
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2837204950)
reACK e98c51fcce
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2837204950)
reACK e98c51fcce
💬 hebasto commented on pull request "build: add `-W[leading|trailing]-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2876950405)
> GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
Concept ACK on that.
> Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
>
> ```shell
> [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> /root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW
...
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2876950405)
> GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
Concept ACK on that.
> Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
>
> ```shell
> [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> /root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW
...
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2087092911)
Done
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2087092911)
Done
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2087093235)
Done!
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2087093235)
Done!
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2087093815)
Added the comment as requested.
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2087093815)
Added the comment as requested.
💬 fanquake commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2876976229)
> Here are the identified typographic and grammatical errors that impact comprehension in the provided git diff:
>
> CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd); -> CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd)); [missing closing parenthesis]
I can't seem to find `CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd)` in either our, or libsecp256k1s codebase.
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2876976229)
> Here are the identified typographic and grammatical errors that impact comprehension in the provided git diff:
>
> CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd); -> CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd)); [missing closing parenthesis]
I can't seem to find `CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd)` in either our, or libsecp256k1s codebase.
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2876983186)
Force-pushed to address recent comments by @Sjors:
1. Added an explanation for why we ignore the result of `WaitTipChanged`.
2. Clarified the comment for `AddMerkleRootAndCoinbase`.
3. Shortened line lengths for improved readability.
Additionally, in the recent push, I added a commit that skips the fee check during block template total fees computation in `waitNext`. This check is redundant after #31897.
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2876983186)
Force-pushed to address recent comments by @Sjors:
1. Added an explanation for why we ignore the result of `WaitTipChanged`.
2. Clarified the comment for `AddMerkleRootAndCoinbase`.
3. Shortened line lengths for improved readability.
Additionally, in the recent push, I added a commit that skips the fee check during block template total fees computation in `waitNext`. This check is redundant after #31897.
🤔 ismaelsadeeq reviewed a pull request: "Add checkBlock() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31981#pullrequestreview-2837265952)
Concept ACK after https://bitcoincore.reviews/31981
(https://github.com/bitcoin/bitcoin/pull/31981#pullrequestreview-2837265952)
Concept ACK after https://bitcoincore.reviews/31981
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2877008733)
> unless @luke-jr/Knots argues convincingly that the removal of these config options in Core makes maintaining them in Knots an excessive amount of work
It really shouldn't be. After this PR it's a matter of changing the default. After we actually drop the deprecated option, it would be a matter of reverting that one commit.
> I don't know if it makes sense for Core to try catering to Knots' objectives
In general Bitcoin Core provides very limited catering to forks, whether that's altc
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2877008733)
> unless @luke-jr/Knots argues convincingly that the removal of these config options in Core makes maintaining them in Knots an excessive amount of work
It really shouldn't be. After this PR it's a matter of changing the default. After we actually drop the deprecated option, it would be a matter of reverting that one commit.
> I don't know if it makes sense for Core to try catering to Knots' objectives
In general Bitcoin Core provides very limited catering to forks, whether that's altc
...
👍 fanquake approved a pull request: "Update `secp256k1` subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/32028#pullrequestreview-2837285736)
ACK 915c1fa72c07a1908f7dc18a065230c7c4a64db2
(https://github.com/bitcoin/bitcoin/pull/32028#pullrequestreview-2837285736)
ACK 915c1fa72c07a1908f7dc18a065230c7c4a64db2
🚀 fanquake merged a pull request: "Update `secp256k1` subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/32028)
(https://github.com/bitcoin/bitcoin/pull/32028)
💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2877018237)
re-tACK 4e1aae19512df82af584a064640c2143c5c5fa4f
Briefly retested on macOS. Will retest on Windows later.
Main changes since my last review:
- using subprocess in Windows to replace a bunch of string manipulation code (good idea @theStack)
- `bitcoin node` instead of `bitcoin daemon` to disambiguate from (detaching) `-daemon`
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2877018237)
re-tACK 4e1aae19512df82af584a064640c2143c5c5fa4f
Briefly retested on macOS. Will retest on Windows later.
Main changes since my last review:
- using subprocess in Windows to replace a bunch of string manipulation code (good idea @theStack)
- `bitcoin node` instead of `bitcoin daemon` to disambiguate from (detaching) `-daemon`
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2877028785)
Thanks. I still need to address the questions by @stringintech.
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2877028785)
Thanks. I still need to address the questions by @stringintech.
💬 bytejedi commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2877049144)
Bitcoin is a digital currency that OP_RETURN should NOT EXIST ever since the beginning. You all turning bitcoin to shitcoin. Look what you all did these years, just fix bugs and CVEs please.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2877049144)
Bitcoin is a digital currency that OP_RETURN should NOT EXIST ever since the beginning. You all turning bitcoin to shitcoin. Look what you all did these years, just fix bugs and CVEs please.
💬 maflcko commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2877052289)
> I can't seem to find `CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd)` in either our, or libsecp256k1s codebase.
See https://github.com/maflcko/DrahtBot/issues/50.
The prior result was correct, but it (obviously) can't be fixed here in a subtree bump anyway:
- dependendencies -> dependencies [typo]
- uninitalized -> uninitialized [typo]
- lamba -> lambda [typo]
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2877052289)
> I can't seem to find `CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd)` in either our, or libsecp256k1s codebase.
See https://github.com/maflcko/DrahtBot/issues/50.
The prior result was correct, but it (obviously) can't be fixed here in a subtree bump anyway:
- dependendencies -> dependencies [typo]
- uninitalized -> uninitialized [typo]
- lamba -> lambda [typo]
🤔 stickies-v reviewed a pull request: "kernel: Separate UTXO set access from validation functions"
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2802813202)
Concept ACK
Reducing the coupling between validation functions and the UTXO set makes the validation functions more usable, and more testable. The approach taken here seems sensible, but I'm going to think about alternatives a bit more.
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2802813202)
Concept ACK
Reducing the coupling between validation functions and the UTXO set makes the validation functions more usable, and more testable. The approach taken here seems sensible, but I'm going to think about alternatives a bit more.
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2066960211)
I presume adding `block_hash` is done to avoid calculating `CBlockHeader::GetHash()` multiple times? Did you find that to have a measurable impact somewhere?
I think the most elegant approach would be to implement caching inside `CBlockHeader::GetHash()` (if meaningful), but that would involve a pretty big LoC change making the `CBlockHeader` members private and adding getters.
If we stick with this approach, I think at least it should be documented that `block_hash` is duplicated for perf
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2066960211)
I presume adding `block_hash` is done to avoid calculating `CBlockHeader::GetHash()` multiple times? Did you find that to have a measurable impact somewhere?
I think the most elegant approach would be to implement caching inside `CBlockHeader::GetHash()` (if meaningful), but that would involve a pretty big LoC change making the `CBlockHeader` members private and adding getters.
If we stick with this approach, I think at least it should be documented that `block_hash` is duplicated for perf
...
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2075811232)
Since `std::reference_wrapper<T>` implicitly converts to `T&`, I don't think we actually need this helper? At least it compiles fine on my machine. If you've found any compiler issues, perhaps documenting them would be good so we can remove it in the future?
<details>
<summary>git diff on 76a8f22c5c</summary>
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..f2a0259b23 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2075811232)
Since `std::reference_wrapper<T>` implicitly converts to `T&`, I don't think we actually need this helper? At least it compiles fine on my machine. If you've found any compiler issues, perhaps documenting them would be good so we can remove it in the future?
<details>
<summary>git diff on 76a8f22c5c</summary>
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..f2a0259b23 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify
...
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2065965592)
nit: I think this is an `@param[in,out]`?
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2065965592)
nit: I think this is an `@param[in,out]`?
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2075895517)
nit: for non-trivial functions, I think it helps to narrow down `T` instead of having to deduct it from its callsites and usage.
What do you think about adding an `IsCoinRef` concept, and adding `std::span` to the function signature?
<details>
<summary>git diff on 76a8f22c5c</summary>
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..ab27387cc4 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -14,6 +14,8 @
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2075895517)
nit: for non-trivial functions, I think it helps to narrow down `T` instead of having to deduct it from its callsites and usage.
What do you think about adding an `IsCoinRef` concept, and adding `std::span` to the function signature?
<details>
<summary>git diff on 76a8f22c5c</summary>
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..ab27387cc4 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -14,6 +14,8 @
...