💬 MarcoFalke commented on pull request "doc: update broken str util reference links on developer-notes":
(https://github.com/bitcoin/bitcoin/pull/27220#issuecomment-1461712699)
lgtm ACK da347de530242ead8f6a9718ee1440385bd3d44d

💬 fanquake commented on pull request "test: skip all backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#discussion_r1130757104)
> Which raises the question how many settings we want --valgrind to be supported under.

I thnk currently the only realistic place we can fully support Valgrind, is within the CI system, where things like BDB are working, and we can more easily control settings and suppressions. Outside of that, things may work fine, or may not at all, depending on the system where it is being run.

💬 1440000bytes commented on pull request "doc: document json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#discussion_r1130770078)
Why are the examples using signet port? I think we should use mainnet port (8332) same as all the [RPC examples](https://github.com/bitcoin/bitcoin/blob/23e2bfcbc42849daa8e2b69f9bbdc526bc8743a7/src/rpc/util.cpp#L160).

💬 willcl-ark commented on pull request "doc: document json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#discussion_r1130818747)
I'm happy to be persuaded either way here, no particular reason for using signet (other than I was using it for testing). Although I personally don't think it matters much as the comment for each shows which variables are being used

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129434505)
nit: `Addrman` in the method name is redundant. Maybe `AddrManImpl::LookupEntry()` or even `AddrManImpl::Lookup()` or `AddrManImpl::Get()`.

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