🚀 fanquake merged a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(https://github.com/bitcoin/bitcoin/pull/27191)

🤔 ajtowns reviewed a pull request: "mempool: keep CPFP'd transactions when loading from mempool.dat"
(https://github.com/bitcoin/bitcoin/pull/27476#pullrequestreview-1408532061)
Approach NACK: I don't think holding cs_main or the mempool lock for the full import period is okay.

I'm not sure this needs fixing at all at this point: it doesn't affect nodes that don't restart in between the tx being broadcast and mined; it doesn't affect nodes that don't have a full mempool (eg, if you raise the limit to 1GB or 2GB instead of 300MB); and it's not needed if the child tx is either rebroadcast or rbf'ed. In some sense, saving the mempool and reloading it on restart is just
...

💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182180279)
We currently only do `bypass_limits` for txs from a reorg; not even packages can bypass limits otherwise. I think leaving `bypass_limits=false` for loading from mempool.dat should be fine until that changes?

💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182181426)
This may be evicting txs that were present in the mempool prior to the import, in which case it ill overestimate failed and underestimate count?

💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182176759)
I don't think locking `cs_main` or `pool.cs` for this long is okay.

💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182182757)
If `bypass_limits` is left as false, I think these changes are not necessary?

🤔 theStack reviewed a pull request: "test: added coverage to rpc_scantxoutset.py"
(https://github.com/bitcoin/bitcoin/pull/27453#pullrequestreview-1408719439)
Concept ACK,

and warm welcome as a new contributor!

Looks good to me, just one nit: the commit subject line has a missing-character-typo in the functional test filename (rpc_cantxoutset.py -> should be rpc_scantxoutset.py), can you fix that please?

💬 glozow commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#discussion_r1182298691)
To be clear I agree that this was not the right thing to do.

💬 glozow commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1531154152)
Thanks for writing up a full review @ajtowns. Will keep this feedback in mind for the future if/when we get to this bridge again.

> implement cluster mempool first and export mempool.dat in chunks from highest to lowest fee; when importing, assume that the mempool contains chunks from highest score to lowest, build up a chunk to import; once it's over the mempool minfee, import the txs immediately; if it's over 1MvB and still under the mempool minfee, drop it and stop attempting to import txs
...

💬 glozow commented on pull request "Only allow getdata of recently announced invs":
(https://github.com/bitcoin/bitcoin/pull/19109#discussion_r1182313081)
Hope you don't mind me digging up an old PR - why do responses to mempool requests bypass the recently announced filter? And could it make sense to remove this special case? I get the idea is to give the peer access to full mempool contents, but it still seems better to only serve the stuff we announced. Concerned about `-peerbloomfilters=1` nodes getting fingerprinted through sending `mempool` + `getdata` for arbitrary transactions.

⚠️ liuyangc3 opened an issue: "Build broken when enabling fuzzing on Apple M1 hw using homebrew llvm."
(https://github.com/bitcoin/bitcoin/issues/27550)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I got same error in https://github.com/bitcoin/bitcoin/issues/24386


### Expected behaviour

shoule be able to compolice fuzzer

### Steps to reproduce


```bash
./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined --disable-asm CC=/opt/homebrew/opt/llvm/bin/clang CXX=/opt/homebrew/opt/llvm/bin/clang++

make
Making all in src
CXXLD test/fuzz/fuzz
Undefined sym
...

💬 sipa commented on pull request "Only allow getdata of recently announced invs":
(https://github.com/bitcoin/bitcoin/pull/19109#discussion_r1182319182)
I believe that was just done not to break existing use cases of BIP35. The thinking was perhaps that since it requires special setting/permission anyway for that peer, it can bypass the fingerprinting protections.

💬 sipa commented on pull request "p2p: Request NOTFOUND transactions immediately from other outbound peers, when possible":
(https://github.com/bitcoin/bitcoin/pull/15505#issuecomment-1531213218)
I believe this PR's goal has now effectively been solved by #19988.

💬 MarcoFalke commented on pull request "test: added coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27453#issuecomment-1531226284)
lgtm ACK ecb79aed4d847d8c95936ac80b7e137f9c17b6f8

💬 MarcoFalke commented on issue "Build broken when enabling fuzzing on Apple M1 hw using homebrew llvm.":
(https://github.com/bitcoin/bitcoin/issues/27550#issuecomment-1531233709)
For testing and fuzzing I recommend `master`. Does it happen there as well?

💬 yusufsahinhamza commented on issue "Improve porting documentation for legacy-only wallet RPCs":
(https://github.com/bitcoin/bitcoin/issues/25363#issuecomment-1531239393)
@laanwj Hello, as #25680 merged, do you think this issue may be closed?

💬 willcl-ark commented on pull request "Only allow getdata of recently announced invs":
(https://github.com/bitcoin/bitcoin/pull/19109#discussion_r1182380379)
Just repeating a comment made on IRC here for posterity.

It seems to be the case that no special setting/permission is required to solicity a response to a mempool msg query, only that the local node has set NODE_BLOOM (_or_ we have whitelisted it with the mempool netpermission): https://github.com/bitcoin/bitcoin/blob/d89aca1bdbe52406f000e3fa8dda12c46dca9bdd/src/net_processing.cpp#LL4603C52-L4603

🤔 real-or-random reviewed a pull request: "contrib/init: Better systemd integration"
(https://github.com/bitcoin/bitcoin/pull/25975#pullrequestreview-1408834823)
ACK mod nits

This is very useful for systemd users. The changes match systemd docs and I tested that process management and logging works as intended.

@achow101 Can you reopen this?


One minor issue: This means what (with default config), we have log output both in systemd's journal and in the log file. In principle, it may make sense to also pass `-nodebuglogfile`, but I'm not sure if this is really what we want because it means that the user can't override this option in the config
...

💬 real-or-random commented on pull request "contrib/init: Better systemd integration":
(https://github.com/bitcoin/bitcoin/pull/25975#discussion_r1182373299)
```suggestion
-startupnotify='systemd-notify --ready' \
-shutdownnotify='systemd-notify --stopping'
```
Shutdown notifications are just "nice to have" and I don't think anyone will rely on this, but now that we have the command-line option, why not use it here.

💬 MarcoFalke commented on pull request "test: add ripemd160 to test framework modules list":
(https://github.com/bitcoin/bitcoin/pull/27542#issuecomment-1531251936)
lgtm ACK 768ae178affa5be5960dfff38f7167a13f99e774

💬 MarcoFalke commented on pull request "test: add ripemd160 to test framework modules list":
(https://github.com/bitcoin/bitcoin/pull/27542#discussion_r1182382247)
Maybe add a comment that this should be kept in sync with the output from `grep`:

```sh
git grep unittest.TestCase ./test/functional/test_frame