💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255448800)
There was no reordering, it was just inserted after the other helper, `IsPayToWitnessScriptHash` (looks like GitHub got confused).
The `BOOST_CHECK_EQUAL` to `BOOST_CHECK` change is just to test the new helper method in a simpler way, now that we don't have to use `Solver`.
You still think it needs additional explanations?

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255352640)
Done, thanks, kept the rename and added you as a co-author

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255358039)
Added your explanation to the commit message - even though the changes themselves made that obvious, since we've added new comments to the methods

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255644392)
I played with this, but really dislike the new version, mostly for how awkward `CheckSigopsBIP54` has become after it.
But I have kept your comment update and change the ternary, let me know what you think of this instead:
```C++
if (prev_txo.scriptPubKey.IsPayToScriptHash()) {
Assert(prev_txo.scriptPubKey.CountSigOps(/*fAccurate=*/true) == 0);
sigops += CountP2SHSigOps(txin.scriptSig);
} else {
sigops += prev_txo.scriptPubKey.CountSigOps(/*fAccurate=*/true);
}
```

If we
...

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255456847)
In most other cases I used constants, but for the sizes I prefer redundancy and clarity, to make absolutely sure we're not accidentally changing anything here

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255457272)
I prefer the constant here for redundancy

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255462973)
Yes, same reasoning as before - normally I'm all for deduplication, so that the reason is clear + if we change it, we don't have to touch multiple places. But here I want a bit of extra redundancy to make sure that we **do** have to change in multiple places if we **really** want to change this value.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255465558)
We kinda' depend on pubkey here - if you have a more concrete suggestion, please let me know, all other ones I thought of were uglier than this.

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