💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129442167)
nit: I know it was using `int` to index the array, but those should be `size_t`. Now looks like a good time to fix that:
```suggestion
int AddrManImpl::LookupAddrmanEntry(bool use_tried, size_t bucket, size_t position) const
```

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129522848)
`std::string` will do heap allocation. It is an overkill in this case.

```suggestion
LogPrint(BCLog::ADDRMAN, "Selected %s from %s\n", info.ToStringAddrPort(), search_tried ? "tried" : "new");
```

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129439158)
Would be good to have an assert that checks there is no out-of-bounds access. This method itself does not know what the callers will use for arguments or how they will be derived so better check.

Maybe change the arrays to `std::array` and use `.at()` here.

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129539099)
nit: use `std::nullopt` instead of `{}` which I believe is more readable and consistent with the comment that says "nullopt = all". Like here:

https://github.com/bitcoin/bitcoin/blob/8d12127a9c19cb218d661a88ab9b6871c9d853b9/src/addrman.h#L109

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129547711)
nit: I think it is good to be explicit and use `network.has_value()` here. Otherwise it can get confusing, especially with booleans (`network` is not boolean, but anyway). For example:

```cpp
std::optional<bool> is_odd
...
if (is_odd) { // did the author mean is_odd.has_value() or *is_odd?
```

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129568636)
nit:
```suggestion
// Loop through the addrman table until we find an appropriate entry
```

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129552332)
nit:

```suggestion
if (new_count + tried_count == 0) return {};
```

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129569710)
nit: same as elsewhere: consider `network.has_value()`.

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129566625)
This maybe warrants a better description:

```suggestion
* @param[in] new_only Whether to only select addresses from the new table. Passing `true` guarantees either an address from the new table or an invalid return value. Passing `false` requests 50% chance of new or tried, it does not guarantee an entry from the tried table.
```

💬 willcl-ark commented on issue "RPC: conf var rpcallowip=::/0 stopped working when upgrading to 0.21":
(https://github.com/bitcoin/bitcoin/issues/21070#issuecomment-1461843520)
@MarcoFalke should this be closed now that the old behaviour is buried under 3 releases?

Although, perhaps we keep it open as I don't see any test which would check for regression of this behaviour...

💬 furszy commented on pull request "gui: use the stored CSubNet entry when unbanning":
(https://github.com/bitcoin-core/gui/pull/717#discussion_r1130922428)
to enable it, just need to set the table selection mode to multi selection. e.g. `peerTable->setSelectionMode(QAbstractItemView::MultiSelection);`.

I didn't mention it because the PR is fine, it is not changing behavior.

👍 vasild approved a pull request: "Use string interpolation for default value of -listen"
(https://github.com/bitcoin/bitcoin/pull/27232)
ACK 5c938e74cfdf6b89c6c4d5cce2fd07cbfc8b29c2

👍 ryanofsky approved a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
Code review ACK a7afa98ca38fd66fc69b77b95a6a57d50207911b. This looks great, and much cleaner and simpler than the starting point. I'd encourage @ajtowns and anybody else who looked at this previously to have another look.

I think that one remaining drawback is that the PR description is very verbose. I think the PR could be described in a few sentences as:

* Move `CChainParams` class definition into the kernel, and give it new factory functions `CChainParams::{RegTest,Signet,Main,TestNet}`
...

👍 vasild approved a pull request: "p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`"
(https://github.com/bitcoin/bitcoin/pull/26899)
ACK 928ff360ebc4b76db65b8ad5bc688f4404b32dec

This change, by itself is an improvement and I am ok to get it merged and move on.

However, can we do better? This PR does not change the behavior of `-maxconnections=0 -listen=1`, but lets change that too! :) It does not make sense and probably means that the user does not know what they are doing.

What about either giving an error on `-maxconnections=0 -listen=1` or disallowing low values for `maxconnections`?

My request is kind of [feat
...

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

> There is no patch done yet introducing the kernel/chainparams.cpp/h into the kernel namespace. Such a change would be very intrusive to the entire codebase and hard to review as part of this patch set. Due to its intrusiveness, I am not sure whether this is the correct path forward or not.

I think this PR is doing the right thing. If you move code into the kernel incrementally, and then set the namespace at the end after
...

💬 ismaelsadeeq commented on issue "Run functional tests from make check":
(https://github.com/bitcoin/bitcoin/issues/18816#issuecomment-1462025695)
Hello! I noticed this issue and would like to contribute by enabling running the tests in ./test/functional/test_runner.py with `make check-functional`. My plan is to also add a `make check-all` target to run both `make check` and `make check-functional` tests.
Any suggestions on how to make the solution even better, I'm all ears! Thank you so much for your time and help.

💬 stratospher commented on pull request "Use string interpolation for default value of -listen":
(https://github.com/bitcoin/bitcoin/pull/27232#issuecomment-1462084589)
ACK 5c938e7.

💬 mzumsande commented on pull request "gui: use the stored CSubNet entry when unbanning":
(https://github.com/bitcoin-core/gui/pull/717#discussion_r1131045610)
Agree, no need to change it here, I just mentioned it because I found it a bit strange and maybe someone would be interested to change it in a follow-up.

💬 MarcoFalke commented on issue "RPC: conf var rpcallowip=::/0 stopped working when upgrading to 0.21":
(https://github.com/bitcoin/bitcoin/issues/21070#issuecomment-1462088325)
Maybe the docs or a warning can be improved, see the previous comment: https://github.com/bitcoin/bitcoin/issues/21070#issuecomment-899098587

🚀 hebasto merged a pull request: "Use the stored CSubNet entry when unbanning"
(https://github.com/bitcoin-core/gui/pull/717)

💬 hebasto commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1462108897)
> @mzumsande, it is possible to avoid calling `LookupSubNet()` from the GUI, see [bitcoin-core/gui#717](https://github.com/bitcoin-core/gui/pull/717). If that gets merged then this PR will be simplified - no need to touch the GUI code from here.

https://github.com/bitcoin-core/gui/pull/717 has just been merged.