💬 pinheadmz commented on pull request "ci: Move tidy to persistent worker":
(https://github.com/bitcoin/bitcoin/pull/28214#discussion_r1289038500)
doesn't need `PERSISTENT_WORKER_TEMPLATE_ENV` ?

💬 pinheadmz commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1289102281)
nit, extra space ?

```suggestion
argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0), a network/CIDR (e.g. 1.2.3.4/24), all ipv4 (0.0.0.0/0), or all ipv6 (::/0). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
```

💬 zkfrio commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1672111259)
Some full RBF transactions are being mined.

Who cares about origin?

>OpenTimestamps

Even if opentimestamps full RBF transactions are mined

Do you even think for a moment that it doesn't matter if have it default or not?

Or is it just about emotional drama?

Full RBF transactions are being relayed and miners include them in blocks.

If making it default affects your security, I want to know your project. I won't exploit.

🤔 jonatack reviewed a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1570446946)
Post-merge ACK.

The last commit, [p2p: network-specific management of outbound connections](https://github.com/bitcoin/bitcoin/pull/27213/commits/1b52d16d07be3b5d968157913f04d9cd1e2d3678), looks like it could use test coverage.

I've been testing the changes here these past two days, with added/improved logging to monitor the new behavior.

💬 jonatack commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1289135985)
We log the disconnection and protection actions in `EvictExtraOutboundPeers()` and it would be good to log this action as well.

While testing and monitoring the changes in this pull, I found the following diff to be helpful.

```diff
- if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) return;
+ if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) {
+ LogPrintLevel(BCLog::NET, BCLog::Level::Debug,
+
...

💬 jonatack commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1289160066)
The first line of this conditional seems redundant, as it is already checked a few lines earlier for the `OUTBOUND_FULL_RELAY` case.

💬 jonatack commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1289134467)
Suggestions for this log entry:

- use `LogPrintLevel`

- replace "Making" with "Trying to make" (like the anchor connection logging), as connections to privacy networks regularly fail

- add missing hyphen to "network specific" and log the network just after it, instead of at the end where it is less useful (and a bit less clear, e.g. "on onion")

- log the IP address only if the `-logips` config option is set

- add the connection type

<p><details><summary>possible diff</summary><
...

💬 jonatack commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1289142795)
This can more simply use the existing helpers.

```cpp
bool IsManualOrFullOutboundConn() const { return IsFullOutboundConn() || IsManualConn(); }
```

💬 jonatack commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1289140734)
> The return value and the optional output argument are redundant. Better to return `std::optional` and take no arguments:
>
> ```c++
> std::optional<Network> MaybePickPreferredNetwork()
> ```

Agree. This method could also be const and have a Clang thread-safety annotation and related assertions.

📝 jonatack opened a pull request: "p2p: outbound network diversity improvements"
(https://github.com/bitcoin/bitcoin/pull/28248)
Please see the commit messages for details.

🤔 donsheddy4 reviewed a pull request: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467#pullrequestreview-1570563013)
Very good

💬 jonatack commented on pull request "doc: Use GitHub's "Alert" markdown syntax":
(https://github.com/bitcoin/bitcoin/pull/28243#issuecomment-1672248656)
Just one opinion, but I find these alerts/admonitions distract from the text itself both when read on GitHub and also when read in markdown format. They also make reading the source files more difficult, which is how I consume the docs.

It's also preferable to avoid GitHub-specific features and workflows. Greater developer lock-in has been a clear goal for GitHub for years now.

💬 jonatack commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1289302225)
While retouching, mind `s/Valid for/Valid values for/` or `s/source. Valid for <ip> are/source, which can be`

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289335082)
Just changed it to not use two srd solutions and I created a function to "simulate" a manual selection. Also, added more asserts, including checking the input set.

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289335245)
Sounds good. Just did it.

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289336536)
just changed to `1000`. but the reason is to make it more realistic. e.g. do we really use it for 1 sat?

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1672291591)
Force-pushed:

- Improved `Merge`: now it checks the input set and it tries to merge every solution with a "manual" one.
- Improved `AddInputs` to check the total weight.

💬 jonatack commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1672297168)
Seeing these failures locally:

```
$ ./src/test/test_bitcoin -t denialofservice_tests
Running 5 test cases...
Assertion failed: lock m_nodes_mutex not held in net.cpp:1598; locks held:
'cs_main' in net_processing.cpp:5203 (in thread 'test')
unknown location:0: fatal error: in "denialofservice_tests/stale_tip_peer_management": signal: SIGABRT (application abort requested)
test/denialofservice_tests.cpp:182: last checkpoint
Assertion failed: (ret.second), function AddArg, file args.cpp,
...

💬 ariard commented on pull request "doc: Use GitHub's "Alert" markdown syntax":
(https://github.com/bitcoin/bitcoin/pull/28243#issuecomment-1672297394)
+1 Sooner we’ll move out of Github better we’ll be and we would be rather to prepare for greater disruptions of GH availability (e.g regularly backing up the decade of comments on the repo) than rely more on its features.

💬 brunoerg commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#issuecomment-1672302859)
Concept ACK