💬 pinheadmz 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_r1164601290)
Yeah 😬 waddaya think?

💬 ismaelsadeeq commented on pull request "test: add coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27422#issuecomment-1505870199)
Thank you for picking that up.
I will take a look at that.

💬 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_r1164606990)
Sure, will update. And add the comments mentioned in https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1164530898.

💬 ismaelsadeeq commented on pull request "test: added coverage to rpc_scantxoutset.py":
(https://github.com/bitcoin/bitcoin/pull/27453#issuecomment-1505878687)
LGTM Ack ecb79aed4d847d8c95936ac80b7e137f9c17b6f8

🤔 furszy reviewed a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1380282608)
Haven't finished checking the latest changes but, if you like it, https://github.com/furszy/bitcoin-core/commit/8bb14c1eaf09b0bca9efa78e1a7a7a00c61c0a29 is all yours. During the review, the functional test wasn't clear enough for me so I spent a bit of time making it more friendly.

💬 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 {}; })();
}

...