💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3151434723)
> Can find a similar workflow in https://github.com/bitcoin/bitcoin/pull/26966 for the block filter index: https://github.com/bitcoin/bitcoin/commit/fa4348463dc44099ef1fdccb919056a0be44b97d. Which had comparable constraints due to the filter headers’ chain structure (each header references its predecessor).

Ah, I see, cool, I didn't know this PR went that far.

💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#discussion_r2251982022)
with [e79c50c](https://github.com/bitcoin/bitcoin/commit/e79c50cea2096e4d4bf018d2672b94d8c2bc959a) updated the `node.h` and added `snapshot.h` to the `/interfaces`.
Also updated `interfaces.cpp` with a `SnapshotImpl` class to process the path and extracts the metada and activate the snapshot.
@ryanofsky let me know if that's what you had in mind?

🤔 jsarenik reviewed a pull request: "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee"
(https://github.com/bitcoin/bitcoin/pull/33106#pullrequestreview-3084890435)
Looks good to me. Thanks!

💬 jsarenik commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2251956220)
Thanks for keeping it clear and obvious stil!

💬 jsarenik commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2251953284)
Even though this was clean in the code before, there were (and maybe still are) historically some wallets which divided by 1024 rather than 1000 and so were never able to pay really the minimum fee that already made the default nodes relay the transaction.

💬 ryanofsky commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#discussion_r2252027721)
re: https://github.com/bitcoin/bitcoin/pull/33117#discussion_r2249012242

Thanks! I think b464f388197c5c31937d309d817d38c4b4849a91 is an improvement over previous de11e3b89f8aba9a90a3419086807e5fee0f941d because it passes an `fs::path` argument from the GUI to the node instead of an `AutoFile` file argument. This is nice because it lets node be fully responsible for parsing the snapshot file instead of relying on the GUI.

One comment on b464f388197c5c31937d309d817d38c4b4849a91 is that it lo
...

✅ ryanofsky closed a pull request: "wallet: Avoid potentially writing incorrect best block locator"
(https://github.com/bitcoin/bitcoin/pull/29652)

💬 ryanofsky commented on pull request "wallet: Avoid potentially writing incorrect best block locator":
(https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-3151665095)
re: https://github.com/bitcoin/bitcoin/pull/29652#pullrequestreview-3084332144

> Since #30221 got merged, I think this one can be closed

Thanks! #30221 avoids the issue of wallets potentially writing the incorrect locators on flush notifications by just ignoring them and flushing in a less coordinated way. So the main concern of this PR is no longer an issue and it makes sense to close.

I still think the changes here dropping the `Chain::getTipLocator` and `Chain::getActiveChainLocator`
...

🤔 jonatack reviewed a pull request: "refactor, index: Remove member variables in coinstatsindex"
(https://github.com/bitcoin/bitcoin/pull/33134#pullrequestreview-3085116698)
Review ACK a53063a6d93103926509d3f95dc791140aae58c6

Nice cleanup.

🤔 janb84 reviewed a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3085116958)
NIT; Please correct the commit message to better describe the change.

The description "This has been incorrect since a3789c700b5a43efd4b366b4241ae840d63f2349
(v25; master since Sept. 2022). Noticed while setting up monitoring
using getpeerinfo." isn't descriptive of the changes in the commit.

"Commit messages should be helpful to people reading your code in the future, so explain the reasoning for your decisions.
[commit messages](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBU
...

💬 darosior commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2252176492)
Done.

I somewhat disagree that this is already covered though, the rationale given in txdownloadman refers to an outdated reason (wtxid relay) rather than the contemporary reason for `WITNESS_STRIPPED`'s existence (1p1c through orphan resolution). But improvements to existing code documentation can be done separately.

💬 ryanofsky commented on pull request "wallet: remove use of GetWalletDir and weakly_canonical when creating backup_prefix":
(https://github.com/bitcoin/bitcoin/pull/33122#issuecomment-3151760116)
I like the test simplification in fa9f30f509ed0ba6e4afdcf98775acb4e8c9e461 but think it would be best to drop commit d1b71e5846e125f59cd60468f2d76c9721b1dc06 for now since @davidgumberg disagreed with this approach in https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3033891866 it wouldn't be good to override his preference [while he's out](https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3133523088).

I do think d1b71e5846e125f59cd60468f2d76c9721b1dc06 is a minor improvemen
...

💬 theStack commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3151811168)
post-merge ACK c0642e558a02319ade33dc1014e7ae981663ea46

🚀 glozow merged a pull request: "test: Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060)

🚀 glozow merged a pull request: "init: make `-blockmaxweight` startup option debug only"
(https://github.com/bitcoin/bitcoin/pull/32654)

💬 brunoerg commented on pull request "test: Slay BnB Mutants":
(https://github.com/bitcoin/bitcoin/pull/33060#issuecomment-3151857157)
post merge ACK a3cf623364e84819bc16fd407b80d8dba46bbcb5

Nice!

💬 kevkevinpal commented on pull request "wallet: remove use of GetWalletDir and weakly_canonical when creating backup_prefix":
(https://github.com/bitcoin/bitcoin/pull/33122#issuecomment-3151863262)
> 6a7c0d3

Thanks! I did not see that comment

I dropped that commit and pushed [6a7c0d3](https://github.com/bitcoin/bitcoin/pull/33122/commits/6a7c0d3f874942a460bf5b13a107a2b21eb2e572) to just include the functional test change for this PR!

🤔 janb84 reviewed a pull request: "refactor: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/33116#pullrequestreview-3085204457)
Concept ACK 92c5e147d9445e126f86a08538200d1f6a390efd

This PR is the last pr of the Txid type safety refactor initiative. In this PR `transaction_identifier.h` is moved to primitives to finish the refactor and `uint256` to `Txid` (where relevant) along with any necessary explicit conversions. Which seems all correct.

During my code review, I have found some optional NITS regarding the use of variable name "hash". Is this still a descriptive name after this refactor?

Steps undertaken:
...

💬 janb84 commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2252193904)
NIT; is the variable name still descriptive ? would something like txId not be better suited ?

```suggestion
auto txid{Txid::FromUint256(ParseHashV(request.params[0], "parameter 1"))};
```

💬 janb84 commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2252251215)
NIT; is the variable name still descriptive ? would something like txId not be better suited ?
```suggestion
auto txid{Txid::FromHex(hashStr)};
```