💬 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_r1248280153)
Improved the test comment.

```diff
- assert all(v == True for v in node.logging().values()) # -debugexclude=[none,qt] does not exclude qt
+ # Expect the "none" value passed in -debugexclude=[none,qt] to mean that
+ # none of the categories passed with -debugexclude are excluded: neither
+ # qt passed here nor leveldb/libevent/rand passed by the test framework.
+ assert all(v == True for v in node.logging().values())
```

💬 TheCharlatan commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1615189889)
Re https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1613018513

> (assuming our self compiled valgrind isn't skipping more expensive checks)

I checked the debian/rules file and did not see anything that we might be missing there in terms of build configuration.

Concept ACK

💬 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#issuecomment-1615192850)
Updated the tests with @luke-jr's review feedback (thanks!)

<details><summary>git diff 08a367c 93b387f</summary><p>

```diff
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -262,11 +262,11 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)

BOOST_FIXTURE_TEST_CASE(logging_IsNoneCategory, LogSetup)
{
- for (const std::string& s : {"none", "0"}) {
- BOOST_CHECK(LogInstance().IsNoneCategory(s));
+ for (const char* const& c : {"none", "0"}) {
+
...

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1615199522)
Seeing this thread safety warning locally with the fuzz build only (ARM64 clang 16).

```
net_processing.cpp:5769:63: warning: calling function 'GetSequence' requires holding mutex 'm_mempool.cs' exclusively [-Wthread-safety-analysis]
tx_relay->m_last_inv_sequence = m_mempool.GetSequence();
^
1 warning generated.
```

📝 kevkevinpal opened a pull request: "init: adding check for : for -torcontrol flag"
(https://github.com/bitcoin/bitcoin/pull/28018)
right now for -torcontrol you can pass in any string as long as it doesn't have a : but that is invalid. I added an additional check before calling SplitHostPort to see if there is a :


closes https://github.com/bitcoin/bitcoin/issues/23589
<!--
*** 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
firs
...

💬 kevkevinpal commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1615220421)
<img width="500" alt="Screenshot 2023-06-30 at 4 40 24 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/d95ba8f9-dad2-4ada-b235-41acf8b58aa4">

-----

<img width="500" alt="Screenshot 2023-06-30 at 4 40 12 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/d2280a41-b1c2-44e6-b32a-9112321b1664">

-----

<img width="500" alt="Screenshot 2023-06-30 at 4 39 51 PM" src="https://github.com/bitcoin/bitcoin/assets/15950706/ec94182a-b9dd-4701-b57d-6a45ad3487b3">

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248314125)
> I see, so maybe current code is buggy and returns bad rpc results then?

yeah.

> I guess simplest thing to do is just document GetFirstStoredBlock's odd behavior in this case and keep it unchanged, rather than returning nullptr which could segfault, or adding an assertion.

Have struggled a bit with this.
I tried to maintain the previous `GetFirstStoredBlock` behavior but it conflicts so much with the function's description: "Find the first stored ancestor of 'start_block' immediately
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248333228)
hm, this one was intentional. I edited `AddTestNode` and it was previously missing a whitespace between functions

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248337602)
removing the optional from the out param means that the compiler can't guarantee that the value of `network` was set. so `preferred_net` needs a default value, which I set to `NET_UNROUTABLE` and then checked for later in the function. this approach doesn't feel much cleaner to me than the previous of having network be optional, but I don't feel strongly either way. or, is there a better solution I'm not seeing right now?

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248337827)
changed back to a map, so this is relevant again. removed the explicit initialization due to the default behavior you mentioned. thanks!

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248337990)
fixed

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248338620)
thanks! okay, fixed and confirmed that none of the current patch invokes `ConnectedThroughNetwork` anymore :)

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248338719)
ah, good catch. updated

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248338974)
updated and double checked that clang-format doesn't have any other suggestions for the patch

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1248339085)
ah I see. okay updated :)

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1615277942)
latest push returned to map & addressed review comments

@vasild
I very much agree with your line of questioning. storing as a data structure definitely has some explicit tradeoffs to counting the number on the fly. neither is a clear winner for me, so I'm fine implementing whatever reviewers prefer.

> It is also not generic - the "full outbound or manual" number is used only in this specific case and unlikely to be used elsewhere in the future. A more generic approach, that could possib
...

👍 luke-jr approved a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1508023222)
utACK e7cf8657e1165ea5da3911a9e543837cd8938f97

Approach: Unsure about the refactoring, but ok

💬 luke-jr commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#discussion_r1248375619)
It's the pointer that's `const` here. Seems pretty reasonable, though it only has a net effect of preventing accidentally changing `wallet_model` in this method.

💬 luke-jr commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#discussion_r1248375640)
The `const` has no effect here.

💬 tuanggo commented on issue "ThreadI2PAcceptIncoming temporarily bypasses 125 connection ceiling":
(https://github.com/bitcoin/bitcoin/issues/27843#issuecomment-1615739130)
ok