💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2092092067)
That constant came from your [comment](#pullrequestreview-2697495377) (which I've adopted into this PR), so I really don't know how to document this value. Where did you get it from? This stuff tends to have magic numbers; I don't think this PR makes it any worse.

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2092094020)
Yes, and there is a fairly detailed comment on lines 194-195 (just before the `unordered_set` version of `DynamicUsage`), so I think that's sufficient. But if you feel strongly, I'll try to come up with something.

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2092095271)
I think the comment immediately above explains it sufficiently, and the "1" was not a named constant either (so this pull is not making it worse). But if you feel strongly, I can do something. (As someone once said, the hardest part of programming is coming up with good names!)

💬 davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2092102849)
Agreed that checking just one is sufficient to verify that the function works for every exe the cmake function is invoked on, but it seems reasonable to me to glob check each executable in the `bin` folder, to enforce that e.g. new executables get the manifest embedded and existing ones don't silently lose them:

```powershell
Get-ChildItem -Filter "bin\*.exe" | ForEach-Object {
echo "Checking $($_.Name)"
mt.exe -nologo -inputresource:$_.FullName -validate_manifest
}
```

💬 davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2092105581)
I think a more general approach as described here: https://github.com/microsoft/vswhere/wiki/Start-Developer-Command-Prompt#using-powershell would be better, but I think this question is better suited for #32513.

📝 w0xlt opened a pull request: "util: C++20 `ToIntegral()` Improvement"
(https://github.com/bitcoin/bitcoin/pull/32522)
While reviewing https://github.com/bitcoin/bitcoin/pull/32520, I noticed that the function `ToIntegral()` could be improved:

* Instead of manually asserting `std::is_integral_v<T>`, the template can be constrained with the standard `std::integral` for clearer intent.

* Annotation with `[[nodiscard]]` and `noexcept`. Compilers will warn if the return value is discarded and since `std::from_chars` is guaranteed to throw nothing, declaring the wrapper `noexcept` makes the exception guarantee
...

🤔 w0xlt reviewed a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520#pullrequestreview-2845212748)
ACK https://github.com/bitcoin/bitcoin/pull/32520/commits/fa24012c181850d4adc5cca0fdba76d584ec3a00

💬 davidgumberg commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2885338351)
Reviewed and tested ACK https://github.com/bitcoin/bitcoin/commit/8f4fed7ec70093e2535423d63e9f9dd400c378ac

<details><summary>
Checked that each executable has a manifest.
</summary>

```powershell
PS Z:\bitcoin-8f4fed7ec700> Get-ChildItem -Filter "bin\*.exe" | ForEach-Object {
>> echo "Checking $($_.Name)"
>> mt.exe -nologo -inputresource:$_.FullName -validate_manifest
>> }
Checking bitcoin-cli.exe
Parsing of manifest successful.
Checking bitcoin-qt.exe
Parsing of manifest suc
...

💬 gituser commented on issue "listunspent, fundrawtransaction, getwalletinfo locks wallet for any other operation":
(https://github.com/bitcoin/bitcoin/issues/27002#issuecomment-2885340621)
> its prerequisite [#27286](https://github.com/bitcoin/bitcoin/pull/27286)

Hey I'm using a build from this PR (from your branch) on production but it's not recent one, using branch 48545bdb3b9f59a8b78640dbf20afff6e2663ad7 october 2024 version.

It works much better, but there is still `fundrawtransaction` lock issue I think.

💬 achow101 commented on issue "listunspent, fundrawtransaction, getwalletinfo locks wallet for any other operation":
(https://github.com/bitcoin/bitcoin/issues/27002#issuecomment-2885350354)
> It works much better, but there is still `fundrawtransaction` lock issue I think.

I don't think the locking is solvable without another large rewrite of the wallet. Currently, the wallet requires taking a mutex for exclusive access over many of its internal members, which necessarily means that generally only one wallet thing can happen at a time. So far, we've been focusing on the low hanging fruit of reducing the runtime of operations that are known to take a long time.

💬 gituser commented on issue "listunspent, fundrawtransaction, getwalletinfo locks wallet for any other operation":
(https://github.com/bitcoin/bitcoin/issues/27002#issuecomment-2885362613)
> I don't think the locking is solvable without another large rewrite of the wallet. Currently, the wallet requires taking a mutex for exclusive access over many of its internal members, which necessarily means that generally only one wallet thing can happen at a time. So far, we've been focusing on the low hanging fruit of reducing the runtime of operations that are known to take a long time.

Yeah, all your contributions are highly appreciated. Many thanks, hopefully these fixes will be merged
...

💬 davidgumberg commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2885370086)
> My bikeshed color: Since block hashes start with zeroes, maybe one could shard based off the last two bytes:

Just curious because the FAT32 file limit per-directory is so small, is there any scenario where a miner could DoS nodes with this format by also mining the last two bytes?

I think no, because at tip the additional hash needed for two bytes would be prohibitively expensive, although I'm not sure if there is a birthday-problem-like advantage because an attacker doesn't necessarily
...

📝 achow101 opened a pull request: "wallet: Remove watchonly behavior and isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523)
Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet. With legacy wallets now removed, all of the watchonly handling and reporting code is no longer needed. This PR removes watchonly options and results from the RPCs and the handling of watchonly things from the wallet's internals.

Additionally, ISMINE_USED is removed as its use is only as a filter for cached balances. This PR changes the caching to utilize bools
...

🤔 shahsb reviewed a pull request: "util: C++20 `ToIntegral()` Improvement"
(https://github.com/bitcoin/bitcoin/pull/32522#pullrequestreview-2845493151)
Concept ACK

Approach ACK

🤔 shahsb reviewed a pull request: "util: C++20 `ToIntegral()` Improvement"
(https://github.com/bitcoin/bitcoin/pull/32522#pullrequestreview-2845494443)
LGTM. Indeed, good improvement.

Thanks for making the changes.

🤔 shahsb reviewed a pull request: "util: C++20 `ToIntegral()` Improvement"
(https://github.com/bitcoin/bitcoin/pull/32522#pullrequestreview-2845494857)
ACK https://github.com/bitcoin/bitcoin/pull/32522/commits/6135cc3f07ae91cab91927a2b610aaed2fb38858

💬 davidgumberg commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2885708663)
It might be better to use a more general solution for setting up VS prompts, this would also work once #32396 is merged.

```yml
- name: Set up VS Developer Prompt
shell: pwsh
run: |
$vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
$installationPath = & 'C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe' -latest -property installationPath
if ($installationPath -and (test-path "$i
...

💬 maflcko commented on pull request "util: C++20 `ToIntegral()` Improvement":
(https://github.com/bitcoin/bitcoin/pull/32522#discussion_r2092382847)
not sure about adding docs that are already self-explanatory from the code without doubt. Adding such docs just means that two lines will have to be modified when modifying a trivial single line of code. Also, if the docs aren't needed, they are only a source of inconsistencies and typos, etc.

💬 maflcko commented on pull request "util: C++20 `ToIntegral()` Improvement":
(https://github.com/bitcoin/bitcoin/pull/32522#discussion_r2092378885)
not sure. We actually want C++26 erroneous behavior here, not ` [[indeterminate]] `, nor (zero-)initialized. Otherwise, it will become harder to diagnose bugs in the code with the compiler or with sanitizers.

💬 maflcko commented on pull request "util: C++20 `ToIntegral()` Improvement":
(https://github.com/bitcoin/bitcoin/pull/32522#discussion_r2092393107)
(realistically speaking in this very instance it probably doesn't make a difference and anything is fine, I just wanted to mention it for the general context.)