💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248207817)
In f2daebf8d1ac3829faa6443425cb97a763e9f780 "Bump unconfirmed parent txs to target feerate"

nit: I prefer that comments about what a test does are attached to the test implementation itself rather than the caller.

💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248219188)
This is incorrect, see https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248205239

🤔 mzumsande reviewed a pull request: "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1507793118)
Do I understand it correctly that using the new `forceinbound` permission instead of `noban` would be useful in the following two cases:
1.) If the user lowered `-maxconnections` to a value so low that the protection logic (that protects a fixed number between 20 and 28 inbound peers from eviction) could protect all inbound peers, but we still want to make sure to accept an inbound connection from a protected peer.
2) If we have a slightly higher `-maxconnections` but want to make sure a large
...

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1248242406)
No longer relevant

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1248242732)
Updated to use the mempool sequence number rather than `SteadyClock`

💬 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