💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165801803)
See this ci failure: https://cirrus-ci.com/task/5039125594636288?logs=ci#L2638-L2640

This is why I used a shorter path, sometimes TMPDIR is > 50 characters! You have a good point about getting max `sun_path` from the system though, maybe it will balance out

```
File "/private/var/folders/v7/fs2b0v3s0lz1n57gj9y4xb5m0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-arm64-apple-darwin/test/functional/test_framework/socks5.py", line 131, in __init__
self.s.bind(conf.addr)
OSError: AF_UNIX p
...

💬 mzumsande commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165808152)
This removes the check `node->fDisconnect`, which I think could be dangerous:
We set `fDisconnect` in `AttemptToEvictConnection()`, but don't immediately disconnect. The actual cleanup happens at some later point, when `ThreadSocketHandler` calls `DisconnectNodes()`.
So if we have an attacker that makes new connections to us rapidly, there would be a race between `ThreadSocketHandler` and the attacker making the next connection to us. If the attacker wins this race, we could accept two inbound
...

💬 mzumsande commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165793761)
Could use `Assume` and continue in order to avoid the `assert`.

💬 mzumsande commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165796262)
00230350cd32d5a1d2421b588a7f493afa7c69dd: I think that `prefer_evict` could also be removed from `CNodeOptions`.

💬 1440000bytes commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1507338618)
Hi @MarcoFalke

More than 5 tests are failing

💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1507391422)
I changed the approach to force named RPC options, compatible with https://github.com/bitcoin/bitcoin/pull/26485

💬 MarcoFalke commented on issue "24.0.1 crash on restart":
(https://github.com/bitcoin/bitcoin/issues/27088#issuecomment-1507426977)
I meant the Bitcoin Core log :)

Maybe also check for OOM?

💬 kevkevinpal commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1165893783)
would it make sense to add a `./pathto/mempool.dat` to the example param because right now when I type
`bitcoin-cli importmempool`

I get

```
Examples:
> bitcoin-cli importmempool
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "importmempool", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
```

This might be confusing
```suggestion
RPCExamples{HelpExampleCli("importmempool", "./pathto/mempool.dat") + HelpExample
...

💬 1440000bytes commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1507446950)
I retract my NACK

https://github.com/bitcoin/bitcoin/pull/27426#pullrequestreview-1381688305

ACK for removing this useless p2p message

💬 brunoerg commented on pull request "test: add support for all networks in `deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/27295#discussion_r1165900736)
You're correct, seems better your approach.

💬 brunoerg commented on pull request "test: add support for all networks in `deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/27295#issuecomment-1507449831)
Force-pushed changing the approach for torv3 addresses, added @pinheadmz as co-author.

💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165903453)
good catch, removed NET_UNKNOWN.

💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165903653)
fixed

💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165903972)
done

💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165904167)
fixed

💬 jonatack commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1507486375)
Concept ACK, will review.

💬 kevkevinpal commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1507496302)
Concept ACK 846ff37fe7ac2a5b4f855faaadb3434158ed6364 (need to check latest commit)

### tested by doing the following in regtest

`bitcoin-cli createwallet "testwallet"`
`bitcoin-cli getnewaddress`
`bitcoin-cli generateto address 6 <previous commands output>`
`bitcoin-cli generate 20 (did this a couple times)`
`bitcoin-cli getbalance` (validate you have a balance)
`bitcoin-cli getnewaddress`
`bitcoin-cli send '{"<above output>": 0.1}' null "unset" null '{"fee_rate": 1}'` (did this a fe
...

💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1165959935)
In f998cdbc:

Would be good to move this `NotifyTransactionChanged` to `RecursiveUpdateTxState` too. So `MarkConflicted` and the `BlockDisconnected` txes state changes are notified to the upper layers.

💬 furszy commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1165954948)
In f998cdbc:
This lock isn't needed. All callers have `cs_wallet` acquired. Could add the lock required annotation instead.

🤔 furszy reviewed a pull request: "refactor: Remove CAddressBookData::destdata"
(https://github.com/bitcoin/bitcoin/pull/27224#pullrequestreview-1384148699)
Code ACK a5986e82