💬 ajtowns commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1164826176)
Probably worth documenting that passing in a `network` here rather than `nullopt` can make the search significantly slower (though still fast)?

💬 ajtowns commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1164833780)
Do we really want an unconditional assertion failure here, vs say:

```c++
if (Assert(it != mapInfo.end()) && it->second.GetNetwork() == *network) break;
```

(maybe a similar question for `GetEntry`)

💬 ajtowns commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1164840456)
Seems like having:

```c++
int counter = 256;
while (--counter > 0 && (!new_selected || !tried_selected)) { ... }
BOOST_CHECK(new_selected);
BOOST_CHECK(tried_selected);
```

would be better here than an infinite loop leading to CI timeout on error. A one-in-2**256 chance of failure is already the security of bitcoin as a whole, after all; and if our randomness generator is biassed enough that it fails more than that, it's probably better that the test case fails anyway? That we use the
...

💬 ajtowns commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1164902731)
Why not just use the `evictionman` from `connman` directly (or have `connman` use its `peerman`'s eviction manager)? Is there any reason to ever want them to have different eviction managers?

Having done that, why not have `CConnman` (or `PeerManager`) create (and own) its own eviction manager, rather than needing one to be passed in?

💬 ajtowns commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1164910266)
`candidates.reserve(m_candidates.size())` ?

💬 ajtowns commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1164935668)
I think something like this would work:

```c++
class EvictionManagerImpl
{
...
template <typename Fn>
auto CandidateOp(NodeId id, Fn&& fn)
{
LOCK(m_candidates_mutex);
auto it = m_candidates.find(offset);
if (it != m_candidates.end()) {
return fn(it->second);
} else {
// return a dummy value of the right type, including void
return ([]() -> decltype(fn(it->second)) { return {}; })();
}

...

💬 ajtowns commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1164945576)
Repeatedly looking up the eviction manager map and locking and unlocking access to it, and introducing the potential for maps to be out of sync and having to add asserts to check for that doesn't seem ideal...

💬 ajtowns commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1164943097)
`m_evictionman.CopyEvictionStats(pnode->GetId(), vstats.back());` ?

💬 denavila commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1165016111)
This change appears to trigger this assert when executing "getnewaddress" from the Console:
Assertion failed: (invoked), function NotifyAddressBookChanged, file walletmodel.cpp, line 392.

debug.log shows this error message:
GUI: QMetaMethod::invoke: Unable to handle unregistered datatype 'wallet::AddressPurpose'

💬 MarcoFalke commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#issuecomment-1506443972)
`ci_native_fuzz_msan` fails for me locally

👍 vasild approved a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1382698478)
ACK 6be8b6d8106210c6f44bc9c0124bfd1476d2952e

💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165066216)
nit: it is case-insensitive, so no point in using a mixture of lower case and upper case.

💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165045822)
You may expand commit b3261389eadf050c0de590c2d9cd69ad9f62acff `net, refactor: pass reference for peer address in GetReachabilityFrom` to remove `NET_UNKNOWN` since it is unused after that commit.

💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165072581)
nit: might as well ditch `addr_empty` and use

```suggestion
BOOST_CHECK(!GetLocalAddress(*peer_onion).IsValid());
```

💬 vasild commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1165073055)
```suggestion
// local addresses from all networks
```

🤔 MarcoFalke reviewed a pull request: "refactor: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1382795595)
any reason there are two commits? Seems odd to first add unused includes and then remove them in the second commit.

It might be better to not add the unused includes in the first place

💬 MarcoFalke commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1165110007)
Any reason to change the order? Unless there is a reason, this shouldn't be changed, because previously and commonly the first include in the cpp file is the corresponding header file to catch missing includes.

📝 hebasto opened a pull request: "build: Fix USDT detection on FreeBSD"
(https://github.com/bitcoin/bitcoin/pull/27458)
From https://github.com/hebasto/bitcoin/pull/13#pullrequestreview-1381000080:
> [On FreeBSD] Tracepoints (USDT) are not detected even though `sys/sdt.h` is available in `/usr/include`, this is the same as with autotools (unrelated to the cmake effort).

The suggested commit fixes USDT detection, but compiling fails because the `/usr/include/sys/sdt.h` header does not define `DTRACE_PROBE{6,7,8,9,10,11,12}` macros.

💬 hebasto commented on pull request "build: Fix USDT detection on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506586033)
cc @0xB10C @vasild

💬 0xB10C commented on pull request "build: Fix USDT detection on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1506629380)
Thanks for looking into this, however, I'm not sure if it's the right time for this. At the moment, we only really support the tracepoints on Linux.

> The suggested commit fixes USDT detection [..]

The suggested commit **enables** USDT detection on FreeBSD, something that (somewhat unintentionally) wasn't enabled before. Not sure if it's worth enabling them at the moment for FreeBSD. It would be awesome to have them on more OS's (e.g. macOS see https://github.com/bitcoin/bitcoin/pull/25541
...