💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2120994262)
Yes, seemed like the most fair way to do it.

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2120989801)
It's a part of what boost multiindex requires for its "key extractor" concept: https://www.boost.org/doc/libs/1_88_0/libs/multi_index/doc/reference/key_extraction.html

💬 instagibbs commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2121231438)
Would we be able to distinguish shorttxid collision from an intentionally mutated block without the check?

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121277977)
See `AddCoins`'s declaration in `src/coins.h`:
```cpp
void AddCoins(CCoinsViewCache& cache, const CTransaction& tx, int nHeight, bool check = false);
```

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2121282517)
I don't think there could/should be, you could name them as `const auto [x, y, z]{hss.ProcessNextHeaders(std::span{second_chain.begin() + 1, second_chain.end()}, false)};` as well

💬 hebasto commented on pull request "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`":
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930944523)
My Guix build:
```
aarch64
6ec55ec67ac33aeb89eae88bd55525e36e05d3c768eb98cf9c718cc8221de76e guix-build-18cf727429e9/output/aarch64-linux-gnu/SHA256SUMS.part
f3e260544a199b5b06f3f5de4352fcff61f6a3e36fd12a827317c2ba168ab3a8 guix-build-18cf727429e9/output/aarch64-linux-gnu/bitcoin-18cf727429e9-aarch64-linux-gnu-debug.tar.gz
259b8bd0260adc1517c11a8579983210501f6625083b780f3451c5bf6d531c52 guix-build-18cf727429e9/output/aarch64-linux-gnu/bitcoin-18cf727429e9-aarch64-linux-gnu.tar.gz
6772df34
...

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