🤔 mzumsande reviewed a pull request: "p2p: rename GetAddresses -> GetAddressesUnsafe"
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3055853811)
Code review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41

🚀 fanquake merged a pull request: "p2p: rename GetAddresses -> GetAddressesUnsafe"
(https://github.com/bitcoin/bitcoin/pull/32994)

💬 fanquake commented on pull request "[29.x] final changes for v29.1rc1":
(https://github.com/bitcoin/bitcoin/pull/33056#issuecomment-3118510216)
ACK 4c2d285b706058ece7092d03f0e5ad1112c2097f

🚀 fanquake merged a pull request: "[29.x] final changes for v29.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33056)

💬 pinheadmz commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3118790215)
I'm unable to reproduce so far building master at 6cdc5a90cffe9ced4ec50d2028c9896d25a9cb6a


on Debian:

`cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang-16' -DCMAKE_CXX_COMPILER='clang++-16' -DCMAKE_CXX_FLAGS='-std=c++20' -DBUILD_GUI=OFF -DBUILD_TESTS=OFF -DSANITIZERS=thread && cmake --build ./bld-cmake --parallel $( nproc ) && TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1" ./bld-cmake/test/functional/wallet_multiwallet.py`

on
...

💬 olegrok commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3119155140)
> [@olegrok](https://github.com/olegrok) If you want, you can test [#33002](https://github.com/bitcoin/bitcoin/pull/33002)

It looks a bit tricky (however I can't suggest solution that will be better) but this fixes an issue.

💬 ishaanam commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#issuecomment-3119410214)
re-utACK ea17a9423fb431a86d36927b02d3624f654fd867

I used range_diff to compare to my previous review. The changes were just to address review comments and rebase.

📝 theStack opened a pull request: "rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes`"
(https://github.com/bitcoin/bitcoin/pull/33065)
This PR takes use of the `FormatAllOutputTypes` helper (introduced in PR #32432, commit 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640) to get rid of the remaining hardcoded output types in wallet RPC documentation [1]. Note that it can't be used in the [`createmultisig` RPC](https://github.com/bitcoin/bitcoin/blob/fc162299f0cc5b61bbb55736fe85e27b705f78cc/src/rpc/output_script.cpp#L100), as this one is only for pre-taproot output types and hence doesn't contain "bech32m" in the list.

[1] instances
...

🤔 jonatack reviewed a pull request: "ci: Run unit test parallel with functional tests"
(https://github.com/bitcoin/bitcoin/pull/33000#pullrequestreview-3056247871)
I don't know how much variance we typically see in CI run times, but light lgtm ACK fab25c6f5803ae8699f068d089f1c6e4f1387743

```
$ /test_runner.py -h
--valgrind run nodes under the valgrind memory error detector: expect at least a ~10x slowdown. valgrind 3.14 or later required. Does not apply to previous release
binaries.
--bash-cmd-extra-tests BASH_CMD_EXTRA_TESTS
Run an arbitrary string as Bash command in parallel to the fu
...

🤔 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.