💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1671589152)
>> makes operations such as deleting them from multiple threads difficult
>
> Why is this a goal?

I missed this in the OP and think that this should not be a goal or should be well justified.

I like getting rid of the custom ref counting for other reasons. Mainly, because it's annoying and bug prone.

Something I have been working on is hiding CNode from the public net interface and turning the connection manager into a NodeId based manager, e.g. `CConnman::PushMessage(CNode*, CSerial
...

💬 willcl-ark commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1671595104)
> @dergoegge Thanks for the ping.
>
> > makes operations such as deleting them from multiple threads difficult
>
> Why is this a goal?
>
> I'm quite nervous about these changes. I've spoken with @dergoegge about this a few times now, but I'm afraid my information and opinions on the code involved are quite dated at this point (2017 to be exact :), so I haven't had much to contribute.
>
> I'll say though, any changes or refactors for future changes to the threading model should be ver
...

📝 achow101 opened a pull request: "wallet: Use CTxDestination in CRecipient instead of just scriptPubKey"
(https://github.com/bitcoin/bitcoin/pull/28246)
For silent payments, we want to provide a `SilentPaymentsDestination` to be used as the recipient, which requires `CRecipient` to use something other than just the `scriptPubKey` as we cannot know the output script for a silent payment prior to transaction creation. `CTxDestination` seems like the obvious place to add a `SilentPaymentsDestination` as it is our internal representation of an address.

In order to still allow paying to arbitrary scriptPubKeys (e.g. for data carrier outputs, or th
...

⚠️ reclaim4u opened an issue: "BITCOIN RECOVERY"
(https://github.com/bitcoin/bitcoin/issues/28247)
### Motivation

Losing money on the internet is all too common these days, and I don't see why anyone should be ashamed to admit it. The frightening thing is that anyone can become a victim, regardless of intelligence,I would have considered reporting the theft to law enforcement or the FBI's anti-money laundering unit, but the chances of those actions resulting to the recovery of stolen crypto are extremely remote.

### Possible solution

I learnt about the Reclaim RECOVERY CENTRE INC. through
...

💬 reclaim4u commented on pull request "wallet: Use CTxDestination in CRecipient instead of just scriptPubKey":
(https://github.com/bitcoin/bitcoin/pull/28246#issuecomment-1671770517)
I thought I was entering an age of new money from online investment when I learnt about binary options, I was greatly eager to get on board even more especially because i had some knowledge of online trading, everything changed when I was ready to make my withdrawal and I could get nothing. The entire thing literally broke down in my face when I became convinced my money was stuck with them, read came across a recovery experts and they did a good job helping me settle a problem that was having m
...

🤔 mzumsande reviewed a pull request: "doc: diversify network outbounds release note"
(https://github.com/bitcoin/bitcoin/pull/28189#pullrequestreview-1570099879)
ACK 7463d259e14676846f0b43e2b5f2af3faf688cb4

Release note looks good to me!

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