💬 gmaxwell commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2959803519)
> this change was made surreptitiously

It's described in the first line of the PR, and its the whole subject of the second paragraph along with the reason it works that way.

No one, including you, has raised any specific issue with it.

🤔 danielabrozzoni reviewed a pull request: "tests: Remove hardcoded addresstype in `rpc_psbt.py`"
(https://github.com/bitcoin/bitcoin/pull/32701#pullrequestreview-2914228840)
I tried reviewing this PR, but I'm a bit confused about what's going on, and I'm not sure what's the best path forward :) My two sats:

As I understand it, setting addresstype enforces a specific address format. If it's not set, the default is used. Currently, the default is bech32, so removing the config option has no effect:

https://github.com/bitcoin/bitcoin/blob/c1d4253d316ea627f46b26e395d7b48842258381/src/wallet/wallet.h#L149)

While reviewing this PR, I expected that removing the ad
...

💬 average-gary commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2959839585)
Concept ACK.
I find CTV desirable for [condensing mining pool coinbase payouts](https://delvingbitcoin.org/t/scaling-noncustodial-mining-payouts-with-ctv/1753) to many self-custodial addresses without consuming additional block space or encountering firmware limits on mining devices.

💬 hebasto commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-2959870313)
cc @vasild

🤔 Prabhat1308 reviewed a pull request: "wallet, test: best block locator matches scan state follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32580#pullrequestreview-2914299373)
ACK [`3112fb5`](https://github.com/bitcoin/bitcoin/pull/32580/commits/3112fb5c5bdc66b520c384e590ec99ff655daa21)

🚀 fanquake merged a pull request: "doc: make `-DWITH_ZMQ=ON` explicit on `build-unix.md`"
(https://github.com/bitcoin/bitcoin/pull/32696)

💬 fanquake commented on pull request "doc: make `-DWITH_ZMQ=ON` explicit on `build-unix.md`":
(https://github.com/bitcoin/bitcoin/pull/32696#issuecomment-2959926997)
Backported to 29.x in #32589.

💬 Crypt-iQ commented on pull request "p2p: Add witness mutation check inside FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#issuecomment-2959931009)
ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1

🤔 jonatack reviewed a pull request: "init, doc: Replace datacarrier(size) deprecation with non-recommendation text"
(https://github.com/bitcoin/bitcoin/pull/32714#pullrequestreview-2914319980)
Concept ACK, modulo the following

> I think we should either (a) recommend the option not be used and mark it deprecated; or (b) not mark it deprecated, and leave it up to the user to decide whether to use the option, without making any particular recommendation; and not try (c) some other option to split the non-existent difference. If it were just up to me, I'd choose (b) over (a), but it's not a terribly strong opinion.

Agree with @ajtowns.

💬 jonatack commented on pull request "init, doc: Replace datacarrier(size) deprecation with non-recommendation text":
(https://github.com/bitcoin/bitcoin/pull/32714#discussion_r2138338000)
Prefer dropping the extra recommendation sentence for each of these.

```suggestion
argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u).", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
```

(if not, grammar nit for each of these: move the default before the recommendation sentence, or drop the period "." preceding the default.)

💬 maflcko commented on pull request "wallet, refactor: Remove Legacy wallet unused warnings and errors":
(https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2138353475)
there is now dead code in line 401: ` if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) {
`

📝 hebasto opened a pull request: "doc: Update Qt 6 packages on FreeBSD"
(https://github.com/bitcoin/bitcoin/pull/32717)
See:
- https://cgit.freebsd.org/ports/tree/devel/qt6-base
- https://cgit.freebsd.org/ports/tree/devel/qt6-translations

💬 hebasto commented on pull request "doc: Update Qt 6 packages on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/32717#issuecomment-2959980069)
cc @vasild

💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138165793)
> Nit: There is no reason to declare this (and CBlockUndo) outside the if statement, right?

There’s actually a slightly important reason for that. If the object is declared inside the if-block, it will be destructed when going out of scope. This would leave null pointers in the `BlockInfo` struct that we provide to child classes.

💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138224678)
> In commit "test: indexes, avoid creating threads when sync runs synchronously" ([331a25c](https://github.com/bitcoin/bitcoin/commit/331a25cb16632042dd6782a9b62fcc5c8aa6da3b))
>
> Motivation for this commit seems unclear. It seems to initialize indexes in unit tests differently than they are initialized in the node and reduce test coverage a little. Maybe commit message can be updated to mention reasons for wanting to do this?


Having unit tests waiting for secondary thread creation seem
...

💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138145261)
> Should be ProcessBlock(pindex, block.get()) to avoid rereading the block from disk?

upps, yes!

💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138167718)
> In commit "index: remove CBlockIndex access from CustomAppend()" ([3f390ef](https://github.com/bitcoin/bitcoin/commit/3f390efa313f4ac43e9134d5299d7faec74cca60))
>
> It's a bit inefficient to copy the undo data here, and the code seems overcomplicated because the `block_undo` variable is only referenced once, 60 lines below. Would suggest dropping the `block_undo` variable and just accessing `block.undo_data` directly below.
>
> The `block_undo` variable in BlockFilterIndex::CustomAppend
...

💬 furszy commented on pull request "index: move disk read lookups to base class":
(https://github.com/bitcoin/bitcoin/pull/32694#discussion_r2138367736)
Done as suggested.

💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2138368309)
I'll look into this for a follow-up. `InvalidChainFound` log inconditionally, so just moving it into the loop could be too verbose when disconnecting thousands of blocks.

💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2138368441)
Will add one in a follow-up if I don't need to push.