🤔 pablomartin4btc reviewed a pull request: "test: check P2SH sigop count for coinbase tx"
(https://github.com/bitcoin/bitcoin/pull/32850#pullrequestreview-2980935709)
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3

(coinbase transactions don’t refer to a real previous output and thus cannot be spending P2SH outputs, making the sigop count irrelevant for them).

🤔 marcofleon reviewed a pull request: "fuzz: Make process_message(s) more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2980950526)
tACK fac673d434b4d32d8c44dcc50a3655ba4a1816de

Running the stability script, there's significantly less coverage differences with this PR. My only question is why does `ResetChainman` set mock time in `process_messages` but not in `process_message`? Didn't see an obvious reason, but I could be missing something.

💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181163397)
that was actually done on purpose. See https://leimao.github.io/blog/Python-Default-Argument-Mutable-Object/.

💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181167049)
ups, reverting. Thanks.

💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181173483)
Thanks, but I'm about a ~0 here. I don't really find that change better. I'll just leave the current code unless someone feels strongly about changing it

💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181177845)
This is actually just a leftover from debugging the issue in #26966.. sorry for that. Will just remove the line.

💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181170410)
> It might be good to send in the same startup_args to check_clean_start() further down in the loop?

I don't think so. All files are restored from the backup after deletion, so we know for sure the indexes are synced at that point (nothing changed).

💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3029743647)
Updated per feedback. Thanks @hodlinator

💬 furszy commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2181204274)
We are already doing the backup file existence check at the end of `migrate_and_get_rpc`, what if you include this extra check there too?

💬 w0xlt commented on pull request "wallet: Track no-longer-spendable TXOs separately":
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2181221787)
Why is it optional instead of false by default?

💬 pablomartin4btc commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2181674862)
nit: I haven't seen the format I'm proposing here but just to make it more clear if it makes sense...
```suggestion
self.log.info(
"Test that on a duplicate block disconnection event after unclean shutdown:\n"
" - wallet transactions are un-abandoned after temporarily invalidated blocks;\n"
" - wallet doesn't crash."
)
```

🤔 pablomartin4btc reviewed a pull request: "wallet, test: best block locator matches scan state follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32580#pullrequestreview-2981629314)
cr-ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b

Reviewed #30221 to verify the agreed follow-ups:
- In `wallet.cpp`:
- another opportunity to use `setLastBlockProcessedInMem` - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090917963
- `(!loc.IsNull())` - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2096279905
- In `wallet_reorgsrestore.py`:
- update test log - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091417700
- assert rescanning - http
...

👍 rkrux approved a pull request: "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs"
(https://github.com/bitcoin/bitcoin/pull/32618#pullrequestreview-2981864736)
ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055

💬 rkrux commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2181951390)
Got it. Generally how long do we wait to remove a field from the code after marking it deprecated?

💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3031079858)
> mock time

My bad and nice catch. Added it there as well.

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2182003768)
04e51ba1110e13598e0b11a35ce5abf4a1789f53: now that the wrapper binary is available, the release note could enumerate how to call these now: `bitcoin test`, etc.

👍 Sjors approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2981960982)
re-ACK 705791cd436f237fe9bbac2cf52d63ab4b2a41c7

Left a suggestion for the release note.

💬 maflcko commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2182072018)
Possible typos and grammar issues:

In feature_chain_tiebreaks.py: “Make sure than only the former connects” -> “Make sure that only the former connects” [‘than’ should be ‘that’]
In feature_chain_tiebreaks.py: “# Restart and check enough times to this to eventually fail if the logic is broken” -> “# Restart and check this enough times to eventually fail if the logic is broken” [‘to this’ is misplaced and hinders comprehension]

💬 maflcko commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3031197708)
Maybe turn into draft while CI is red and this is still a WIP?

💬 maflcko commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3031268246)
Just ensure enough random transactions have been created to reliably return a fee estimate in any run?