💬 edilmedeiros commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2876927264)
I agree with you, but I also prefer the current code; seems easier to understand.

💬 Sjors commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2876927696)
I added `OP_2` through `OP_16`, with a new test case for the latter. It's not practical to catch _all_ possible trivial scripts though.

📝 fanquake opened a pull request: "build: add `-W[leading|trailing]-whitespace`"
(https://github.com/bitcoin/bitcoin/pull/32482)
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. Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
```bash
[ 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/qrc_bitcoin.cpp:5603:2: warning: trailing
...

💬 edilmedeiros commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2876932472)
Code Review ACK 6adec89e3961508875e79240a19bc981e5addf1a (untested)

💬 l0rinc commented on pull request "lint: Check for missing trailing newline":
(https://github.com/bitcoin/bitcoin/pull/32477#discussion_r2087077335)
Did a git clean -fxd before running it so I think it should've covered only the tracked files.

------

Will let you decide of course which ones makes sense to include, just pointing out that we may want to extend this list.

💬 edilmedeiros commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2876941545)
Code Review reACK 2be93f7ba3866892d98795384779ce2e965753a1 (untested)

💬 l0rinc commented on pull request "lint: Check for missing trailing newline":
(https://github.com/bitcoin/bitcoin/pull/32477#discussion_r2087083142)
I haven't tested it, if you say it's fast enough, I believe you

💬 l0rinc commented on pull request "lint: Check for missing trailing newline":
(https://github.com/bitcoin/bitcoin/pull/32477#issuecomment-2876944750)
ACK fa9198af55df74b0c19c9125d256ad4df83cf005

🤔 caesrcd reviewed a pull request: "config: allow setting -proxy per network"
(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
...

💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(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!

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

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

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

🤔 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

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

👍 fanquake approved a pull request: "Update `secp256k1` subtree to latest master"
(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)

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