💬 MarcoFalke commented on pull request "fuzz: Add HeadersSyncState target":
(https://github.com/bitcoin/bitcoin/pull/26662#issuecomment-1502965567)
concept ack

💬 josibake commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1502967998)
ACK https://github.com/bitcoin/bitcoin/pull/27217/commits/635c50a57b3f72f83f1ee681f6a4b7766b8988f6

💬 MarcoFalke commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502977423)
I just tried bookworm and it was sufficient to remove the gcc suppression.

https://packages.debian.org/bookworm/gcc
https://packages.debian.org/bookworm/valgrind

💬 real-or-random commented on pull request "Update src/secp256k1 subtree to release v0.3.1":
(https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1502994264)
Okay, I think I understand what's going on here:
1. `make distdir` is supposed to create a directory with all files to be distributed
2. the `distdir` target depends on all files to be distributed
3. these file include `src/wycheproof/secdsa_secp256k1_sha256_bitcoin_test.h` because it's listed in `EXTRA_DIST` (perhaps `noinst_HEADERS` would be more appropriate, but the result is the same)
4. `src/wycheproof/secdsa_secp256k1_sha256_bitcoin_test.h` has a dependency on `src/wycheproof/secdsa_se
...

💬 dergoegge commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#issuecomment-1503006167)
utACK 676671527f08ef8113af3661de73db583f6ea9e2

💬 fanquake commented on pull request "wallet: undo conflicts properly in case of blocks disconnection":
(https://github.com/bitcoin/bitcoin/pull/17543#issuecomment-1503021950)
Dropped up for grabs here, see #27145.

💬 fanquake commented on pull request "[wallet] Track conflicted transactions removed from mempool and fix UI notifications":
(https://github.com/bitcoin/bitcoin/pull/18600#issuecomment-1503022715)
Dropped up for grabs here, see #27307.

🚀 fanquake merged a pull request: "test: LLVM/Clang 16 for MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27436)

💬 MarcoFalke commented on pull request "refactor, net processing: Avoid CNode::m_relays_txs usage":
(https://github.com/bitcoin/bitcoin/pull/27270#issuecomment-1503063052)
lgtm ACK 55c4795c5794c5c2f8a69b394b847413c9cfbe36

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1503076466)
> Would be nice to split the actual fix from the other changes. I guess it would be easier for other reviewers too.

I'm working on it.

> I would have liked to remove the `replySent` argument, but that's secondary to this PR which fixes a serious bug. Let's get the bug fixed and if I feel strong enough about it, then maybe I will open a followup to remove that parameter.

I agree, later on a follow-up, I'd try to investigate if `evhttp_request* req` can be mocked somehow so the fuzz can g
...

🚀 fanquake merged a pull request: "refactor, net processing: Avoid CNode::m_relays_txs usage"
(https://github.com/bitcoin/bitcoin/pull/27270)

👍 vasild approved a pull request: "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns"
(https://github.com/bitcoin/bitcoin/pull/27374#pullrequestreview-1379043911)
ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70

I think this fixes the problem described in https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1481322628 and is ok to be merged in its current form.

Could it create another problem of connecting too much to Tor and not try to diversify to IPv4? Yes. Is this unlikely problem? Yes, with a common addrman that is filled mostly with IPv4 addresses. With a non-common addrman? I do not know.

💬 vasild commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1162685367)
This `(int)setConnected.size() >= std::min(nMaxConnections - 1, 2)` was introduced in c769c4af11fc58dd4813d328c7f71042bc577676.

I think that for the purposes of checking whether we are offline, being connected to just one Tor peer means that we are not. This is because the Tor network is global (same for I2P or CJDNS). There is no such thing as "local area network" of Tor nodes (e.g. `10.0.0.0/24`) or localhost Tor address (like `127.0.0.1` in IPv4). If we are connected to one Tor node, it me
...

💬 MarcoFalke commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1503221693)
rebased

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1162744608)
In #8065, @gmaxwell pointed out:

> This is still somewhat incomplete protection because our outbound peers could be down but not timed out

We never really know if our peers are connected or not, because when your network goes down it usually takes some amount of time for each connection to time out. Requiring two connections to be up (versus just 1 connection to e.g. Tor, or some other non-local ipv4/ipv6 network) is a tradeoff around how long we spend in a state where we're punishing addr
...

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1503257503)
> I'd try to investigate if `evhttp_request* req` can be mocked somehow so the fuzz can go thru all the buffering behaviour and other stuff once the replySent argument is removed.

The fuzz _will_ go through this:

https://github.com/bitcoin/bitcoin/blob/53eb4b7a212db7563291509ff4e53327440a9df2/src/test/fuzz/http_request.cpp#L51-L62

if the `HTTPRequest` object is created successfully (once `replySent` is removed and the test does `try/catch`). If the input is such that `HTTPRequest:HTTPRe
...

💬 sdaftuar commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1503258229)
Personally, I don't use this too much, but I have used it in the past (for a while I maintained a patch for myself to allow me to sync one node's mempool from another by using an RPC command). Supporting a mempool sync between two nodes that you control, to bootstrap newly started nodes, seems like a useful thing conceptually.

I don't feel strongly though, and if this is a mostly rarely or never used feature, then I'm not opposed to removing it.

I'd be curious to know how people feel abou
...

💬 MarcoFalke commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1503313635)
In the pull description NODE_NETWORK should say NODE_BLOOM?


> Supporting a mempool sync between two nodes that you control, to bootstrap newly started nodes, seems like a useful thing conceptually.

If you control both nodes, wouldn't it be easier to run the RPC on the sending side directly to fan out all transactions instead of going an extra hop over P2P? If the receiving node is offline, you can copy the mempool dat (see https://github.com/bitcoin/bitcoin/issues/25018#issuecomment-111
...

👍 vasild approved a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1379215224)
ACK 7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55

📝 fanquake opened a pull request: "depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode"
(https://github.com/bitcoin/bitcoin/pull/27447)
It was deprecated in LLVM 15, turned into compile-time error in LLVM 16:
```bash
In file included from /usr/lib/llvm-16/bin/../include/c++/v1/cassert:19:
/usr/lib/llvm-16/bin/../include/c++/v1/__assert:22:5: error: "Defining _LIBCPP_DEBUG is not supported anymore.
Please use _LIBCPP_ENABLE_DEBUG_MODE instead."
^
1 error generated.
```

and has been removed entirely in LLVM 17 (main): https://github.com/llvm/llvm-project/commit/ff573a42cd1f1d05508f165dc3e645a0ec17edb5.

[Building l
...