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

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226950075)
```suggestion
/** Compute how much work an nBits value corresponds to. */
```

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2226956422)
If we're changing it, we might as well modernize it to:
```suggestion
return std::ranges::all_of(headers,
```

💬 ajtowns commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2227051044)
Returning a pointer from an anonymous function with no explicit return type seems safer and easier to follow than returning a reference? Not sure what's weird about having the Assert outside the lock. We have a similar instance already:

https://github.com/bitcoin/bitcoin/blob/eb137184482cea8a2a7cdfcd3c73ec6ae5fdd0d6/src/node/blockstorage.cpp#L935

💬 ajtowns commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3111631541)
> One example where the opportunistic disconnection could be useful is in the case of a hardfork coin

In that case you'll already get a network split when the hard fork side mines a block.

> It doesn't help in adversarial situations, but may still avoid wasting resources on innocently misbehaving peers. With this PR, opportunistically detecting such peers and evicting them comes at virtually no cost (unlike the current situation).

Disconnecting peers is only useful in adversarial situat
...

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2227082312)
I prefer validating closely to the source, but I don't mind either way, thanks for the comment

🤔 w0xlt reviewed a pull request: "test: check proper OP_2ROT behavior"
(https://github.com/bitcoin/bitcoin/pull/33047#pullrequestreview-3049651677)
ACK https://github.com/bitcoin/bitcoin/pull/33047/commits/b94c6356a29b03def6337c91caabb3b8642187e8

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3111683386)
> @HowHsu, are you going to include the test or a patch to reproduce the issue? I’m happy to ACK it either way.

Hi furszy, I thought the test I put already demonstrate the issue, though it is not proper to be included to the codebase.

🤔 pablomartin4btc reviewed a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-3049728359)
tACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77

I've re-[tested](https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-2964023741) the relative path cases and the plainfile [issue](https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-2819784017) has been fixed.

Regarding the usage of `fs::weakly_canonical` vs `lexically_normal()`, I agree with @ryanofsky, I think that resource cost-wise, to "access" the filesystem in order to validate the name that we'll use for the backup fi
...