💬 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

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

```suggestion
// Copyright (c) 2025-present The Bitcoin Core developers
```

🤔 w0xlt reviewed a pull request: "wallet, sqlite: Encapsulate SQLite statements in a RAII class"
(https://github.com/bitcoin/bitcoin/pull/33033#pullrequestreview-3049382506)
Looks good to me

ACK https://github.com/bitcoin/bitcoin/pull/33033/commits/b2c824a3ce0877f2cb7cbed1731088ed8ba6171a

💬 achow101 commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2226917021)
In f26bf5fd24ffed501dfdb3528c56cfaee0203a72 "scripted-diff: refactor: rename CWallet::LoadWallet"

I think we could actually drop the `PopulateWalletFromDB` here and in other test setups that use `MockableDatabase` since there's nothing in the databases anyways to load. These calls should be no-ops.

💬 tnndbtc commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-3111442116)
Since it happened again, should we consider adding more error/exception handlings on `subprocess_close`? Currently we assume it's always successful, but it may return -1 as error code.

🤔 l0rinc requested changes to a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3045078110)
Concept ACK, I'm not sure about the first change but the rest make sense.

reviewed f4b33a14f27bb3ee734e8d4eb9c2b2ccaa5df52a, left some comments for most commits, my main concern is the commit where we're reusing the headers vector, making the code significantly more contrived.
It would also be great if we could extract the common parts of this and @hodlinator's change and merge that (or his change) before we do this.

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226896908)
It was already hard to follow this stateful test, now it's even more so - could we maybe cherry-pick @hodlinator's cleanup commit before of this change?

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226805065)
Hmm, so now we have a dedicated return value called "ProcessingResult" and it won't contain the processing results, but the input parameter will be repurposed for that?

I understand that it's a lot of data to move or copy, but I don't find the result to be natural at all - see how awkward and stateful the tests have became, it's often a good sign that the interface isn't intuitive.

Given that `MAX_HEADERS_RESULTS = 2000` we're optimizing for 160kb of memory here? I agree that that's not no
...

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226911674)
hmmm, shouldn't `ValidateAndStoreHeadersCommitments` be responsible for this? The state is spread out all over now, I have a hard time following

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2223964122)
"populate" doesn't hint at the precidition state: should it be empty or do we just erase it instantly?

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226945830)
`const std:::span` in a header definition doesn't make a lot of sense:
```suggestion
bool HasValidProofOfWork(std::span<const CBlockHeader> headers, const Consensus::Params& consensusParams);
```

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226940519)
so if `already_validated_work` is `false` and `IsAncestorOfBestHeaderOrTip` returns `true`, we update `already_validated_work` to `true` - we could simplify it to:
```suggestion
already_validated_work = already_validated_work || IsAncestorOfBestHeaderOrTip(last_received_header);
```

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226956719)
Not sure why we're modifying this method in this commit, it's not strictly needed for the `GetBitsProof` change.

nit:
```suggestion
bool HasValidProofOfWork(std::span<const CBlockHeader> headers, const Consensus::Params& consensusParams)
```