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

💬 furszy commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3111850225)
> > @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.

Are you referring to the one you linked in https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3045485948? That link returns a 404.

🤔 pablomartin4btc reviewed a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3049872913)
ACK 319abd609b3d2194e7a05454b6af55b6f16adbf1

I like this refactoring — the general idea of reducing unnecessary `string` allocations via `string_view` makes sense.

I’m a bit unsure about the changes in `mining.cpp` and `signmessage.cpp`. It seems like we’re going from `string` → `string_view` → `string`, which introduces a bit of unnecessary churn and could lead to confusion down the line. I understand the intention may be to eventually update those functions to accept `string_view`(?), b
...

🤔 Ari4ka reviewed a pull request: "Including exception what() in Runaway dialog box"
(https://github.com/bitcoin-core/gui/pull/876#pullrequestreview-3049876903)
****

💬 nervana21 commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3111877679)
tACK [942d95d](https://github.com/bitcoin/bitcoin/pull/33026/commits/942d95d3c6c026d0e10a682c18e3a907a6241d8a)

I thoroughly reviewed and understood all code changes. I ran the tests locally and they all passed.

📝 nervana21 opened a pull request: "doc: Fix typos in asmap README"
(https://github.com/bitcoin/bitcoin/pull/33049)
This minor PR fixes some spelling mistakes found while reviewing #33026.

💬 maflcko commented on pull request "test: check proper OP_2ROT behavior":
(https://github.com/bitcoin/bitcoin/pull/33047#issuecomment-3112272988)
lgtm ACK b94c6356a29b03def6337c91caabb3b8642187e8

💬 maflcko commented on pull request "doc: update headers and remove manual TOCs":
(https://github.com/bitcoin/bitcoin/pull/33040#issuecomment-3112293137)
lgtm ACK ca38cf701dc635d39db987105c5b2ccc87fd9815 🌔

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK ca38cf701dc635d3
...

💬 ArmchairCryptologist commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3112315080)
> feerate estimation currently doesn’t do below 1 s/vB, so this PR might be more involved than it seems at first glance

IMHO it would probably be reasonable to stagger changes to wallet/fee estimation for one release, and only change the thresholds for mempool/relay and possibly mining initially. Based on some stats from my own nodes, I would assume that relaying sub-1 sat/vB transactions will still be spotty for some time after a new version with changed defaults is released.

Node 1: Of 1
...