💬 achow101 commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3110226273)
> nit: Commit [3662960](https://github.com/bitcoin/bitcoin/commit/3662960470d74850008b7b48b6820d02002f7a5d) appears to have CI failures.

I believe that's only because the tasks were canceled by a new push.

💬 Karim8090 commented on issue "guix: Zip file non-determinism when building in WSL":
(https://github.com/bitcoin/bitcoin/issues/32931#issuecomment-3110253591)
Babu8090

🤔 w0xlt reviewed a pull request: "wallet: Set descriptor cache upgraded flag for migrated wallets"
(https://github.com/bitcoin/bitcoin/pull/33031#pullrequestreview-3049159589)
ACK https://github.com/bitcoin/bitcoin/pull/33031/commits/4e5faaba82b4e1f42edb3a52270ea9086c1edfa9

🤔 mzumsande reviewed a pull request: "net: Fix Discover() not running when using -bind=0.0.0.0:port"
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3049008730)
Does the added test succeed for others? I tried to follow the instructions and get

```
test 2025-07-23T21:50:01.787971Z TestFramework (ERROR): Address 1.1.1.1 not found in node0's local addresses: []
test 2025-07-23T21:50:01.788044Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "XXX/bitcoin/32757_bind0000/test/functional/test_framework/test_framework.py", line 195, in main

...

💬 mzumsande commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2226651425)
unrelated to the changes here, but `ifconfig` is deprecated, so this could be updated.

💬 pinheadmz commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3110456945)
Test worked for me on mac and failed as expected without 1.1.1.1 and 2.2.2.2

On my system the command was
`ifconfig en0 alias 1.1.1.1`

💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3110475043)
Maybe the intent of my comment wasn’t clear. We are seeing that blocks including lower feerate transactions take significantly longer to get propagated, and the way I’m reading the sentiment here, a lot of people do not share my concern over a potential DoS protection reduction. In so far, it seems that there is at least interest in this PR being further developed. My suggestion is that if someone were to address the review feedback here, they should probably lower all three settings, not just t
...

💬 achow101 commented on pull request "doc: update headers and TOC in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3110481836)
I would prefer to drop the table of contents entirely.

I pretty much only ever read `.md` files on GitHub or with `mdcat`. On GitHub, the table of contents will be automatically generated, and with `mdcat`, the links don't work anyways.

🤔 stickies-v reviewed a pull request: "log: rate limiting followups"
(https://github.com/bitcoin/bitcoin/pull/33011#pullrequestreview-3046739749)
code lgtm 8aa2726f1cfbc1e4267a796cb000c4eeae5910f5 - but would like to see if we can make improvements on the `m_limiter` lifetime guarantees, and on the `logging_filesize_rate_limit` tests. Both are not essential, but I think it could make sense to include them here.

💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225128347)
nit on 35585c54b57bbb05bfc4de63880c1767931e3696: more consistent parameterise both:

<details>
<summary>git diff on d0b3a80b7f</summary>

```diff
diff --git a/src/init.cpp b/src/init.cpp
index 63244c802e..863b2e088e 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1382,7 +1382,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
LogInstance().SetRateLimiting(std::make_unique<BCLog::LogRateLimiter>(
[&scheduler](auto func, auto window) {
...

💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225171353)
nit: using descriptive names might make more sense, e.g. `INFO_1`, `INFO_2`, `DEBUG`, `INFO_NOLIMIT`

💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225484458)
Note: this is already handled by the `LogSetup` fixture, so I think we can remove this logic here.

On that note: I think `LogSetup` could benefit from explicitly setting `LogInstance().SetRateLimiting(nullptr);` in both its constructor and destructor?

💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225184386)
nit: I find setting `line_length{1024}` and documenting the subtraction more straightforward:

<details>
<summary>git diff on d0b3a80b7f</summary>

```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 81c0bacba0..245940f284 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -414,11 +414,10 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
bool prev_log_threadnames{LogInstance().m_log_threadnames};
LogIns
...

💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2226805223)
The test has definitely improved in both functionality and readability, but I find it's still quite clunky (e.g. lots of repeated code) and unspecific (e.g. using any increase in filesize as measurement, not accurately checking the (non) existence of certain log statements most of the time), so I dove into a little rabbit hole today to see what my ideal test would look like, and I think it'd be close to something like this: https://github.com/stickies-v/bitcoin/commit/8aa2726f1cfbc1e4267a796cb00
...

💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225156552)
nit: `auto` can sometimes make things less readable, but I think in all of these changes that's not the case. Repeating types (e.g. `std::source_location source_loc_1{std::source_location::current()}` is just overly verbose imo), and for the others the name/literal is already explanatory. For `sched_func`, I think the logic is now drowned in rather unimportant type definitions. I think this addresses [this comment](https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2188256330), but I pers
...

💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2226684372)
There are some lifetime concerns wrt `scheduler_func`, that are okay in our current codebase, but make the contract perhaps more fragile than it needs to be. Specifically: `Logger` has a `std::unique_ptr<LogRateLimiter> m_limiter` member. The node's scheduler holds [a reference](https://github.com/bitcoin/bitcoin/blob/eb137184482cea8a2a7cdfcd3c73ec6ae5fdd0d6/src/logging.cpp#L379) to the limiter. This means that the limiter may not be destroyed before the scheduler is stopped. In our current code
...

💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2225276727)
nit: use variable
```suggestion
MockForwardAndSync(scheduler, time_window);
```

💬 l0rinc commented on pull request "doc: update headers and TOC in `developer-notes.md`":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3110641027)
I'm fine with that option as well, though it's not the only doc with a TOC:
* https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md *
* https://github.com/bitcoin/bitcoin/blob/master/doc/design/multiprocess.md *
* https://github.com/bitcoin/bitcoin/blob/master/doc/files.md

The first two are also missing headers from the TOC - the last one seems up-to-date.
So I agree - we either update them all or delete the TOC in all 3 docs.

🤔 jonatack reviewed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3049290299)
Began reviewing, then realized that I was reviewing the same code twice with the two "modernize" commits following "original" commits.

Propose to either (1) make clear in the commit message where the code is being pulled from, and that it will be re-styled afterward, or (2) preferably squash the modernization commits into the previous ones, thereby easing life a bit for your reviewers :)

💬 jonatack commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2226846994)
535daaf15fd754335116d17833a45261cdff4e93 nit, sort