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

💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447)
I'm not sure this is threadsafe with our `g_work_queue`. Will we not have race conditions here when multiple requests on the same `RPCHelpMan` are in the queue at the same time?

💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285831220)
nit: as we're getting the parameter value, would `Arg` be a better name? Perhaps it's too similar to `RPCArg` though (which unfortunately would have been more appropriately named `RPCParam`, I think).

💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285838697)
I think we also want to set this to nullptr in the many cases where we throw early in this function?

💬 MarcoFalke commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285862343)
`RPCHelpMan` are constructed and allocated fresh for each RPC call.