💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255459207)
Yes, good point, already did that as part of the other similar comment you had.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255500828)
Even better, I used `CKey dummyKey{GenerateRandomKey(/*compressed=*/true)}`

💬 l0rinc commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3157260359)
ACK c83741e4033824b4c798fc3e9cdc33cb30bbaf32

please fix the typo in the PR subject: locater -> locator

💬 pablomartin4btc commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2255780830)
nit:
```suggestion
"Creates a wallet file at the specified destination containing a watchonly version "
"of the current wallet. This watchonly wallet contains the wallet's public descriptors, "
"its transactions, and address book data. The watchonly wallet can be imported into "
"another node using 'restorewallet'.",
```

🤔 pablomartin4btc reviewed a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3090320231)
re-ACK 84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4

Since my last review `CanSelfExpand()` has been refactored as @ryanofsky [suggested](https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2246172625).

We'd need release notes for this new RPC.

👋 waketraindev's pull request is ready for review: "qt: add shift key modifier to clear command history when clearing the console"
(https://github.com/bitcoin-core/gui/pull/882)

💬 Sjors commented on pull request "wallet: prevent accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2256045327)
That said, I'm not a huge fan of this much string parsing logic either.

📝 maflcko opened a pull request: "test: Avoid shutdown race in NetworkThread"
(https://github.com/bitcoin/bitcoin/pull/33140)
Locally, I am seeing rare intermittent exceptions in the network thread:

```
stderr:
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 744, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 603, in run_forever
self._run_once()
File "/python3.10/asyncio/base_events.py", line 1871, in _run_once
e
...

📝 maflcko opened a pull request: "test: Remove polling loop from test_runner (take 2)"
(https://github.com/bitcoin/bitcoin/pull/33141)
(This picks up my prior attempt from https://github.com/bitcoin/bitcoin/pull/13384)

Currently, the test_runner is using a `time.sleep` before polling to check if any tests have completed. This is largely fine when running a few tests, or when the tests take a long time.

However, when running many fast tests, this can accumulate and leave the CPU idle for no reason.

A trivial improvement would be to only sleep when really needed:

```diff
diff --git a/test/functional/test_runner.py b/
...

💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2256280041)
not sure about implicit copying and having to maintain a list of stuff to ignore. it would be better to explicitly copy just the git-tracked files.

💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2256345063)
I don't think it makes sense to enumerate them or even mention them. It should simply use the native arch. This can be achieved by just passing `--platform=linux`, like in the CI system

📝 maflcko opened a pull request: "test: Run bench sanity checks in parallel with functional tests"
(https://github.com/bitcoin/bitcoin/pull/33142)
The ctest target `bench_sanity_check` has many issues:

* With sanitizers enabled, it is one of the slowest targets, often taking several minutes. See https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-2984264066.
* All benchmarks are run sequentially, when they could run in parallel instead.

Both issues can lead to CI timeouts and leave CPU unused during testing.

Fix all issues by running it as part of the functional tests instead. This is similar to the rpcauth tests (https:
...

💬 maflcko commented on pull request "ci: Run unit tests parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33000#issuecomment-3158236096)
> Concept ACK. Not sure about the approach of more Bash, to call new Python, that wraps more Bash.

Yeah, it is ugly. I think I'll drop some of the (actually redundant) Bash in https://github.com/bitcoin/bitcoin/pull/33136 first. Then, this can be done in pure Python, possibly.

💬 fanquake commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3158289885)
Concept NACK. The benefits of this are unclear, this seems to just add a new dependency/abstraction layer (for development?), without solving any specific problem. It's also trying to do three things at once, and there already exist maintained Dockerfiles, for the deployment scenario, i.e https://github.com/willcl-ark/bitcoin-core-docker.

> developers can edit the Dockerfile or mount their source with custom flags.

This is more work than just using CMake and setting options?

> Provide a
...

💬 maflcko commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3158322672)
> Ah, I guess on my machine, these jobs are the slowest:
>
> ```
> 143/144 Test #4: secp256k1_tests ...................... Passed 217.55 sec
> 144/144 Test #6: bench_sanity_check ................... Passed 433.08 sec
> ```
>
> In the ci environment, we see this:
>
> ```
> [17:30:48.523] 143/144 Test #4: secp256k1_tests ...................... Passed 1134.59 sec
> [17:51:54.170] 144/144 Test #6: bench_sanity_check ...................***Timeout 2400.23 sec
> ```

I
...

💬 maflcko commented on issue "Slow unit tests delay functional tests and leave CPU unused":
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3158330108)
`bench_sanity_check` could make sense to split up as well. With sanitizers enabled, it takes several minutes, even on fast hardware. See https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3146639801. So I created https://github.com/bitcoin/bitcoin/pull/33142

💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3158495632)
> > developers can edit the Dockerfile or mount their source with custom flags.
>
> This is more work than just using CMake and setting options?

Yeah, for devs it could make sense to have an image with just the build deps, similar to the lint ci image:

```
DOCKER_BUILDKIT=1 docker build -t bitcoin-core-build-deps --file "./contrib/dev-build-deps_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-core-build-deps
... then run cmake, etc...
```

However, I am not sure if
...

🤔 fanquake reviewed a pull request: "ci: Pass CI_FAILFAST_TEST_LEAVE_DANGLING into container"
(https://github.com/bitcoin/bitcoin/pull/33138#pullrequestreview-3091670370)
ACK fa1d2f63803e71d21b470cf4eb52fbe941aa0b28

🚀 fanquake merged a pull request: "ci: Pass CI_FAILFAST_TEST_LEAVE_DANGLING into container"
(https://github.com/bitcoin/bitcoin/pull/33138)

🤔 fanquake reviewed a pull request: "cmake: Switch to generated `ts_files.cmake` file"
(https://github.com/bitcoin/bitcoin/pull/33115#pullrequestreview-3091739994)
ACK a26fbee38f95e71fbbeb6cf09e18ed7fff089ec8