💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1615138376)
Rebased and switched to using the mempool sequence instead of `SteadyClock`

💬 sipa commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1615148105)
> I also spent some time whiteboarding whether package RBF could be feasible for non-v3 this way. I haven't found anything I'm happy with yet, but I think incorporating "cost to RBF conflicts" into the chunking algorithm could be a start.

I haven't thought hard about this, but package RBF sounds a whole lot more complex - and that's probably something that actually becomes easier to reason about post-clustermempool.

> One thing I want to preserve from the current approach is minimizing gro
...

💬 kevkevinpal commented on issue "init: torcontrol argument should be validated":
(https://github.com/bitcoin/bitcoin/issues/23589#issuecomment-1615162677)
I see it not throwing the error because it is returning true here if it has no `:`

https://github.com/bitcoin/bitcoin/blob/61d59fed74108f31eb4e9a2faa3f36422a37000e/src/util/strencodings.cpp#L117

I can open a PR to change this to false but it would affect these files that use `SplitHostPort` in `./bitcoin/src`

```
./src/test/fuzz/string.cpp:87
./src/test/netbase_tests.cpp:89
./src/util/strencodings.cpp:102
./src/util/strencodings.h:106
./src/bitcoin-cli.cpp:746
./src/net.cpp:547
.
...

💬 santochibtc commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1615177745)
I don't see the use of the histogram for fee estimation. You can have a lot of transactions with minimum fee that generate a large bar dominating the histogram. I see more useful a representation like mempool.space showing the list of transactions ordered by decreasing fee and grouped by blocks. That is a list of pending blocks with associated feerate ranges.

💬 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
...