💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2226706401)
In theory we could change `uint256`'s `value_type`, and we wouldn't need to make any changes here in that case.

🤔 w0xlt reviewed a pull request: "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt"
(https://github.com/bitcoin/bitcoin/pull/32895#pullrequestreview-3049112073)
ACK https://github.com/bitcoin/bitcoin/pull/32895/commits/b0470f573735a04ebc87ca5d3e00a73e3f10fee1

nit: Commit https://github.com/bitcoin/bitcoin/pull/32895/commits/3662960470d74850008b7b48b6820d02002f7a5d appears to have CI failures.

🤔 w0xlt reviewed a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3049114357)
reACK https://github.com/bitcoin/bitcoin/pull/32984/commits/060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542

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