💬 m3dwards commented on pull request "guix: produce a `-static-pie` bitcoind":
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-3503574153)
This is very cool.

Have I got the potential trade-offs correct here?

Positives:

- More portable - especially running a modern binary on an older linux
- Enable very small docker images

Downsides:

- Static glibc can struggle with resolvers and locale (although I don't think locale is an issue here) potentially undermining the portability benefit. I don't know if `--enable-static-nss` solves this? But as referenced in this [line](https://github.com/bitcoin/bitcoin/pull/25573/files#
...

💬 luke-jr commented on pull request "Comment out sensitive console commands in history to prevent re-execution":
(https://github.com/bitcoin-core/gui/pull/909#discussion_r2504521790)
Why does this need a signal/slot?

💬 luke-jr commented on pull request "Comment out sensitive console commands in history to prevent re-execution":
(https://github.com/bitcoin-core/gui/pull/909#discussion_r2504531173)
Should probably check this before parsing (top of RPCConsole::on_lineEdit_returnPressed)

💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3503617446)
Guix build for a02282d20c87:

- dist-archive
- `bitcoin-a02282d20c87.tar.gz`: `79d1ff42f480bd6894b1d44fd7a59bc7c3b17cf25cccae7fe10b7f540d37af10`
- aarch64-linux-gnu
- `bitcoin-a02282d20c87-aarch64-linux-gnu-debug.tar.gz`: `6e3b9a2e39e207d98c347ccc0a81dfc6c79484befa5dbbeaaccdd0b44b759e99`
- `bitcoin-a02282d20c87-aarch64-linux-gnu.tar.gz`: `827f5c070c2ee3c8e1eb96cfd6f05551e4a69a00e422f808c15840026c7a27fc`
- `SHA256SUMS.part`: `a3647a46e00124b207263b695d9598cf9fe6c845ea9072c933462dd0
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504559542)
In the longer run it probably makes sense for `sv2-tp` to have one major release of sv2-tp for every major Bitcoin Core release, but current it doesn't have backports. Each release is a combination of bug fixes and new features.

📝 stickies-v opened a pull request: "kernel: trim Chain interface"
(https://github.com/bitcoin/bitcoin/pull/33820)
Removes `btck_chain_get_genesis` and `btck_chain_get_tip`.

They are trivially replaced with `btck_chain_get_by_height` (as indicated in the updated `bitcoinkernel_wrapper.h`, so I think it makes sense to trim the interface.

For `btck_chain_get_tip`: on `master` we don't provide any guarantees that the returned block index still corresponds to the actual tip, so the extra call doesn't seem like a regression to me.

💬 average-gary commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#issuecomment-3503637844)
@l0rinc I applied your patch in commits that I hope are focused enough and made you co-author. Thank you.

I'll find some time to review the rest of the comments and address as needed after this patch.

💬 fanquake commented on issue "test: intermittent issue in p2p_1p1c_network.py":
(https://github.com/bitcoin/bitcoin/issues/33318#issuecomment-3503644320)
Seen again in https://github.com/bitcoin/bitcoin/actions/runs/19173508101/job/54812101221?pr=33810.

💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3503646384)
> Sorry, but one more rebase is needed.

I think I've rebased correctly here -- I removed an additional `SKIP_BUILD_RPATH OFF` override that seemed like it would no longer be needed, https://github.com/bitcoin/bitcoin/commit/a02282d20c87471a3399e4061c7edad7ecdb391f#diff-2a07660db8ebdbbf967cf42f6b4da3ffc8558a322f45a3a08ce292c9e91767b8L13

Guix build for a02282d20c87:
a3647a46e00124b207263b695d9598cf9fe6c845ea9072c933462dd0e31a0ed9 guix-build-a02282d20c87/output/aarch64-linux-gnu/SHA256SUMS
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3503650032)
It's ready for testing with:
- https://github.com/stratum-mining/sv2-tp/pull/59

I also pushed a small change that switches `ExtractCoinbaseTemplate` to take `CTransactionRef coinbase_tx` as its argument rather than a `CBlockTemplate`. It doesn't really make a difference here, but lets me copy-paste it to `sv2-tp` which doesn't have `CBlockTemplate`.

💬 fanquake commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3503682477)
The TSAN failure is #33318.

📝 fanquake opened a pull request: "guix: build glibc with `--enable-static-pie`"
(https://github.com/bitcoin/bitcoin/pull/33821)
From the glibc `2.27` release notes:

> Major new features:
> The GNU C Library can now be compiled with support for building static
> PIE executables (See --enable-static-pie in INSTALL).
> These static PIE executables are like static executables but can be
> loaded at any address and provide additional security hardening benefits
> at the cost of some memory and performance. When the library is built with
> --enable-static-pic the resulting libc.a is usable with GCC 8 and above to
>
...

💬 l0rinc commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#issuecomment-3503719786)
Thanks @average-gary.
Would it be possible to group the commits so that they're telling a story, so that the commit messages explain the "why", not the "what". The "what" is already in the code. The commit messages seem LLM generated, they don't add a lot of value this way.
The first commit still adds a test that claims we're testing the witness when in fact it's empty: [`a4376ab` (#32840)](https://github.com/bitcoin/bitcoin/pull/32840/commits/a4376abd72aae5c0ea158db66440618cb1abfe5a#r24921793
...

💬 purpleKarrot commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503730840)
> I think there is no explanation in the code, but the reason why the copies/symlinks are created is that it keeps the source dir clean of `.pyc` files.

The generation of `.pyc` files can be suppressed with python's `-B` option or `PYTHONDONTWRITEBYTECODE` env var.
See: https://docs.python.org/3/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE

💬 Sjors commented on issue "Header-only support for waitNext()":
(https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3503736237)
>> I'd like to expand the `waitNext()` Mining IPC function to optionally (via BlockWaitOptions) return an (empty) new template as soon as we have a header with sufficient proof-of-work, pending block download and/or validation.

> Why? you should expand on the motivations behind this and what exactly you aim to achieve with this feature.

To allow miners to start hashing on an empty block template as soon as we see a new header, rather than waiting for the block to be fully downloaded and valida
...

💬 ismaelsadeeq commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504658204)
> In the longer run it probably makes sense for sv2-tp to have one major release of sv2-tp for every major Bitcoin Core release, but current it doesn't have backports. Each release is a combination of bug fixes and new features.


Hmm, fwiw I am approach NACK changes that introduce conflicting interface method that deprecate and don't prune on experimental interface that did not offer interface stability for the reason discussed above.
However I am open minded to convincing otherwise and cur
...

📝 yuvicc opened a pull request: "[kernel] Add block header support and validation"
(https://github.com/bitcoin/bitcoin/pull/33822)
Adds a new `btck_BlockHeader` type and associated functions to create, access, and validate block headers. Block headers will have their own type (`btck_BlockHeader`) that can be created from raw data, copied, and queried for all the standard header fields (hash, prev hash, timestamp, bits, version, nonce). We can also extract headers from full blocks or block tree entries.

#### New Block Header API

- **`btck_BlockHeader` type**: Opaque handle for block headers
- **Header methods**:
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504684591)
The old methods are redundant, but they're not conflicting. Clients can use either and both have test coverage.

From this projects perspective there's hardly any additional review burden when we prune deprecated methods in a later PR, that should be trivial.

💬 maflcko commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503780491)
I know, but `-B` does not help to fix the other problems: (1) They are not run via ctest, and (2) they are stand-alone scripts, where the user/dev would have to specify it every time, or set the env var.

💬 fanquake commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3503803553)
https://github.com/bitcoin/bitcoin/actions/runs/19175802541/job/54820162776?pr=33822#step:5:179:
```bash
<snip>
src/kernel/bitcoinkernel.h:1726: *
src/kernel/bitcoinkernel_wrapper.h:759:
src/kernel/bitcoinkernel_wrapper.h:762:
^^^
Trailing whitespace (including Windows line endings [CR LF]) is problematic
```