💬 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_r1163469554)
I would try to not provide the context by reference to the function. The lambda function's `wtx` shadows the function's `wtx`. Could just provide `disconnect_height`.

💬 rsafier commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505973365)
I made some quick mods to have working docker for this PR: https://github.com/rsafier/bitcoin_signet/tree/carman-blocktime-adjustable

I used it to get my 10Msats.

💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164674497)
Done, and also dropped `DEBUG_LOG_OPTS` and the no-longer useful `Assume` check (the `NONFATAL_UNREACHABLE` check does it better), and made the two `EnableOrDisableLogCategories` functions simpler and more similar. Very helpful feedback (thanks!)

<details><summary><code>git diff e9f6fd0 8ac6539</code></summary><p>

```diff
diff --git a/src/init/common.cpp b/src/init/common.cpp
index a5b07420d94..c99a512459b 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -77,26 +77,28 @@
...

💬 jonatack commented on pull request "doc: remove incorrect line from example":
(https://github.com/bitcoin/bitcoin/pull/27454#discussion_r1164694164)
Instead of removing, maybe clarify along the lines of

`Create a transaction with no inputs specified (automatically include coins from the wallet)`

💬 jonatack commented on pull request "doc: remove incorrect line from example":
(https://github.com/bitcoin/bitcoin/pull/27454#discussion_r1164695637)
(While here, perhaps add more examples to the help, if that seems like it would be useful.)

📝 Muhammedhamid23 opened a pull request: "Muhammedhamid23 patch 1"
(https://github.com/bitcoin/bitcoin/pull/27457)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ achow101 closed a pull request: "Muhammedhamid23 patch 1"
(https://github.com/bitcoin/bitcoin/pull/27457)

📝 achow101 locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/27457)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

🤔 ajtowns reviewed a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214#pullrequestreview-1382390744)
ACK 17e705428ddf80c7a7f31fe5430d966cf08a37d6

💬 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