💬 stickies-v commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258586027)
nit: would `SetNotificationArgs` be a better name to clarify the side effects this function has?

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1258613252)
Done where the code was more than just moved.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1258613989)
Perhaps, but I think we should test it in this test as it's important to have the upgrade and downgrade tests for the older wallet version.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1258614077)
Done

👍 ryanofsky approved a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044#pullrequestreview-1522637462)
Code review ACK 4b405318d4c476351cccc30588f9126edd94cc35

💬 ryanofsky commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#discussion_r1258649315)
In commit "test: indexes, fix on error infinite loop" (4b405318d4c476351cccc30588f9126edd94cc35)

I found "As index sync failures" comment difficult to understand because it was hard to see how it directly related to code. Would suggest something more literal like "Assert shutdown was not requested to abort the test, instead of looping forever, in case there was an unexpected error in the index that caused it to stop syncing and request a shutdown"

💬 ryanofsky commented on pull request "wallet: address book migration bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28038#discussion_r1258636424)
This looks ok, but I think would have been more straightforward to write as:

```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 8fa93b97d655..ac6520e95a29 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4059,10 +4059,9 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
WalletBatch batch{wallet.GetDatabase()};
for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
auto
...

🤔 ryanofsky reviewed a pull request: "wallet: address book migration bug fixes"
(https://github.com/bitcoin/bitcoin/pull/28038#pullrequestreview-1522617977)
Code review ACK 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9

👍 ryanofsky approved a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053#pullrequestreview-1522654951)
Code review ACK 5867bb1ae7d1d345a5fd3b341d88ff777b6ef404. This looks good, and now can be rebased and simplified because #27607 is merged

💬 dsldsl commented on issue "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin":
(https://github.com/bitcoin/bitcoin/issues/28049#issuecomment-1629472415)
I guess the answer is that its not a known false positive... so likely was an actual issue. hm.

unfortunately i deleted the files so can't confirm the checksums.

The virus scanner is https://www.intego.com/virusbarrier-scanner

💬 furszy commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#discussion_r1258681867)
hmm ok.
What I meant with the "As index sync failures" comment is that all index sync errors trigger a shutdown.

💬 furszy commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#issuecomment-1629484952)
Updated comment per feedback.

💬 instagibbs commented on pull request "Bugfix: net_processing: Restore "Already requested" error for FetchBlock":
(https://github.com/bitcoin/bitcoin/pull/28055#discussion_r1258722968)
```suggestion
self.log.info("But every fetch to another peer causes us to forget previous attempts for same block")
self.nodes[0].add_p2p_connection(P2PInterface())
peers = self.nodes[0].getpeerinfo()
assert_equal(len(peers), 4)
slow_peer_id_2 = peers[3]["id"]
assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id_2), {})
assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id), {})

```

🤔 instagibbs reviewed a pull request: "Bugfix: net_processing: Restore "Already requested" error for FetchBlock"
(https://github.com/bitcoin/bitcoin/pull/28055#pullrequestreview-1522752824)
The result of this PR would be that it would return an error IFF that particular block was requested by that specific peer last. If you request the block from another peer, the behavior would be unchanged. Is that acceptable?

💬 instagibbs commented on pull request "Bugfix: net_processing: Restore "Already requested" error for FetchBlock":
(https://github.com/bitcoin/bitcoin/pull/28055#discussion_r1258724942)
example test covering the behavior I'm mentioning

💬 theuni commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1629581578)
See [here](https://github.com/theuni/bitcoin-core-clang-plugin/commit/05fb23f21df32df4445951123d281172b12ee61b) for an example of this implemented as a `clang` plugin rather than `clang-tidy`. As noted above, it's pretty rough because LLVM does not yet expose several useful helpers.

Advantages:
- We can invent our own attributes, which is way nicer for future-proofing. See [here](https://github.com/theuni/bitcoin-tidy-experiments/pull/1) for an example of the kind of synchronization problems
...

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629627284)
Hey there, is this PR ready to merge if there aren't any errors?

💬 brunoerg commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1258853476)
```suggestion
if (!SanityCheckASMap(asmap, 128)) return FuzzResult::UNINTERESTING;
```

💬 sipa commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629690355)
People will need to review it.

👋 achow101's pull request is ready for review: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286)