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

💬 ajtowns commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2227177932)
Could consider adding something like:

```c++
// SCRIPT_BASE_SIZE should be set to avoid wasting space in padding
BOOST_CHECK(sizeof(CScriptBase) != sizeof(prevector<SCRIPT_BASE_SIZE+1, uint8_t>));
```

It might be nicer to have prevector include a `static constexpr size_t STATIC_SIZE = N;` member, and use `CScriptBase::STATIC_SIZE` rather than `SCRIPT_BASE_SIZE`.

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

On a side note: should we not check the wallet chain before executing the migration on it instead of letting it go that far?

💬 ajtowns commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3111757079)
> I haven't investigated in detail why there are so many empty prevectors, can very well be just an unoptimized debug run artifact of all the Coin copies.

I think it's just that coins get cleared when they're spent, so end up as 0 size by the time they're destructed? Empty scriptSig's for witness spends might also have some impact.

I think the tradeoff is something like:

- spends of p2pk, p2sh, p2pkh coins -- these cost 8 more bytes
- spends of p2wpkh -- these cost 16 more bytes (sPK
...

💬 ajtowns commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3111760925)
> To show the exact behavior, I've plotted the height-vs-time (and the coins-vs-time) before and after:

Probably would be better to bucket the heights and do bar charts, ie: how long did the 1st, 2nd, 3rd, etc 100k blocks take with both approaches. That way it's easier to see if a large gain for this PR was temporary (eg, due to lots of inscriptions) and is not providing a benefit with current traffic.