💬 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_r1130104615)
On this one, when I raise the error or when I have to pass the strings for the insertion into the chain, it doesn't look right to pass twice the same variable (e.g.: `INVALID_CLI_COMMAND_ARGUMENT.format(rpcwallet3, rpcwallet3, "-generate"`).

Would you accept:

`INVALID_CLI_COMMAND_ARGUMENT = 'Error: invalid cli-command argument. If \"{}\" is a valid option try passing it before the \"{}\" command.'`

💬 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_r1130114475)
fixed

💬 achow101 commented on pull request "Switch hardened derivation marker to h (in normalized descriptors and new wallets)":
(https://github.com/bitcoin/bitcoin/pull/26076#issuecomment-1460962371)
reACK 4677f1923c9509025de0e77892638d5013f044bf

📝 sipa opened a pull request: "Update src/secp256k1 subtree to upstream release v0.3.0"
(https://github.com/bitcoin/bitcoin/pull/27230)
This updates the libsecp256k1 subtree to v0.3.0. I don't believe there are code changes that are particularly important to Bitcoin Core, apart from the added CMake build system support.

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1460991593)
Thanks for the review,

Updated c44983faa02b4845cd7605abe99c332980a07af1 -> a7afa98ca38fd66fc69b77b95a6a57d50207911b ([tc/2022-09-libbitcoinkernel-chainparams-args_4](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_4) -> [tc/2022-09-libbitcoinkernel-chainparams-args_5](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-
...

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1130159494)
This makes sense, I wasn't connecting the fact that the util library includes `chainparamsbase` anyway with segregating the kernel code. I dropped the commit.

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1130159566)
Merging with the option seems more in tune with how the options are handled in other places in the code, so I went with that.

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1130159703)
I dropped this by mistake. Included now.

💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1461041818)
> I think it goes like this: (...)
>
> I think this is enough to ditch the idea of random-chance `Select()`. Also, maybe it would not be so much more simpler than this PR.

I agree with that conclusion, but I think you can't just add the probabilities like that. I think the correct formula is
$$p={k! S(n,k) \over k^n}$$
where k is the number of networks, $n=8$ the number of outbounds and $S(n,k)$ the Stirling number of the second kind, see [here](https://stats.stackexchange.com/question
...

💬 Xekyo commented on issue "Small unspent can get removed from OutputGroup for being uneconomical probably leading to later partial spending":
(https://github.com/bitcoin/bitcoin/issues/20287#issuecomment-1461138806)
I agree, that we still have this issue with potentially making a partial spend, when we have an uneconomical UTXO that’s part of a group. I think there may be a confusion here between dust and uneconomical UTXOs, though: in Bitcoin Core, dust pertains to an output whose value is less than `dustrelayfee × (input_weight + output_weight)` (where dustrelayfee is a feerate) with the default `dustrelayfee` being 3 ṩ/vB. So, dust outputs will always be dust (unless the dustrelayfee is changed). We only
...

📝 jonatack opened a pull request: "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs"
(https://github.com/bitcoin/bitcoin/pull/27231)
The logging RPC doesn't seem to behave as described in its help when `0` or `none` are passed, including not recognizing `none`:

```
$ ./src/bitcoin-cli logging '["none"]'
error code: -8
error message:
unknown logging category none
```

This pull adds unit and functional test coverage, the latter of which fails on master and passes on this branch, fixes the behavior as described in the help, and improves the documentation and examples.

💬 vostrnad commented on pull request "BIP324: Enable v2 P2P encrypted transport":
(https://github.com/bitcoin/bitcoin/pull/24545#issuecomment-1461152689)
After updating, node is stuck in a loop trying to connect to manually added nodes that haven't been updated yet, without downgrading to a v1 connection. It might be just fine, but I thought I'd mention it.

💬 achow101 commented on pull request "build: Re-enable external signer on Windows":
(https://github.com/bitcoin/bitcoin/pull/25696#issuecomment-1461152815)
ACK 1a0d8e178c7b9a3e94d14f94b77305802ebdc93b

🚀 achow101 merged a pull request: "build: Re-enable external signer on Windows"
(https://github.com/bitcoin/bitcoin/pull/25696)

👋 jonatack's pull request is ready for review: "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs"
(https://github.com/bitcoin/bitcoin/pull/27231)

💬 amitiuttarwar commented on pull request "p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`":
(https://github.com/bitcoin/bitcoin/pull/26899#issuecomment-1461221191)
are there situations where you'd recommend a node operator to use `-maxconnections=0` over `-connect=0` or `-noconnect`? I find the current behavior with `-maxconnections=0` to be odd how the node just stalls with very little guidance for the user.

I can't think of cases where I'd recommend an operator to set `-maxconnections` to be less than 8 or 10 for security reasons. If someone was seeking a lower bandwidth environment, I'd recommend disabling automatic connections & having manual truste
...

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1461229059)
to minimize the complexity that we need to maintain over time, we try to reduce the amount of client-side niftiness that we offer. it seems to me that the string would offer all the information needed for a client to write a simple query if they wanted the sum of multiple. agreed that the syntax is another challenge of the array type

so I am +1 to leaving as is

💬 fanquake commented on pull request "build: Re-enable external signer on Windows":
(https://github.com/bitcoin/bitcoin/pull/25696#issuecomment-1461495567)
post-merge Concept NACK. I don't know why we are leaning back into this.

💬 S3RK commented on pull request "wallet: 25806 follow-up":
(https://github.com/bitcoin/bitcoin/pull/27227#issuecomment-1461505650)
Code review ACK 475c20aa568d597c7850c784058596ae26f37496

💬 stratospher commented on pull request "p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`":
(https://github.com/bitcoin/bitcoin/pull/26899#discussion_r1130625017)
> Historical reasons and no one bothered yet to make this consistent with the other arguments?

most possible reason :)