💬 fjahr commented on issue "interface_ipc functional test failing in CI":
(https://github.com/bitcoin/bitcoin/issues/33884#issuecomment-3539397600)
I have opened #33880 to address this.

💬 starius commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-3539405058)
> Are you still working on this?

Yes. I'm addressing all the feedback. I'll publish new version soon.

✅ pinheadmz closed an issue: "Improve Bitcoin’s resilience to large-scale power grid failures and Carrington-type solar storms"
(https://github.com/bitcoin/bitcoin/issues/33885)

💬 pinheadmz commented on issue "Improve Bitcoin’s resilience to large-scale power grid failures and Carrington-type solar storms":
(https://github.com/bitcoin/bitcoin/issues/33885#issuecomment-3539414510)
This should be posted on the [bitcoin-dev mailing list](https://groups.google.com/g/bitcoindev), the [Delving Bitcoin forum](https://delvingbitcoin.org/) or some other platform where broad, protocol-level concepts are discussed. Conceptual questions and most usage questions can be posted on [Stack Exchange](https://bitcoin.stackexchange.com/). The Bitcoin Core issue tracker is reserved for discussion about this specific software project only, its implementation and usage.

💬 fjahr commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532296702)
One might say that "NO" is _literally_ the inversion of "ON" 😜

Changed in the latest push :)

💬 fjahr commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532296866)
Done

💬 fjahr commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532299269)
Missed the bit about the commit message initially, now also addressed in #33878

💬 fjahr commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3539450021)
This is now in sync with #33026 and https://github.com/bitcoin/bitcoin/pull/33878 again so technically ready for review but of course the other PRs should be checked out first.

👋 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