💬 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 :)

💬 1440000bytes commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1461538006)
Concept ACK

💬 vasild commented on pull request "gui: use the stored CSubNet entry when unbanning":
(https://github.com/bitcoin-core/gui/pull/717#discussion_r1130639712)
Yes, I noticed the same - looks like we don't need a loop here since it is always just one being selected. But it was like this and I left it as is (with a loop) to minimize the amount of unnecessary changes that can have unexpected adverse effects. I do not know why the GUI wouldn't let me select more than one row. Maybe a change elsewhere in the code would allow that and then this code would be broken without a loop.

👍 1440000bytes approved a pull request: "p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`"
(https://github.com/bitcoin/bitcoin/pull/26899)
utACK https://github.com/bitcoin/bitcoin/pull/26899/commits/4e9d06bf22da5e972c541b61036ffda73a51a114

💬 willcl-ark commented on pull request "doc: docment json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#issuecomment-1461577699)
> Nit - there is typo in commit message / PR title (s/docment/document/).

Thanks, i've updated this in 65e3abcbf

💬 MarcoFalke commented on pull request "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1130705346)
```suggestion
for (const auto& c:categories.get_array().getValues()) {
const std::string& c{c.get_str()};
```

no need to create three copies of the same thing

💬 kristapsk commented on pull request "doc: docment json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#issuecomment-1461643692)
> > Nit - there is typo in commit message / PR title (s/docment/document/).
>
> Thanks, i've updated this in 65e3abcbf

You should also edit PR title. It is used by [github-merge.py](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/github-merge.py).

💬 fanquake commented on pull request "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1461655213)
> or none are passed, including not recognizing none:

Any idea when/how this was broken?

💬 willcl-ark commented on pull request "doc: document json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#issuecomment-1461659016)
>

Thanks, I thought that updated from the force push, but apparently not. Now also done :)

💬 fanquake commented on pull request "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1461666350)
Probably in f1379aeca9d3a8c4d3528de4d0af8298cb42fee4 from #25614:
> replace the unused BCLog::Level:None string "none" with an empty string
as the case will never be hit

So only broken for 24.x.