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

Because using external signers is one of the best security options for users who run their Bitcoin Core software on Windows.

> post-merge Concept NACK.

Better alternatives are welcome.

📝 ekzyis opened a pull request: "Use string interpolation for default value of -listen"
(https://github.com/bitcoin/bitcoin/pull/27232)
This is a refactoring change. So I have read the following and will try to answer why this change should be accepted

> * Refactoring changes are only accepted if they are required for a feature or
bug fix or **_otherwise improve developer experience significantly_**. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bu
...

💬 MarcoFalke commented on issue "Blockchain sync - highly variable time to connect trasactions":
(https://github.com/bitcoin/bitcoin/issues/21722#issuecomment-1461695573)
Closing for now, but please let us know if there are more details available.

✅ MarcoFalke closed an issue: "Blockchain sync - highly variable time to connect trasactions"
(https://github.com/bitcoin/bitcoin/issues/21722)

💬 ekzyis commented on pull request "p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`":
(https://github.com/bitcoin/bitcoin/pull/26899#discussion_r1130744881)
Added a [PR](https://github.com/bitcoin/bitcoin/pull/27232) if you want to take a look :)

💬 MarcoFalke commented on pull request "test: skip all backward compatibility tests under valgrind":
(https://github.com/bitcoin/bitcoin/pull/27228#discussion_r1130751422)
> Conversely, since some tests do work, maybe someone wants to enable one later.

If you want to make it more fine grained, it could make sense to move the check to the test_node and condition it on the version number?

> BDB

Good point. I also can't run `--valgrind` with BDB on my system. Which raises the question how many settings we want `--valgrind` to be supported under.

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