💬 maflcko commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#issuecomment-3520585217)
> Coverage before adding test: <img alt="before" width="2000" height="66" src="https://github.com/user-attachments/assets/28cb35b1-5127-4f82-ba74-5891bd36553c">
>
> Coverage after adding test: <img alt="after" width="2000" height="66" src="https://github.com/user-attachments/assets/41ebbd66-872e-4ba4-b550-84ff9bc61752">

I don't think those links work. Also, this is redundant to the corecheck result anyway?

👋 maflcko's pull request is ready for review: "ci: Enable experimental kernel stuff in most CI tasks"
(https://github.com/bitcoin/bitcoin/pull/33824)

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3520618909)
> we do getBlock and that provides everything we need

Ok, that's not really how you're supposed to use it :-)

It does make a breaking change potentially unpainful.

However there's another gotcha if we remove a method from the interface: there's no gaps allowed in the `.capnp` method numbering. This could be worked around as follows:

```capnp
interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
destroy @0 (context :Proxy.Context) -> ();
getBlockHeader @1 (cont
...

👍 TheCharlatan approved a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3452188347)
Re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97

🤔 hodlinator reviewed a pull request: "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState"
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3452156315)
Concept ACK moving out-reference-parameters to return value

Not familiar with the kernel API but had a brief look at net_processing and validation.

💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517333716)
Should remove the shadowing `state` on line 4515 IMO.

💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517330536)
Why not reuse `state` here?

💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517354777)
Worth adding an `Assume(!headers.empty())` before this, as `state` will be unset if we don't process any, whereas before we would return `true` regardless?

💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2517387755)
I don't think there was ever a `signtransaction`, it was just a typo in the docs. The external signer feature was introduced in v22. The docs from that time mentions `signtransaction` and not `signtx`: https://github.com/bitcoin/bitcoin/blob/v22.0/doc/external-signer.md

Looking at the code from v22.0 however, the actual command was already `signtx`.

💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2517395290)
`signtx` is not inherently stdin

All (required) commands should work with or without `--stdin`. Bitcoin Core currently only uses it for `signtx`, and that's worth mentioning, but it can change.

💬 hodlinator commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3520773269)
Thanks for trying this out @Eunovo!

> Also, attempting to reindex while offline wouldn't work anymore if we are below minchainwork.

Would it be acceptable to emit an error and halt, saying that `-reindex` for cases where nodes don't have sufficient block data also requires decreasing `-minimumchainwork=` to whatever level we have in the block data?
Could also explain that this is in order to keep logic consistent regardless of `-reindex`, and also say that running with lowered `-minimumch
...

💬 Sjors commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3520877571)
ACK 866bbb98fd365962840ee99df0d9f7ad557cc025

On macOS 26.0.1 I ran the full test suite with `-DENABLE_IPC` set to `OFF` and `ON`.

💬 janb84 commented on pull request "build: Bump g++ minimum supported version to 12":
(https://github.com/bitcoin/bitcoin/pull/33842#issuecomment-3520878892)
re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97

💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3520921384)
Here is my Guix build on RISC-V machine:
```
riscv64
451630ecff800ab320a9d5ad8062758df9d700310690110e764c916ae4c8121e guix-build-527acc5ee497/output/dist-archive/bitcoin-527acc5ee497.tar.gz
bf19a3b8e9e9cf609102c38cd6c00dca4d2645f66ae3af591f29fdeceef7b6cf guix-build-527acc5ee497/output/x86_64-w64-mingw32/SHA256SUMS.part
b69dc956f6fd6bb9b4e20e5f4d29eb4ef74e450d250346bf0450cc051b5cfa95 guix-build-527acc5ee497/output/x86_64-w64-mingw32/bitcoin-527acc5ee497-win64-codesigning.tar.gz
9e44c7635
...

🤔 frankomosh reviewed a pull request: "transaction: Adding script witness to ToString for CTxIn"
(https://github.com/bitcoin/bitcoin/pull/33711#pullrequestreview-3452460486)
I think the goal for this is reasonable, but doesn't it remove witness data from `CTransaction::ToString()` and break existing debug output?

💬 frankomosh commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2517605640)
Though I have not done a thorough test, I think that completely removing this might affect debug workflow. Before removing it, `Ctransaction::ToString()` is able to print all inputs, then a separate block with all witnesses, then all outputs. I don’t think this is possible anymore.

💬 frankomosh commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2517569031)
I think there might be an inconsistency in formatting here. `scriptWitness.ToString()` is called directly, while `scriptSig` uses `HexStr().substr(0, 24)`. Shouldn't there be some symmetry ?

💬 quapka commented on issue "Fuzz: compare our AES implementation to AES-NI ":
(https://github.com/bitcoin/bitcoin/issues/27548#issuecomment-3521038218)
> Since cryptofuzz is not alive anymore

There is a bit of activity in [Mozilla's fork](https://github.com/MozillaSecurity/cryptofuzz). It's also still present in [OSS-Fuzz](https://github.com/google/oss-fuzz/tree/master/projects/cryptofuzz), but references the non-existent original repository, so probably is not ran anymore.

👍 hebasto approved a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3452644296)
ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97.

💬 willcl-ark commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3521153141)
I get a guix build failure @ 758da5a5732:

```
g++ -pie -Wl,-z,now -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Wconditionally-supported -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -DHAVE_CONFIG_H -pie -Wl,-z,now -static-libstdc++ -static-libgcc -o Tcollect2 \
collect2.o collect2-aix.o vec.o ggc
...

💬 maflcko commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3521179236)
I am not using GCC, and I am not sure how many devs are using GCC-15. Is there even a ci task using gcc-15? Maybe Alpine 3.23 later this month? I haven't looked at the build system changes, otherwise this seems fine to do.