💬 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

📝 MarcoFalke reopened a pull request: "contrib/init: Better systemd integration"
(https://github.com/bitcoin/bitcoin/pull/25975)
```
1. Make logs available to journalctl (systemd's logging system) by not
specifying -daemonwait, which rightfully has its own set of stdout
and stderr descriptors (a user invoking with -daemonwait on the
command line should not see any logs). It makes more sense not to
daemonize in the systemd context anyway.

2. Make systemd aware of when bitcoind is started and in steady state by
specifying -startupnotify='systemd-notify --ready' and Type=notify.
NotifyAccess=all i
...

💬 MarcoFalke commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1182388624)
```suggestion
compute_engine_instance: # https://cirrus-ci.org/guide/custom-vms/#custom-compute-engine-vms
disk: 100
```