💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2121291449)
When we're only iterating inside, it makes sense to have a fine-grained, segregated interface and only define the narrowest functionality which supports the operations we actually need inside. It announces to the reader that we shouldn't expect any extra complexity, only the ones that the interface implements. But if you think vector does it better, I'm also fine with that. For me span is not about a subset of teh data but a subset of the functionality - just a narrowed view to rule out certain
...

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2121293497)
Thanks for checking!

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2930956026)
Rebased e604a57dccac1c4e6802a3ecc7f4cb4c3229c8ed -> 15ff9cb1789b7fe69c0ad24d737712aa6955e9cb ([spendblock_3](https://github.com/TheCharlatan/bitcoin/tree/spendblock_3) -> [spendblock_4](https://github.com/TheCharlatan/bitcoin/tree/spendblock_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_3..spendblock_4))

* Fixed silent merge conflict with #32304

💬 tnndbtc commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#discussion_r2121315218)
@maflcko Nope. I only used chatgpt to study the functions. However, every letter in the PR is typed by me. :)

💬 John-zhan commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2930992630)
Ok, now downgrade my windows cmake version from 4 to 3.31.7。And now when execute 'cmake -B build --preset vs2022-static' it say this:

`正在安装 55/93 个 openssl:x64-windows@3.3.2#1...
正在生成 openssl:x64-windows@3.3.2#1...
C:\Users\win11\AppData\Local\vcpkg\registries\git-trees\2c0920b57254210866a89aa705291b452a31df48: info: installing from git registry git+https://github.com/microsoft/vcpkg@2c0920b57254210866a89aa705291b452a31df48
-- Using cached openssl-openssl-210dc9a50dfd99caa1cf7c3d2fa42850124b1bb
...

💬 hebasto commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2931010856)
> Download failed, halting portfile.

means some network issues.

If it is not intermittent, please report the issue [upstream](https://github.com/microsoft/vcpkg).

💬 brunoerg commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2121335303)
d790b83996ae0d0bc4fe9723606c8c923cec1bcc: nit: It seems the `ConsumeServiceVector` is always called with the `max_vector_size` being 5? Maybe it could be the default value since it's only used here.

👍 brunoerg approved a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2888696559)
reACK 582d9e3dbdf5b64272b65844d679942c5aca643f

Just generated this coverage report with some hours of running: https://brunoerg.xyz/bitcoin-core-coverage/28584/coverage_report/

💬 mzumsande commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2121338410)
For the sake of disconnecting the peer? I think we could use the flags set or not set in the block to learn which mutation check (`CheckMerkleRoot()` or `CheckWitnessMalleation()`) has failed.

👍 l0rinc approved a pull request: "rpc: Use type-safe HelpException"
(https://github.com/bitcoin/bitcoin/pull/32660#pullrequestreview-2888696257)
code review ACK fa0cf42380dec17e73d38aa69dd70662a0bc344a

💬 l0rinc commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2121346010)
👍, alternatively, if you think it's clearer:
```suggestion
for (const auto& pcmd : vCommands | std::views::values) {
```

💬 l0rinc commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2121336417)
nits:
* formatting is usually different for classes/structs - but maybe this style was chosen to align with the rest of the file
* can be `final`
```suggestion
struct HelpException final : std::runtime_error
{
explicit HelpException(const std::string& msg) : std::runtime_error{msg} {}
};
```

🤔 rkrux reviewed a pull request: "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC"
(https://github.com/bitcoin/bitcoin/pull/32593#pullrequestreview-2888425586)
> Keeping track of whether a locked coin was persisted is also useful
information for future PRs.

Is the only benefit that while exporting watch only version of the wallet, the code need not read the locked coin from the disk and instead can read from the `m_locked_coins` map?

https://github.com/bitcoin/bitcoin/pull/32489/files#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8R4604-R4608

Earlier I had few concerns regarding the persistence of the locked coins incase
...

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121257035)
Should it not check the `persistent` boolean parameter of the locked coin just like it's checked in `UnlockCoin`? Otherwise, it tries to erase a locked coin that was not persisted to disk.

Scenario: I lock a coin by calling `lockunspent` RPC with `unlock: false` and `persistent: false`. I then try to unlock all coins by calling `lockunspent` RPC with `unlock: true` and letting the `persistent` param being null.
Though the RPC still returns `true` probably because of the SQLITE's design: htt
...

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121160285)
This looks better to me from readability POV. Earlier it seemed to "lock coin" again right after loading the locked coin and didn't write it again in the disk based on the DB batch not being passed in the second argument that was not intuitive while reading.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121260886)
Though it was present in previous code as well, I find the usage of bitwise `and` in boolean operations undesirable.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121285665)
This is the only place where I find the usage of `LoadLockedCoin` unusual because it comes prior to actually locking the coin in disk. But seems unavoidable because persisting in disk is conditional.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121271089)
This is much cleaner! This PR's intent seems fine to me as it does indeed improve code readability.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121204919)
I see the previous code also needed to figure out whether the locked coin was persisted on disk while unlocking, but it did so in a slightly complicated way as it depended on whether the DB batch was passed in the unlock flow. Theoretically it could be possible that the code forgets to pass the said batch in the unlock flow leading to unintended scenarios.
The new code seems more robust to me as it explicitly tracks whether the coin was persisted on disk.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121359136)
Unrelated: I find the `lockunspent` RPC request params unusual though. The second arg is called `transactions` but expects transaction outputs. The RPC is called `lockunspent` but the first arg is called `unlock` that leads me to straightaway do mental negation calculation while calling the RPC that seems undesirable to me.