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

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