🤔 LarryRuane reviewed a pull request: "test: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL"
(https://github.com/bitcoin/bitcoin/pull/33021#pullrequestreview-3056300082)
tACK b2d82668a57d4ab4eef310c25a2aebda18563677
I thoroughly reviewed and understand all code changes. This test perfectly tests `GetCoinsCacheSizeState()` and is much simpler and less fragile. (Also, I learned a few C++ tricks, very nice.)

I ran the modified test using the debugger (on Ubuntu), and it behaves as designed. The only very minor suggestion if you need to retouch is to consider adding a comment explaining why it's not necessary to verify that the loops didn't exceed `MAX_ATTEMPTS`
...

💬 glozow commented on pull request "p2p: stop special-casing witness-stripped error for unconfirmed transactions":
(https://github.com/bitcoin/bitcoin/pull/32379#issuecomment-3119690432)
I ran the above patch for a while and added logging to collect some stats and see how much more redownloading we’d do. TLDR it seems like we can definitely go ahead with it.

Caveats: Traffic has been very low so I set my mempool expiry to 1 hour to try to induce more orphan-fetching. I’m a default policy node so I don’t really expect to be relayed many transactions I’ll reject. There isn’t much opportunity to test reconsiderable rejections right now because of how low transaction traffic is.
...

💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2231703938)
This is better, will implement.

💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2231710505)
Will implement, the `ScopedScheduler` is better imo and `std::shared_ptr` makes more sense over `std::unique_ptr`.

> With a shared pointer, we can safely and explicitly manage these dependencies without introducing coupling between the logger and the scheduler.

I think there will always be coupling between the two since `Reset` may call `LogPrintLevel_`?

👋 murchandamus's pull request is ready for review: "Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060)

💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3119906723)
I cherry picked https://github.com/monlovesmango/bitcoin/commit/174827285797794928cd87732e2587fea70c0157 and fixed some of the incorrect comments.

> [doc: Fixes GetChildrenFromSamePeer comments](https://github.com/monlovesmango/bitcoin/commit/2a25c28aec3773bdf9a17148e45ce38ee95a7342) - Updates comments to reflect that non-reconsiderable announcements are sorted before reconsiderable announcements.

💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2231808946)
They can be imported with `importdescriptors`.

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2231837660)
Why is it safe to assume that the transaction doesn't already have a child here?

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231847269)
added these and `serialize.h`, hope that'll do. (never got iwyu running locally and the tidy job didn't cover the new file for some reason).

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231849239)
yes, I think the part with the "three items" is still correct, it talks about the values of the map.
I replaced the part about the key from blockfilterindex.cpp by a reference to db_key.h

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231851121)
Done - I added a comment to the file, using parts from `blockfilterindex.cpp`.
I agree that `db_key.h` is nicer, changed it to that.

💬 mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3120065955)
[03b67a2](https://github.com/bitcoin/bitcoin/commit/03b67a27d013e55891df69b1aef9425574b88f5e) to [936bd87](https://github.com/bitcoin/bitcoin/commit/936bd874ac1581de532317741f5ab26e875935b3):
Addressed feedback by @fjahr.

💬 achow101 commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3120080986)
ACK c5c1960f9350d6315cadbdc95fface5f85f25806

💬 achow101 commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3120084517)
ACK d89c6fa4a71810cdb28395d4609632e1b22249b3

💬 macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231890541)
Shouldn't the coin_type be different for regtest/signet? /352h/1h instead of /352h/0h

🤔 macgyver13 reviewed a pull request: "Silent Payments: Receiving"
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3056616801)
working on backup / restore testing I noticed a few issues.

💬 macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231883781)
I think this change is breaking the ability to import sp descriptor with private keys defined for scan and spend
This is the warning I receive when using importdescriptors:
"Not all private keys provided. Some wallet functionality may return unexpected errors"

🚀 achow101 merged a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845)

💬 jonatack commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2231906796)
- For users who don't follow changes closely, perhaps mention when legacy wallets became no longer supported
- Perhaps briefly remind users how to migrate legacy wallets to descriptor ones

🚀 achow101 merged a pull request: "wallet: Remove `upgradewallet` RPC"
(https://github.com/bitcoin/bitcoin/pull/32944)