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

📝 fanquake unlocked a pull request: "refactor: Remove CAddressBookData::destdata"
(https://github.com/bitcoin/bitcoin/pull/18608)
**This is based on #21353.** The non-base commits are:

- [`7a05b1dee2f` refactor: Remove CAddressBookData::destdata](https://github.com/bitcoin/bitcoin/pull/18608/commits/7a05b1dee2fa68b32bfb19e273fb55a5b3836a3e)

---

This is cleanup that doesn't change external behavior.

- Removes awkward `StringMap` intermediate representation
- Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code
- Deals with destination "used" keys in walletdb.
...

💬 MarcoFalke commented on issue "Permission to comment on closed PRs":
(https://github.com/bitcoin/bitcoin/issues/27234#issuecomment-1462229567)
I think you can just leave a comment on the open pull to refer to the closed one. Generally, after a long time (1 year) of being closed the discussion is locked to avoid notification of people who left the project or otherwise open the door widely for drive-by spammers.

If you really need or want to comment on a locked pull, my recommendation would be to unlock it first. Otherwise there is no way for someone else to reply.

💬 dergoegge commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#issuecomment-1462236593)
Concept ACK

📝 fanquake unlocked a pull request: "Blockstorage: Dont access gArgs to get blocks_dir"
(https://github.com/bitcoin/bitcoin/pull/27103)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 fanquake commented on issue "Permission to comment on closed PRs":
(https://github.com/bitcoin/bitcoin/issues/27234#issuecomment-1462228887)
Yea. Threads are locked to combat spam (and/or when it's clear that the PRs are accidental, wrong repo etc). I've unlocked both now, if you'd like to comment. We can look at if there is some permissions change we can make.

💬 dhruv commented on pull request "BIP324: Enable v2 P2P encrypted transport":
(https://github.com/bitcoin/bitcoin/pull/24545#issuecomment-1462232617)
@vostrnad this is expected because the breaking change is in the p2p layer and not the transport. So a v2 connection is successfully established, and likely the inbound peer (not updated) never sees something that it interprets as the VERSION message. It should have timed out/failed eventually though. Did it do that? This isn't currently considered a downgrade scenario because a v2 transport connection was established. If we downgrade to v1 on p2p unresponsiveness, my thinking is that would lead
...