💬 davidgumberg commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#discussion_r2068639507)
non blocking nit, I think we can use `e.what()` in place of `e.code().message()`

💬 laanwj commented on pull request "[PoC] Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2068645615)
Is this necessary with the manifest?

Also we already have
```
SetConsoleCP(CP_UTF8);
SetConsoleOutputCP(CP_UTF8);
```
in `void SetupEnvironment()`.

💬 davidgumberg commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#issuecomment-2841953785)
untested crACK https://github.com/bitcoin/bitcoin/pull/32383/commits/97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3

This code made sense when using boost, but I think `std::filesystem` implementations are responsible for and better equipped to handle platform specific character width issues.

💬 maflcko commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#discussion_r2068662162)
> non blocking nit, I think we can use `e.what()` in place of `e.code().message()`

This will redundantly include the filenames again, or it may not, so it seems worse?

👍 laanwj approved a pull request: "subprocess: Backport upstream changes"
(https://github.com/bitcoin/bitcoin/pull/32358#pullrequestreview-2807061907)
Code review ACK 7e80030a952a06101d5755032ebb1ff15823815e

i have not compared this to upstream but the change in themselves LGTM.

💬 maflcko commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2068672564)
> Am I missing anything important here?

Yes, the preset doesn't check for those. You'll have to run with the integer sanitizer to be able to detect integer sanitizer issues.

The testing steps are also explained in the comment: https://github.com/bitcoin/bitcoin/pull/28865#issue-1991140953

💬 hebasto commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#discussion_r2068672604)
UTF-8 paths included in `e.what()` are not displayed correctly on Windows.

💬 francisco-alonso commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842014836)
This PR must not be merged. I seriously question whether the fact that we can even entertain proposals like this means Bitcoin is truly decentralized. BTC was designed as a peer-to-peer transaction system and it has nothing to do with embedding garbage into transactions.

Core concepts must be untouchable; from that foundation, any proposal is welcome.

💬 LaurentMT commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842018949)
@Sjors
Sorry but it seems that we're talking past each other. Are you telling me that 1 byte of data stored in the witness is weighted like 1 byte of data stored in an op_return output? If it's not the case then I don't see how we can disagree that 1 byte of data stored in an op_return output is more damaging to the throughput of the system than 1 byte of data stored in the witness. The fact that the user pays more fees to the miner doesn't change that.

💬 jesterhodl commented on pull request "policy: allow more than one OP_RETURN outputs per tx":
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2842029217)
The rationale for these changes as outlined on the ML is to facilitate Citrea and nudge them in a more efficient direction of storing data in OP_RETURN, instead of taproot witness. As I understand they plan to use 1 OP_RETURN of 80 bytes and 2 taproot outputs of 32 bytes, totaling 3 data chunks of 144 bytes in sum.

Removing the limit on count of OP_RETURNs is therefore **much over what's realistically attempted by them** and removal of safety measure introduces risk of data insertions which s
...

💬 fanquake commented on pull request "subprocess: Backport upstream changes":
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2842034379)
> During my quick (and possibly not thorough) tests, I was unable to reproduce the issues mentioned in those PRs on our codebase.

Seems odd to not pull in all changes, given no real rationale (i.e the code being incorrect/broken) for excluding them.
This also means that next time this header is updated, they may or may not get pulled in anyways, depending on the author of the change, given there's nothing documented to say they should be excluded.

💬 hebasto commented on pull request "[PoC] Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2068697912)
> Is this necessary with the manifest?

The manifest is responsible for the active code page, which differs from console input and output code pages. It would be easy to compare outputs of `GetACP()`, `GetConsoleCP()` and `GetConsoleOutputCP()`.

> Also we already have
>
> ```
> SetConsoleCP(CP_UTF8);
> SetConsoleOutputCP(CP_UTF8);
> ```
>
> in `void SetupEnvironment()`.

Thanks! I overlooked those calls. However, with the manifest, setting only the console output code pag
...

💬 ajtowns commented on pull request "mining: rename gbt_force and gbt_force_name":
(https://github.com/bitcoin/bitcoin/pull/32386#issuecomment-2842041231)
Note that there this field combines three effects:

1. per bip-9, it means that the template incorporates features from that soft fork and miners not aware of what the soft fork means may produce invalid blocks by mining in the regular way
2. similar to bip-145's implication that clients must specify "segwit" in the rules field of its request in order for witness txs to be included, it becomes an error if a client doesn't explicitly indicate support for a feature that's activated when requ
...

🤔 hebasto reviewed a pull request: "[DRAFT] ipc: add windows support"
(https://github.com/bitcoin/bitcoin/pull/32387#pullrequestreview-2807134774)
Concept ACK.

💬 laanwj commented on pull request "[PoC] Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2068709078)
> The manifest is responsible for the active code page, which differs from console input and output code pages. It would be easy to compare outputs of GetACP(), GetConsoleCP() and GetConsoleOutputCP().

Thanks.

> Thanks! I overlooked those calls. However, with the manifest, setting only the console output code page is sufficient.

OK.

> The scoped implementation is necessary to restore the console code pages to their state prior to the application's execution.

We haven't been doing
...

💬 instagibbs commented on pull request "policy: allow more than one OP_RETURN outputs per tx":
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2842059631)
~0

If we're twiddling a knob to upset the least amount of people we probably should just twiddle it to the value we're considering (plus some buffer). Anything that results in 150 bytes or beyond?

@jesterhodl while I understand your main point I don't think adding more args in the node makes sense.

💬 laanwj commented on pull request "[PoC] Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2068718768)
If we really need to, what if we make `SetupEnvironment` scoped instead? It's a more general solution, it doesn't belong in "common/args", and would work also if there's future environment modifications that need reverting before exit.

✅ w0xlt closed a pull request: "Make `cs_db` mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32384)

💬 theuni commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2842078787)
Can we maybe just hold off on this for now? The investigation above prompted me to start working on a much larger refactor.. one that would finish the network refactor (CConnman creation) that I never managed to fully complete almost a decade ago.

I've been head-down working on it, hope to have a POC to demonstrate in the next week or two.

💬 laanwj commented on pull request "util: Remove `fsbridge::get_filesystem_error_message()`":
(https://github.com/bitcoin/bitcoin/pull/32383#discussion_r2068726292)
> UTF-8 paths included in e.what() are not displayed correctly on Windows.

Maybe they will after #32380? In any case, including the filename doesn't seem necessary.