💬 instagibbs commented on issue "BITCOIN RECOVERY":
(https://github.com/bitcoin/bitcoin/issues/28247#issuecomment-1671800403)
definitely not a scam

✅ fanquake closed an issue: "BITCOIN RECOVERY"
(https://github.com/bitcoin/bitcoin/issues/28247)

:lock: fanquake locked an issue: "BITCOIN RECOVERY"
(https://github.com/bitcoin/bitcoin/issues/28247)

🚀 fanquake merged a pull request: "doc: diversify network outbounds release note"
(https://github.com/bitcoin/bitcoin/pull/28189)

💬 BrandonOdiwuor commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r1288952594)
Also consider adding logs to this test as the ones above

💬 BrandonOdiwuor commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r1288951022)
Incorporating logs within the test is crucial for offering transparent insight into the test's progression, simplifying the identification of problems, and enhancing comprehension of the test's overall behavior.

💬 TheCharlatan commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1289035093)
> Also, the value will be nullptr before and after the call to HandleRequest

Does this also hold if `m_fun` throws an exception?

👍 pinheadmz approved a pull request: "ci: Move tidy to persistent worker"
(https://github.com/bitcoin/bitcoin/pull/28214#pullrequestreview-1570315176)
ACK fa1d8955f69d3934f975f42eb04b5a3fc0d8aa35

Lots of great comments, code change looks good to me and obviously works on ci.
One question below and also I'm wondering if the output of "include-what-you-use" is used at all? It makes a lot of suggestions but none of that counts as errors?

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK fa1d8955f69d3934f975f42eb04b5a3fc0d8aa35
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4
...

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