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

💬 fanquake commented on issue "qa: Intermittent failure in `feature_fee_estimation.py`":
(https://github.com/bitcoin/bitcoin/issues/23165#issuecomment-1462113559)
https://cirrus-ci.com/task/6452772573282304?logs=ci#L3336
```bash
test 2023-03-09T10:13:27.038000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 134, in main
self.run_test()
File "/tmp/cir
...

💬 brunoerg commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1462131721)
that was just a question, thinking about complexity I agree on leaving it as is.

📝 MarcoFalke opened a pull request: "refactor: Replace GetTimeMicros by SystemClock"
(https://github.com/bitcoin/bitcoin/pull/27233)
It is unclear from the name that `GetTimeMicros` returns the system time. Also, it is not using the type-safe `std::chrono` types.

Fix both issues by replacing it with `SystemClock` in the only place it is used.

This refactor should not change behavior.

💬 brunoerg commented on pull request "p2p: return `CSubNet` in `LookupSubNet`":
(https://github.com/bitcoin/bitcoin/pull/26078#issuecomment-1462186854)
Rebased

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1462197743)
re: https://github.com/bitcoin/bitcoin/pull/26177#pullrequestreview-1332732743

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

Thank you for the suggestions. I cleaned the description.

⚠️ ryanofsky opened an issue: "Permission to comment on closed PRs"
(https://github.com/bitcoin/bitcoin/issues/27234)
I noticed recently I couldn't comment on two closed PRs. One is #27103 which was closed without an explanation. I wanted to ask why it was closed and whether #27125 might be a replacement for it. The other was my own PR #18608, which I wanted to reopen, but couldn't (even though the branch commit was unchanged at the time), so I opened another PR #27224, and now I can't comment on #18608 to say that it's been replaced by #27224.

I'm guessing the restrictions on comments are an anti-spam measu
...