👋 fjahr's pull request is ready for review: "build: Embedded ASMap [3/3]: Build binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792)

💬 theStack commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3539520136)
Concept ACK

Looks like half of the `create_empty_fork` method moving slipped into the second commit?

💬 ajtowns commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3539952304)
> Deprecate `getCoinbaseTx()` in favor of a new method that provides a struct with everything clients need to construct a coinbase. This is safer than providing a raw dummy coinbase that clients then have to manipulate.

That seems backwards to me:

1. Constructing a coinbase requires getting various consensus requirements correct, and new requirements may be added in future, such as the nlocktime requirement proposed in bip54. Why not keep all this logic in the node / template generator?

...

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2532806401)
Correct.

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3540138513)
Thanks for the review @stringintech.
- Fixed some indentations and removed some extra comments to maintain consistency.
- Simplified `BlockValidationState` create method by @stringintech [comment](https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2532058549)

💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533079324)
Maybe separate would be clearer for diagnostics. Up to you though,.. if you think the current logging is clear enough then that's fine too.

💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2533096852)
> The BanMan methods used here take `m_banned_mutex` and never call back into `AddrMan`, so the lock order in this policy is currently `AddrMan::cs`>`BanMan::m_banned_mutex` with no reverse lock order in the codebase.
>
Thanks. Might be worth a comment somewhere in the code documenting this ordering requirement so someone does not accidentally introduce the reverse path in future ?

💬 frankomosh commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2533161038)
> yes but its better to roll up responsibility to the object that holds it? Can you point out exactly where maybe? so we can see? if not, this seems like a better approach to me.

I think rolling the witness into `CTxIn::ToString()` is fine only if you keep the old separate witness block in `CTransaction::ToString()` (or try adding a flag to suppress it). Right now you’ve deleted that block, so every existing log, test and user script would lose the witness section.

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3540669363)
@ajtowns the `CoinbaseTemplate` struct introduced by this PR closely matches the [NewTemplate message](https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client) in Stratum v2. The main difference is that the `coinbase_tx_outputs` field of `NewTemplate` uses an ad hoc serialisation that I don't think we should embrace, so I keep the outputs in a vector.

Note that the code in `ExtractCoinbaseTemplate()` was mostly copied from `sv2-tp`. So the mai
...

💬 Sjors commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#discussion_r2533275932)
Maybe just move this line below sync_all.

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