💬 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
```

💬 ajtowns commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1531265734)
> Yeah I'm pretty optimistic that cluster mempool will provide a natural solution like this.

Probably, but I suspect ephemeral anchors or similar will be ready before we've got all the i's dotted and t's crossed on cluster mempool, so some stop gap patch will be worthwhile. Still, we can cross that bridge once we get there.

💬 willcl-ark commented on issue "bitcoin core crashes when too many rpc calls are made":
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-1531268374)
> My impression is that mostly these issues are caused by people spamming their own rest interface with their own scripts. If users know about the issue they should be able to accommodate it pretty easily and not be surprised by the issue.

Agreed.

> I am happy to investigate other ideas but I currently don't see a real fix other than waiting until `libevent` 2.2 is released which should include either [libevent/libevent#578](https://github.com/libevent/libevent/pull/578) and/or [libevent/l
...

📝 brunoerg converted_to_draft a pull request: "test: merge banning test from p2p_disconnect_ban to rpc_setban"
(https://github.com/bitcoin/bitcoin/pull/26863)
There are tests related to banning in both `p2p_disconnect_ban` and `rpc_setban`, some of them are testing same scenarios. So, this PR moves all banning test stuff to `rpc_setban` and leaves `p2p_disconnect_ban` only for disconnecting stuff. Since `p2p_disconnect_ban` doesn't have any banning stuff, this PR also renames that to `p2p_disconnect`.

📝 Bushstar opened a pull request: "Reduce calls to various SetNull methods"
(https://github.com/bitcoin/bitcoin/pull/27551)
This PR aims to defaults initialise various class members and make redundant several calls to SetNull methods for those classes. This makes the default constructor for a few classes without any other constructors redundant as well but rather than remove them I'll created them with default in case of compiler complaints.

- Default initialise m_data member in base_blob which makes calls to SetNull on uint256 redundant, create base_blob() with default rather than remove in case of compiler comp
...

💬 MarcoFalke commented on pull request "Wallet: Add foreign_outputs metadata to support CoinJoin transactions":
(https://github.com/bitcoin/bitcoin/pull/25991#discussion_r1182407309)
For new RPCs it might be good to return an empty dict to avoid breaking changes later on and confusing behavior now.