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

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r2227646845)
It seems so. Will fix release note if I have to retouch.

💬 maflcko commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2227753763)
I don't understand this change. The commit message just says "util: Add value_type and span constructor to (W)Txid", but it doesn't say why or where this is needed. Compiling without this (locally) seems to pass.

Also, conceptually, it seems to go in the opposite direction that we want to go in. This makes implicit construction from any byte span to `transaction_identifier` possible again. The constructor `transaction_identifier(const uint256& wrapped)` is private and will still catch those cas
...

💬 maflcko commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2227638927)

encapsualtes -> encapsulates [spelling error in comment “encapsualtes” makes the word misspelled]

💬 HowHsu commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3112459168)
> > > @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 [#32878 (comment)](https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3045485948)? That link returns a 404.

Sorry for that, check this one: https://github.com/HowHsu/bitcoin/commit/e9
...

💬 maflcko commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2227792656)
> We still need to write the minversion record for newly created wallets so that they can be opened in older versions with the expected feature set.

I think I may be missing something, but I don't understand what feature set this is referring to. `CanSupportFeature()` is only called on legacy BDB wallets, but they can't open sqlite descriptor wallets anyway, so retaining the code here for compatibility does not seem needed.

It is fine to keep it for consistency or for documentation purposes, b
...

💬 maflcko commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2227804494)

unconfiremd -> unconfirmed [spelling error in “unconfiremd parents”]

💬 maflcko commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2227809802)
in the first commit (after the rebase): This is now required. Otherwise:

```
assert_equal(testres[0]["reject-reason"], "missing-inputs")
^^^^^^^^^^^^
NameError: name 'assert_equal' is not defined

💬 maflcko commented on pull request "[29.x] Backport #32521":
(https://github.com/bitcoin/bitcoin/pull/33013#issuecomment-3112537332)
the ci failure is unrelated. The fix was https://github.com/bitcoin/bitcoin/commit/cd8089c20baaaee329cbf7951195953a9f86d7cf, but that never made it into 29.x