💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1135699239)
```suggestion
strPrint += "\nRun \"bitcoin-cli -h\" for help or \"bitcoin-cli listwallets\" to see which wallets are currently loaded.";
```

💬 instagibbs commented on pull request "[POLICY] Ephemeral anchors":
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1468273207)
Should a flag be threaded through to disallow any dust outputs even if they're EA?

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1468285386)
ACK 0fe610deba336f0370d68c130dc9a223b7db964e modulo the dangling closing parenthesis in the error message.
Also, the first line of the commit message should be at most 80 chars.

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1135718835)
This is a demo binary, so I think the point is to show that the `chainman` can now be instantiated with a `chainparams` that is itself instantiated without directly relying on `gArgs`. I'm not sure if this really makes sense though.

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1135725988)
Same as https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1042464406

💬 kiminuo commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1468344458)
@0xB10C

> > > Could a tracepoints approach, similar to #26531, work?
> >
> >
> > I need to research that in detail but it looks promising. Thank you for sharing it here!
>
> I don't think tracepoints are suited for this use-case. Tracepoints allow you to react to a specific event. Here, you request / pull data when a consumer needs it.

Tracepoints would be nice if I need to compute the histograms at specified intervals and fast (to avoid getting complete mempool from a full node).
...

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1468348476)
Updated 45048c0d7e0ee12210d226239fb5ce63d9ed4441 -> 600ab02bf58e073a93936438a7e884b3a7110f1c ([tc/2022-09-libbitcoinkernel-chainparams-args_6](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_6) -> [tc/2022-09-libbitcoinkernel-chainparams-args_7](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_6..tc/202
...

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1135765960)
Could use [`std::visit`](https://en.cppreference.com/w/cpp/utility/variant/visit).

💬 ishaanam commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1135849437)
'assert_raises_rpc_error' could be used here, but wouldn't that introduce another race condition where this assertion will fail if the rescan has already been completed?

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1135857860)
ah that makes sense. thanks

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1135870886)
re: https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1135671989

> This is a demo binary, so I think the point is to show that the `chainman` can now be instantiated with a `chainparams` that is itself instantiated without directly relying on `gArgs`. I'm not sure if this really makes sense though.

IMO, it would be a little clearer if the vague "SETUP: Misc Globals" comment was changed to say specifically that SelectParams() call was needed to set some globals used by validation co
...

💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1135872769)
Right. Maybe just a `try`-`catch`-`pass`?

👍 ryanofsky approved a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
Code review ACK 600ab02bf58e073a93936438a7e884b3a7110f1c. Only changes since last review were rebase, and making suggested changes to make the split commit move only

💬 pablomartin4btc commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1135879724)
fixed

💬 pablomartin4btc commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1468452555)
> ACK [0fe610d](https://github.com/bitcoin/bitcoin/commit/0fe610deba336f0370d68c130dc9a223b7db964e) modulo the dangling closing parenthesis in the error message.
> Also, the first line of the commit message should be at most 80 chars.

I've corrected them both. Thanks!

💬 pinheadmz commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1468528728)
I couldn't reproduce this issue on macOS, here's what I tried:

- remove bitcoin plist file
- open bitcoin-qt with `-regtest`, setup wizard pops up, enter custom non-default path
- close bitcoin
- add `bitcoin.conf` file at custom path including the line `blocksdir=/tmp/blocks-test`
- restart bitcoin-qt

the result was expected, bitcoin used the specified blocks directory.

💬 Empact commented on pull request "refactor: Split logging utilities from system.h":
(https://github.com/bitcoin/bitcoin/pull/27238#issuecomment-1468541697)
Thanks @TheCharlatan for picking this up. 👍

📝 dergoegge opened a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
We should define clear interfaces between CNode, CConnman and PeerManager. This PR makes a small step in that direction by ending the friendship of CNode, CConnman and ConnmanTestMsg. CNode's message processing queue is made private in the process and it's mutex is turned in to a non-recursive mutex.

💬 dergoegge commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#discussion_r1135979252)
#27257 dedupes this code across the entire code base, reviewing that would help move this forward.

💬 MarcoFalke commented on pull request "Remove almost all blockstorage globals":
(https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1135983875)
> there is little benefit ...

Thanks, done.