💬 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

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1195276803)
Not really familiar with this stuff, but that seems to be an MSVC bug? https://stackoverflow.com/questions/55152552/visual-studio-not-performing-rvo-when-ternary-operator-is-used-and-move-copy-cto

We do ternary operators in plenty of places, don't think we need to workaround this?

💬 hebasto commented on pull request "QRImageWidget: Sizing and font fixups":
(https://github.com/bitcoin-core/gui/pull/506#issuecomment-1549832808)
This https://github.com/bitcoin-core/gui/pull/506#pullrequestreview-1043535557:
> This PR effectively renders font size smaller that makes it harder to read.

remains unaddressed.

💬 Sjors commented on pull request "ConnectTip: don't log total disk read time in bench":
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195297945)
Happy to review such a change, but I barely remember making the original PR and am not very familiar with the bench logging.

💬 glozow commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1549845292)
This wouldn't be backported. The first 2 commits could be considered a band aid "fix" but for a regtest-only RPC. The last 2 commits contain a new feature for miners, and we do not backport new features.

I understand this is a big limitation for Lightning, and I agree it should be treated with high priority. The way to fix it is to provide a way to submit and propagate packages to miners. If you would like to make package relay available in Bitcoin Core, please review #26711.

👍 theStack approved a pull request: "Implement Mini version of BlockAssembler to calculate mining scores"
(https://github.com/bitcoin/bitcoin/pull/27021#pullrequestreview-1428805458)
Code-review ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965

💬 kroese commented on issue "Allow getblockfrompeer to use any peer":
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1549867573)
> Well, peers could delay the response or directly not answer. Stalling the post-filter-matching blocks sync mechanism.

Yes, I think that peers who have reached their `maxuploadtarget` will also refuse to provide the block unless you have the whitelist / `download` permission. So there can be some edge-cases where non-pruned peers still fail to deliver it.

💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1549871285)
Even just backporting the first two commits could be helpful, because it at least reduces the size of the patch that a miner needs to run.

📝 ajtowns converted_to_draft 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
...

💬 MarcoFalke commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195220741)
nit: Looks like this will make inbound peers be treated identical to outbound ones for the purposes of getdata, as long as mapRelay is still around. For in-mempool txs it may be possible to fix by moving the time check outside of `info_for_relay` into this function and returning a nullptr early if the tx is known to not be announced. For non-mempool txs, I don't know.

💬 MarcoFalke commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1195225774)
nit: My recommendation would be to not rename the method, or if you want, to pick `GetEntryTime()` over `GetNodeTime()`, because the function doesn't return the current node time but the time of transaction entry.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1549933311)
Thanks for the review! I'm done with nits on this one, and assumeutxo in general.