🚀 fanquake merged a pull request: "ci: Fix "Number of CPUs" output"
(https://github.com/bitcoin/bitcoin/pull/27674)

💬 Sjors commented on pull request "ConnectTip: don't log total disk read time in bench":
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195105709)
We also call `ConnectTip()` from `TestBlockValidity()` and `VerifyDB()`. Do you know how the counter is supposed to behave in those cases?

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1549605549)
Thank you for the detailed comments on your review @ryanofsky! Very happy to see this being fleshed out into a more general and proper kernel notification interface.

Updated 2f9c2d245360b3fad6d469a76c2916d75b027b57 -> 2c58fbf816d73395167a3046c4ce957829bf45f9 ([chainstateRmKernelUiInterface_3](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_3) -> [chainstateRmKernelUiInterface_4](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_4), [co
...

🤔 fjahr reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1428107827)
Code review ACK ac9adf012925c770dfe523c5b57451f313cc8be5

This is alright, I am just not loving the new `assumed_first` parameter to be honest. But curious to hear what you say @jamesob . If you change it, I promise to re-review it swiftly.

💬 fjahr commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1194913264)
This parameter/api seems kind of unnecessary. Do we actually need/want assumed_first=false anywhere explicitly? Otherwise, I would suggest always returning assumed first and adding a comment that this specific ordering is important.

Alternatively, if it is better to have a default behavior of assumed_first=false then I would still find it simpler to `reverse` the order in the one place where it's needed rather than adding the param.

💬 fjahr commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1195081071)
nit: I would prefer `in_ibd` here

💬 fjahr commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1194862228)
somewhat nit: Why not use `ActiveChainstate()` here? From my understanding, it doesn't functionally make a difference but it makes this a cleaner review since the result of this is replacing `ActiveChainstate()`.

💬 furszy commented on issue "Allow getblockfrompeer to use any peer":
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1549635506)
Ok great sipa and Sjors. Notes taken.

I started implementing this in a separate class `BlockRequestTracker` with its own scheduled task to detect the timeout and re-try + follow the requests progress (which also involves listening to the node disconnection signal etc). But, agree that can also add it to the `FindNextBlocksToDownload` existent mechanism as well.
Could merge both worlds and slowly start decoupling the block downloading logic into a separate module while continue using the `Sen
...

💬 MarcoFalke commented on pull request "ConnectTip: don't log total disk read time in bench":
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195171196)
Maybe all globals can be moved to function scope and the `num_blocks_total` can be duplicated to both functions where it is used?

👍 hebasto approved a pull request: "ci: Remove unused errtrace trap ERR"
(https://github.com/bitcoin/bitcoin/pull/27667#pullrequestreview-1428608268)
ACK fad09b703f5c6d8524a09eef771eb4525f9f3225, tested on Ubuntu 22.04: I can still see warnings from the sanitizers in both unit and functional tests.

📝 ajtowns opened a pull request: "net_processing: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675)
This PR replaces the `m_recently_announced_invs` bloom filter with a simple timestamp of the last time we considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally).

The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protect
...

💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1549711784)
As justification for ignoring the privacy benefits of rate limiting INV messages, consider the following approach: an attacker makes multiple inbound connections to our peer, and upon receiving an INV of txs a,b,c,d,... via one connection immediately announces the same txs on each of its other connections. In that case the next INV we send on some other connection to the attacker will skip those transactions, providing a new unique set. With this approach, the attacker should be able to receive
...

📝 hebasto opened a pull request: "qt, test: Add missed header"
(https://github.com/bitcoin-core/gui/pull/729)
Should fix MSVC link errors like [that](https://api.cirrus-ci.com/v1/task/4870882892447744/logs/build.log):
```
addressbooktests.obj : error LNK2019: unresolved external symbol "void __cdecl ConfirmMessage(class QString *,class std::chrono::duration<__int64,struct std::ratio<1,1000> >)" (?ConfirmMessage@@YAXPEAVQString@@V?$duration@_JU?$ratio@$00$0DOI@@std@@@chrono@std@@@Z) referenced in function "void __cdecl `anonymous namespace'::EditAddressAndSubmit(class EditAddressDialog *,class QString
...

💬 hebasto commented on pull request "qt, test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1549746195)
cc @fanquake @sipsorcery

👍 hebasto approved a pull request: "wallet: fix deadlock in bdb read write operation"
(https://github.com/bitcoin/bitcoin/pull/27556#pullrequestreview-1428676304)
re-ACK 69d43905b7f1d4849dfaea1b5744ee473ccc8744

💬 hebasto commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195230362)
Mind elaborating?

🚀 fanquake merged a pull request: "ci: Remove unused errtrace trap ERR"
(https://github.com/bitcoin/bitcoin/pull/27667)

🚀 fanquake merged a pull request: "[23.2] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27663)

💬 sangaman commented on pull request "init: add MALLOC_ARENA_MAX=1 to systemd":
(https://github.com/bitcoin/bitcoin/pull/27642#issuecomment-1549801395)
I'm 64 bit indeed, but wouldn't the same concern be present on 32 bit systems?

I just restarted my node with the changes from #25325 and without the `MALLOC_ARENA_MAX` setting, I'll give it a few days and report back on how the memory usage is going.

🤔 mzumsande reviewed a pull request: "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count"
(https://github.com/bitcoin/bitcoin/pull/27511#pullrequestreview-1428743116)
Tested ACK 7d7ee5e0f4254495349ea68f3d0127c0c336fd12