💬 Sjors commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2890004759)
Concept ACK on adding a `TemplateToJSON` helper, as well as the new alias for `BlockAssembler::Options`.

This breaks the multiprocess build somehow, cc @ryanofsky, e.g.

```
[06:00:12.877] /ci_container_base/src/interfaces/mining.h:54:34: note: unimplemented pure virtual method 'getCoinbaseMerklePath' in 'ProxyClient'
[06:00:12.877] 54 | virtual std::vector<uint256> getCoinbaseMerklePath() const = 0;
```

💬 Sjors commented on pull request "scripted-diff: test: remove 'descriptors=True' argument for `createwallet` calls":
(https://github.com/bitcoin/bitcoin/pull/32544#issuecomment-2890031213)
ACK 86de8c1668005304b2c630ca2ad4a8ca8e348e90

> The argument `descriptors` could also be removed from `createwallet` RPC.

Unfortunately not, as long as we support positional arguments. We can ignore it though.

⚠️ maflcko opened an issue: "intermittent issue in qt addressBookTests: (table_view->model()->rowCount()): [3 != 2] Loc: [./qt/test/addressbooktests.cpp(183)]"
(https://github.com/bitcoin/bitcoin/issues/32558)
Seen on the test-each-commit task, as well as on task `CentOS, depends, gui`: https://github.com/bitcoin/bitcoin/runs/42411027184

```
[12:40:35.901] ********* Finished testing of WalletTests *********
[12:40:35.901] ********* Start testing of AddressBookTests *********
[12:40:35.901] Config: Using QtTest library 6.7.3, Qt 6.7.3 (x86_64-little_endian-lp64 static debug build; by GCC 14.2.1 20250110 (Red Hat 14.2.1-7)), centos 10
[12:40:35.901] PASS : AddressBookTests::initTestCase()
[12:
...

💬 Sjors commented on pull request "qt: drop unused watch-only functionality":
(https://github.com/bitcoin/bitcoin/pull/32459#issuecomment-2890058758)
Rebased just in case. Renumbered columns in `overviewpage.ui`.

💬 maflcko commented on pull request "scripted-diff: test: remove 'descriptors=True' argument for `createwallet` calls":
(https://github.com/bitcoin/bitcoin/pull/32544#issuecomment-2890062931)
lgtm ACK 86de8c1668005304b2c630ca2ad4a8ca8e348e90

ci failure is obviously intermittent and unrelated

💬 laanwj commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout)":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890123116)
That backtrace is exactly what i'd expected. It's the parent process waiting for the forked child there.

💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2095198381)
i intentionally didn't do that, as that would change the semantics. It currently breaks on the first `:`, which would allow semicolons in the password.

💬 hebasto commented on issue "intermittent issue in qt addressBookTests: (table_view->model()->rowCount()): [3 != 2] Loc: [./qt/test/addressbooktests.cpp(183)]":
(https://github.com/bitcoin/bitcoin/issues/32558#issuecomment-2890196882)
A duplicate of https://github.com/bitcoin-core/gui/issues/874?

👍 laanwj approved a pull request: "wallet: Fix logging of wallet version"
(https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2850007943)
Code review ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
Logging the value immediately after reading it, makes it clear the right value is logged.

💬 laanwj commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-2890261145)
i wonder how many people are running bitcoin nodes on 32-bit systems these days. Is this still worth special casing?

💬 Sjors commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-2890264852)
utACK d316a75f1fb807f5384ec9d4ed0b8aad6df7ed5f

This causes the version to be missing from some of the "Please report this issue here" messages. But if someone is running a node, rather than some kernel based application, they can easily find its version when reporting the bug.

💬 laanwj commented on pull request "RFC: bench: replace embedded raw block with configurable block generator":
(https://github.com/bitcoin/bitcoin/pull/32554#issuecomment-2890272937)
Concept ACK

💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258063)
It exercises both `GetSigOpCount` and `GetLegacySigOpCount`, so I kept it.
But the new tests can indeed be renamed, thanks!

💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258155)
Indeed, added, thanks.

💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258344)
I have tried including `interpreter.h` before, but it results in ugly weird compiler mess.
But it seems we can move those constant over to `script.h` instead - done, added you as a co-author.

💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258456)
I took these definitions from `solver.cpp` - the point here is not necessarily to fully validate the internal structure of the script templates, but rather to check if it follows a general template's layout (opcode, followed by a version, followed by a hash, etc). Validating the existing versions is done in other places, it's not important for e.g. SigOp count. If you think I'm mistaken here, please let me know.

💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258535)
This wasn't checked in `MatchPayToPubkey` either.

💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2890294507)
Thanks for the observations @tapcrafter, addressed your concerns in the latest push, adding you as a co-author in one of the commits.

@maflcko, to clarify the effect on the overall behavior, I slimmed down the `DeserializeAndCheckBlockTest` similarly to https://github.com/bitcoin/bitcoin/pull/32457 and added the results to the PR's description.

---

I ran

```bash
cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release
cmake --build build -j$(nproc)
build/bin/bench_bitcoin -filte
...

🚀 fanquake merged a pull request: "scripted-diff: test: remove 'descriptors=True' argument for `createwallet` calls"
(https://github.com/bitcoin/bitcoin/pull/32544)

💬 Fakruradzi commented on something "":
(https://github.com/bitcoin/bitcoin/commit/7710a31f0cb69a04529f39840196826d0b9770ab#r157354157)
Ok!