💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#discussion_r1288400379)
Having the managed objects have a reference to the manager leads to absolute spaghetti code, so leaning towards NACK on that.

💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#discussion_r1288527915)
You can just get rid of NodesSnapshot entirely now, it was only needed as a RAII helper for the manual ref counting.

💬 SambhavsCreation commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1671550220)
Is someone still working on this? Please let me know as if not I might start working on it myself

💬 eriknylund commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1671555701)
> Is someone still working on this? Please let me know as if not I might start working on it myself

I have a PR https://github.com/bitcoin/bitcoin/pull/28212 open for it, so please review if you have a chance. 🙏

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1671573912)
I ended up "borrowing" some code from the followup PRs, without having to rebase on them.

This code compiles and the index doesn't crash. That's about the extend of it though :-) For now.

💬 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` ?