🤔 maflcko reviewed a pull request: "test: enabling wallet migration functional test on windows"
(https://github.com/bitcoin/bitcoin/pull/32219#pullrequestreview-2959123050)
left some nits only

review ACK f4e259b31f5785190c4912975173c5025dfe349c 🚿

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment:
...

💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167386668)
f4e259b31f5785190c4912975173c5025dfe349c nit: Would be nice to use the full option name instead of `-t`

💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167379833)
nit in the same commit: Could just use `print(f'\r...',flush=True,end='')

💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167311981)
nit in ee3d4e4f4940aa1468a65c4bf111ae3155aa227e: For new code, use snake_case: `archive_url` in those three lines.

💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167330301)
nit in the same commit: Can just use `print(flush=True)`? https://docs.python.org/3/library/functions.html#print

💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2167286122)
unrelated nit in 6f916050a9fa9e9fa1b226f1b2d681f8f3e21869: I don't like short option names in scripts, because I never know what they mean. Is this `-t` for `--tags`, ...?

Would be nice to use the correct long form here, while touching this line.

✅ luke-jr closed a pull request: "Bugfix: Wallet: Skip inaccessible directories rather than abort the wallet list entirely"
(https://github.com/bitcoin/bitcoin/pull/32812)

💬 luke-jr commented on pull request "Bugfix: Wallet: Skip inaccessible directories rather than abort the wallet list entirely":
(https://github.com/bitcoin/bitcoin/pull/32812#issuecomment-3005807781)
This actually isn't necessary. Unreadable subdirectories get skipped explicitly when checking for their wallet.dat fails.

🤔 glozow reviewed a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2959312112)
utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6

💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3005831521)
> Comparing this latest output against my previous testing I noticed that the all_networks element has been removed (?)

good observation! the old version was incorrect because all_networks and total were the same thing (total number of addresses from all networks in addrman). pushed an [update](https://github.com/bitcoin/bitcoin/compare/c2f3225622cbbe516ac23059bff47ed3c28653cf..016ab85a13408ad980c3dbce4e041e14a4fcf3b8) which doesn't require us to sum up total number of addresses again since t
...

💬 stratospher commented on pull request "test: refactor out same-txid-diff-wtxid tx to reuse in other tests":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2167407115)
ok agree the terms are confusing - I liked [this explanation](https://bitcoin.stackexchange.com/a/119382) of different possible forms of malleability.

thinking of doing s/`build_malleated_children`/`build_children_tx_pair` and s/`ValidWitnessMalleatedTx`/`DifferentWtxidTx` in next push unless anyone has better suggestions!

💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2167236137)
Thanks I'll take this suggestion. My only concern is that technically there could be a future regression where a user setting timeout to 1 second ends up lasting 60 seconds... I'll add `timeout=5` for a reasonable clamp.

🤔 pinheadmz reviewed a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2958751819)
Rebase to include suggestions from @fjahr and @vasild thanks!!

💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2167221436)
Thanks I'll remove reference to libevent (thats the whole point anyway!) and explain why there is a margin of error in the test

💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2167060793)
👍

💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2167216427)
Good feedback I think you're right, its cleaner to just store a reference to the scheduler here. It requires some weird pointer / `any` juggling in`StartHTTPRPC()` -- to keep the `std::any` abstraction

💬 Zeegaths commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2167432256)
if i run this its goes to version 30, which does not exist yet..,

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3005901522)
> together staying just below the global announcement and usage limits, but massively exceeding the per-peer usage reservation

Whiteboarded in person. Hopefully this explanation is clear and explains it in a more permanent location. If this ends up being correct we can adapt the benchmark to be more realistic.

The attack is as follows, assuming a global announcement count of 24,000, and per-peer reservation limit of 404kWU:

1) Peer 0 sends a 260WU orphan
2) Peers 1 through 124 announce
...

💬 willcl-ark commented on pull request "doc: Add fetching single PRs from upstream to productivity.md":
(https://github.com/bitcoin/bitcoin/pull/32783#discussion_r2167461391)
Thanks, fixed in 45b1d39757668939b03b27401c324a938ef0cd8d

🤔 janb84 reviewed a pull request: "doc: Add fetching single PRs from upstream to productivity.md"
(https://github.com/bitcoin/bitcoin/pull/32783#pullrequestreview-2959405149)
re ACK 45b1d39757668939b03b27401c324a938ef0cd8d

Changes sinds last ACK:
- Small textual changes / fixed typo's