💬 MarcoFalke commented on issue "Many sendcmpct messages are sent during UpdateActiveChain()":
(https://github.com/bitcoin/bitcoin/issues/21903#issuecomment-1461683306)
The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest and direction. Pull requests with improvements are always welcome.

✅ MarcoFalke closed an issue: "Many sendcmpct messages are sent during UpdateActiveChain()"
(https://github.com/bitcoin/bitcoin/issues/21903)

💬 MarcoFalke commented on issue "Mac OS latest Bitcoin core latest release will not run as Mac OS, "you can't open the application 'Bitcoin core'"":
(https://github.com/bitcoin/bitcoin/issues/25834#issuecomment-1461685951)
No, I think the error message is different. But there is no further replies from OP available, so closing for now.

Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?

✅ MarcoFalke closed an issue: "Mac OS latest Bitcoin core latest release will not run as Mac OS, "you can't open the application 'Bitcoin core'""
(https://github.com/bitcoin/bitcoin/issues/25834)

💬 MarcoFalke commented on issue ""error reading from database. shutting down"":
(https://github.com/bitcoin/bitcoin/issues/22426#issuecomment-1461689356)
Again apologies for the slow turn-around. Closing for now, but if you have any more details or steps to reproduce, ideally on a fresh install of the operating system, please provide them here or in a new issue.

✅ MarcoFalke closed an issue: ""error reading from database. shutting down""
(https://github.com/bitcoin/bitcoin/issues/22426)

💬 fanquake commented on pull request "Update src/secp256k1 subtree to upstream release v0.3.0":
(https://github.com/bitcoin/bitcoin/pull/27230#issuecomment-1461690983)
Concept ACK

cc @jonasnick too

💬 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");
```