💬 Sjors commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3540715551)
Good catch. The test needs to check two things:

1. The IPC node actually updates it own chain after `submitBlock()`
2. The other node accepts the block

So the check for `current_block_height + 1` is important, but we should either do it for `self.nodes[0]` (no need to `wait_until`) _or_ keep it for `self.nodes[1]`, but after `sync_all`.

Only relying on `sync_all` is not enough, because it won't catch a regression where the IPC code fails to update the node itself, so there's no block t
...

💬 cedwies commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3540768449)
utACK d442598

👍 TheCharlatan approved a pull request: "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`"
(https://github.com/bitcoin/bitcoin/pull/31823#pullrequestreview-3471932363)
ACK a7c96f874de13ace9814da92fd6160a126a97ebf

💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3540923567)
Also tested on aarch64 with Debian 13.x.

👍 TheCharlatan approved a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3471964192)
ACK 96963b888e5a10f4024fa0449c60c02e3bed6245

💬 fanquake commented on pull request "doc: Update NetBSD Build Guide":
(https://github.com/bitcoin/bitcoin/pull/33876#issuecomment-3540993627)
ACK c29eaeeaf9377b1e3c33e82d29597790a5388403

🚀 fanquake merged a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/33876)

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541021155)
@plebhash wrote:

> however, we always need to call getBlock specifically because it contains the header + txdata of the template... and as a byproduct, we happen to also get the Coinbase Tx data, which saves us one extra call to getCoinbase/getCoinbaseTx

Regarding the header, there's a separate `getHeader()` method.

It's true that you need txdata anyway, but you don't need it immediately. You need it when the pool asks using the `RequestTransactionData` message.

When the node and the
...

🚀 fanquake merged a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537)

🤔 janb84 reviewed a pull request: "kernel: Expose `CheckTransaction` consensus validation function"
(https://github.com/bitcoin/bitcoin/pull/33796#pullrequestreview-3472013748)
Concept ACK a262282abefc0a16be329ab2179c193b4b32cc5b

PR extends bitcoin kernel library with the checkTransaction consensus validation function.

Added a suggestion NIT (non-blocking) and I agree with the NIT from [yuvicc](https://github.com/bitcoin/bitcoin/pull/33796/files#r2506321358)

Steps taken:
- Build & tested
- Code review
- Create a implementation PR for [BitcoinKernel.Net](https://github.com/janb84/BitcoinKernel.NET/pull/2)

💬 janb84 commented on pull request "kernel: Expose `CheckTransaction` consensus validation function":
(https://github.com/bitcoin/bitcoin/pull/33796#discussion_r2533510544)
NIT: (non-blocking) maybe add a note why only the 2 (possible) states are tested ? For a non-bitcoin-core dev this is maybe a bit unclear given all the TxValidationResults and the Check_Transaction that these are the 2 outcomes.
It's for me not logical to add this as a comment on the function because it returns a pointer to the results.

```suggestion
// -----------------------------------------------------------------------------
// Note: CheckTransaction performs only basic consensus ch
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533559315)
I think it's good to have `parse_and_deserialize_coinbase` be safe to use, because we might eventually move it into the test framework as IPC coverage increases. I'll add some documentation.

💬 fanquake commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2533590975)
```bash
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-cli
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-qt
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-tx
/distsrc-base/distsrc-275349011d15-x86_64-linux-gnu/installed/bitcoin-275349011d15/bin/bitcoin-ut
...

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533615466)
I don't think miners use the full `scriptSig` so if we slightly go over it, it shouldn't immediately result in invalid blocks. So it seems overkill to abort the template generation entirely.

I'll log a warning.

The main purpose of this `Assume` is to remind ourselves to inform the mining ecosystem if we ever expand the scriptSig prefix substantially.

🚀 fanquake merged a pull request: "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247)

👋 fanquake's pull request is ready for review: "depends: update xcb-util packages to latest versions"
(https://github.com/bitcoin/bitcoin/pull/33851)

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533642433)
I also clarified the comment that it might be a _silently_ breaking change. Although the spec requires `NewTemplate` to keep it at max 8 bytes, the serialisation supports up to 256 and a Template Provider implementation might just not check / enforce this.

https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533657576)
I'll go for the latter approach.

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2533690075)
The BIP says:

> For backward compatibility, the `Hash(new commitment|witness reserved value)` will go to the coinbase witness, and the witness reserved value will be recorded in another location specified by the future softfork. Any number of new commitment could be added in this way.

So we provide clients with the coinbase witness, which is not necessarily the "witness reserved value".

💬 waketraindev commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533700190)
Went with separate counters and also added the selected network to the log output for clarity.
Example log entry:
```
[addrman] GetAddr returned 62823 random addresses from ipv4; 1104 filtered (0 network, 0 quality, 1104 policy)
[addrman] GetAddr returned 0 random addresses from ipv6; 63927 filtered (63927 network, 0 quality, 0 policy)
[addrman] GetAddr returned 0 random addresses from onion; 63927 filtered (66927 network, 0 quality, 0 policy)
```