💬 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

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1165987641)
Would be good to check that the map contains the element prior accessing it. Otherwise the GUI could be adding a new address book entry by mistake.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1507577994)
Thanks so much for the great review @vasild I think I addressed all your feedback up to the adding UNIX to CNetAddr. I understand your concerns there and we can chat about it, I'll look at alternatives as well. What I did do in this last push was mostly separate unix sockets from regular sockets, and adding `Proxy.GetSocket()` to decide which socket factory to call.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1166002075)
I'm a bit intimidated by the syntax `prevector<ADDR_IPV6_SIZE, uint8_t> m_addr{ADDR_IPV6_SIZE, 0x0};` -- is that only 16 bytes?