💬 glozow commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#issuecomment-1667718598)
Rebased

💬 hebasto commented on issue "error C3203: 'UniqueLock'":
(https://github.com/bitcoin/bitcoin/issues/28229#issuecomment-1667719375)
> It's probably all about habit. I don't see any problems. VS2019 supports ISO C++20 Standard.

More details are available here: https://github.com/bitcoin/bitcoin/pull/24531.

🤔 dergoegge reviewed a pull request: "Use shared_ptr for CNode inside CConnman"
(https://github.com/bitcoin/bitcoin/pull/28222#pullrequestreview-1565029167)
Concept ACK on getting rid of our custom ref counting.

I actually implemented something similar a while ago (https://github.com/dergoegge/bitcoin/commit/b3913b4c052b37790b3737e5ddb9c2d029502477) but didn't pursue because it doesn't retain the behavior we currently have w.r.t. which thread actually deletes the `CNode`. That is also the reason why https://github.com/bitcoin/bitcoin/pull/10738 is a bit more complex and introduces new pointer types.

> which thread actually deletes the CNode.

...

💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#discussion_r1285681548)
Is there a reason why we need to use the shared_ptr type in net processing?

💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#discussion_r1285689978)
In a way this is worse than the custom ref counting because it is harder to assert that we actually don't take new references after this check.

💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#discussion_r1285682361)
(I know this will get push back but)

nit: Could we call this `ConnectionRef`? After all the connection manager is managing... connections.

💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#discussion_r1285683763)
It would also seem more modern to use `using` here

💬 dergoegge commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1667726251)
cc @theuni (just in case you are interested in chiming in)

💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1667726750)
Concept ACK

💬 fanquake commented on issue "error C3203: 'UniqueLock'":
(https://github.com/bitcoin/bitcoin/issues/28229#issuecomment-1667732205)
> but I am not sure what we are supposed to do here?

Yea, nothing we can do about broken software that we explicitly don't support. Closing for now. Discussion can continue if needed.

✅ fanquake closed an issue: "error C3203: 'UniqueLock'"
(https://github.com/bitcoin/bitcoin/issues/28229)

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1285784839)
> Also I think it's a bit unclear where the "additional flags" go, are they part of [permissions@] ?

Yes, see the docs of `-whitelist`:
```cpp
argsman.AddArg("-whitelist=<[permissions@]IP address or network>", strprintf("Add permission flags to the peers using the given IP address (e.g. 1.2.3.4) or "
"CIDR-notated network (e.g. 1.2.3.0/24). Uses the same permissions as "
"-whitebind. "
"Additional flags \"in\" and \"out\" control whether permissions apply to i
...

📝 fanquake opened a pull request: "doc: remove Fedora libdb4-*-devel install docs"
(https://github.com/bitcoin/bitcoin/pull/28231)
These are no-longer installable on any recent Fedora (last working version was 32).
Remove the install instructions, and consolidate this section to be the same as the
Ubuntu & Debian BDB install instructions.

💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285799742)
Not sure about the formatting change and replacing "Fedora" with "Ubuntu". But removing the `dnf install` seems fine.

💬 fanquake commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285801475)
The formatting is the same as the above (hence the typo). Will fixup.

💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285808893)
This is still wrong. Also, I am not sure why the formatting of a section that is about to be removed needs to change at all. Just leave it as-is?

💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285813032)
Ok, I see `dnf` actually installs 5.3 and not 5.1. It would be better to mention such changes in the message instead of claiming the section is the "same" as Ubuntu.

💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285816292)
Nvm my previous comment, it is still wrong. You are changing 5.3 to 5.1 for no reason (not the other way round).

Would be better to just leave the section as-is, unless there is a reason to change something.

💬 fanquake commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285821657)
I didn't see an issue with consolidating it to be the same as another identical section. In any case, no more fomatting changes, and fixed the typo in the Ubuntu/Debian section.

💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285806857)
nit: would be helpful to specify what it throws, and perhaps use `@throws` to make it structured?