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

💬 1440000bytes commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842107326)
Approach NACK

The options `datacarrier` and `datacarriersize` should not be removed. Instead only limits should be removed. Some people may want to use them for different reasons.

Maintaining core with a patch is not easy. Running older versions is not safe either.

💬 adamdecaf commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842123895)
Concept NACK

The silencing of dissenting comments here is worrying because they're not being addressed. There are many valid reasons already expressed in this PR that are valid enough to close this and reject the proposal, but the team has refused so far. Larger data has been tried in other chains, but isn't what Bitcoin is for. We should be focusing on removing opportunities for spammers, not make their lives easier.

💬 laanwj commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2842132320)
Some small berkeleydb (AFAIK, non-migration) related cruft after this:
```
src/wallet/load.cpp: // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
test/sanitizer_suppressions/tsan:race:BerkeleyBatch
vcpkg.json: "berkeleydb",
vcpkg.json: "berkeleydb": {
vcpkg.json: "berkeleydb"
```

💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842149495)
> Maintaining core with a patch is not easy.

Knots already does this, plus all the other mempool filtering stuff people demand. The extra maintenance for them is negligible.

> Some people may want to use them for different reasons.

Such as?

💬 Sjors commented on pull request "mining: rename gbt_force and gbt_force_name":
(https://github.com/bitcoin/bitcoin/pull/32386#issuecomment-2842186970)
I also documented some of the `!` behavior in #26201. It might be worth including that here, along with @ajtowns's explanation.

Behavior (3) is implemented here (code below is in master, so not renamed):

https://github.com/bitcoin/bitcoin/blob/14b8dfb2bd5e2ca2b7c0c9a7f7d50e1e60adf75c/src/rpc/mining.cpp#L965-L970

This seems potentially dangerous when combined with forced signalling, which IIRC has occasionally been proposed _after_ Bitcoin Core already shipped activation code. Once somet
...

💬 BrazyDevelopment commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842197226)
>
> This is so incredibly vague that it reads like an LLM output. @BrazyDevelopment nothing in your comment actually specifies a novel attack vector. Nodes already have resource management logic that mitigates the issues you outlined.

Apologies for the "vagueness", it was not an LLM output, allow me to clarify...

I’m not worried about some brand-new attack vector popping up, my concern is that scrapping OP_RETURN limits could make existing ones, like flood-and-loot or mempool spam, way
...

💬 0ceanSlim commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842208778)
@BrazyDevelopment Who is the arbiter of "low value" here?

💬 jlopp commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2842211803)
> This clogs things up, delays real transactions, and jacks up fees as users compete.

This is literally how the market for block space operates. You haven't described anything different.

> Massive OP_RETURN data could push smaller nodes to their memory or bandwidth limits, making it harder for them to kick out junk transactions and keep things running smoothly.

What, specifically, would make mempool eviction "harder?" The lowest fee rate paying transactions get automatically evicted reg
...

💬 darosior commented on pull request "policy: allow more than one OP_RETURN outputs per tx":
(https://github.com/bitcoin/bitcoin/pull/32381#issuecomment-2842235910)
@jesterhodl conceptual discussions should go on the mailing list. If you do take it there please understand the topic before sharing your strong opinion, this has nothing to do with Taproot witnesses. I just blocked you, which should prevent you from commenting further on this PR and wasting everyone's time.