💬 jesterhodl commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066835896)
I thought in @darosior's first email on this topic, he mentioned removing the size limits first, and subsequently removing the count limit. Being a bit contradictory:
- Why was relaxing proposed in 2 stages?
- Why was implementation done in 1 stage?
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066835896)
I thought in @darosior's first email on this topic, he mentioned removing the size limits first, and subsequently removing the count limit. Being a bit contradictory:
- Why was relaxing proposed in 2 stages?
- Why was implementation done in 1 stage?
💬 instagibbs commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839385366)
> P2P encryption must be default and no more clear relay of txn or block data can happen
[It's been default for a few releases now](https://github.com/bitcoin/bitcoin/pull/29347)
Mandating it for peers would split the network for any nodes that don't support v2 transport. It's not going to happen in the next 5-10 years, maybe ever.
> 30 of which used a miner-based service to exceed the 83 byte limit. Not 30,000. Not 3,000,000. 30.
This directly contradicts your security argument. If
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839385366)
> P2P encryption must be default and no more clear relay of txn or block data can happen
[It's been default for a few releases now](https://github.com/bitcoin/bitcoin/pull/29347)
Mandating it for peers would split the network for any nodes that don't support v2 transport. It's not going to happen in the next 5-10 years, maybe ever.
> 30 of which used a miner-based service to exceed the 83 byte limit. Not 30,000. Not 3,000,000. 30.
This directly contradicts your security argument. If
...
⚠️ dergoegge reopened an issue: "build: x86 ASan build broken "error: inline assembly requires more registers than available""
(https://github.com/bitcoin/bitcoin/issues/31913)
Dockerfile to reproduce: https://gist.github.com/dergoegge/fc97743028f60719759b5498f5f022bf
Building with `clang-19` and `-fsantize=address` fails with:
```
7.574 /bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
7.574 356 | "movq 32(%%rsi), %%r11\n"
7.574 | ^
7.575 /bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
7.575 /bitcoin/src/secp256k1/src/sc
...
(https://github.com/bitcoin/bitcoin/issues/31913)
Dockerfile to reproduce: https://gist.github.com/dergoegge/fc97743028f60719759b5498f5f022bf
Building with `clang-19` and `-fsantize=address` fails with:
```
7.574 /bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
7.574 356 | "movq 32(%%rsi), %%r11\n"
7.574 | ^
7.575 /bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
7.575 /bitcoin/src/secp256k1/src/sc
...
🤔 mzumsande reviewed a pull request: "net: remove unnecessary check from AlreadyConnectedToAddress()"
(https://github.com/bitcoin/bitcoin/pull/32338#pullrequestreview-2804176049)
Code Review ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
(https://github.com/bitcoin/bitcoin/pull/32338#pullrequestreview-2804176049)
Code Review ACK f1b142856a4ecd0a0d90bc3d73ef5137997b14ff
💬 dergoegge commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2839393525)
Now that we've removed `ENABLE_HARDENING` this is happening again. The suggested workarounds of injecting flags manually via `APPEND_*` does not work.
This is an issue for me with https://github.com/dergoegge/fuzzamoto and it''ll also be a problem on oss-fuzz, when they bump their afl++ version.
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2839393525)
Now that we've removed `ENABLE_HARDENING` this is happening again. The suggested workarounds of injecting flags manually via `APPEND_*` does not work.
This is an issue for me with https://github.com/dergoegge/fuzzamoto and it''ll also be a problem on oss-fuzz, when they bump their afl++ version.
💬 moth-oss commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839395732)
Concept NACK
removing this limit won't solve the UTXO bloat issue. There are efforts underway to solve that which is a more productive direction for those worried about the bloat.
I'm not opposed to changes in the future, but right now there is no hurry to do this. In fact, doing so now can be counterproductive, both from a scalability lens, as well as community trust and perception of core.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839395732)
Concept NACK
removing this limit won't solve the UTXO bloat issue. There are efforts underway to solve that which is a more productive direction for those worried about the bloat.
I'm not opposed to changes in the future, but right now there is no hurry to do this. In fact, doing so now can be counterproductive, both from a scalability lens, as well as community trust and perception of core.
💬 ctrlbreak- commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839408099)
Concept NACK.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839408099)
Concept NACK.
💬 hebasto commented on issue "build: x86 afl++ ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2839412263)
> Now that we've removed `ENABLE_HARDENING` this is happening again. The suggested workaround of injecting flags manually via `APPEND_*` does not work.
Which suggestion exactly?
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2839412263)
> Now that we've removed `ENABLE_HARDENING` this is happening again. The suggested workaround of injecting flags manually via `APPEND_*` does not work.
Which suggestion exactly?
💬 dergoegge commented on issue "build: x86 afl++ ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2839416536)
Sorry forgot to link it, from the PR description here: https://github.com/bitcoin/bitcoin/pull/32071.
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2839416536)
Sorry forgot to link it, from the PR description here: https://github.com/bitcoin/bitcoin/pull/32071.
💬 hsjoberg commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839479865)
Concept ACK for the reasons given in OP.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839479865)
Concept ACK for the reasons given in OP.
💬 PlayBounty commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839496799)
We can all at least agree that this is controversial, so the urgency to merge is confusing. Please stop and give folks more opportunity to reach a rough consensus.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839496799)
We can all at least agree that this is controversial, so the urgency to merge is confusing. Please stop and give folks more opportunity to reach a rough consensus.
💬 D33r-Gee commented on pull request "interface: Expose load utxo snapshot functionality":
(https://github.com/bitcoin-core/gui/pull/869#issuecomment-2839515522)
@Sjors,
Thank you for your response and for clarifying the path forward.
I apologize for the confusion on my part regarding the intended repository for the interface change. Following your comment in QML PR [#424](https://github.com/bitcoin-core/gui-qml/pull/424), I incorrectly assumed you meant `bitcoin-core/gui` when you referred to "Bitcoin Core" rather than `bitcoin/bitcoin`. Thank you for clearing that up.
It's encouraging to hear that there's interest in potentially exposing the A
...
(https://github.com/bitcoin-core/gui/pull/869#issuecomment-2839515522)
@Sjors,
Thank you for your response and for clarifying the path forward.
I apologize for the confusion on my part regarding the intended repository for the interface change. Following your comment in QML PR [#424](https://github.com/bitcoin-core/gui-qml/pull/424), I incorrectly assumed you meant `bitcoin-core/gui` when you referred to "Bitcoin Core" rather than `bitcoin/bitcoin`. Thank you for clearing that up.
It's encouraging to hear that there's interest in potentially exposing the A
...
💬 Jeremy-coding commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839517064)
ACK.
So many people in this thread have not read Hijacking Bitcoin & it shows. People here are actually commenting on Github expecting their opinion or "the amount of controversy" to have any impact on what happens at all.
Sorry guys, but that's how you like to imagine Bitcoin works but not how it actually does. Read some Bitcoin history & figure it out.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839517064)
ACK.
So many people in this thread have not read Hijacking Bitcoin & it shows. People here are actually commenting on Github expecting their opinion or "the amount of controversy" to have any impact on what happens at all.
Sorry guys, but that's how you like to imagine Bitcoin works but not how it actually does. Read some Bitcoin history & figure it out.
💬 glozow commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839518223)
Reminder that this is a pull request open for code review, not a per-username voting forum. Reviews are weighted by the merit of their arguments, not the number of times they are posted. Please rest assured that if your point has already been made here or on #28130, you don't need to post it again. There is no reason to leave unsubstantiated or repeat acks/nacks other than to spam the people subscribed to this repo.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839518223)
Reminder that this is a pull request open for code review, not a per-username voting forum. Reviews are weighted by the merit of their arguments, not the number of times they are posted. Please rest assured that if your point has already been made here or on #28130, you don't need to post it again. There is no reason to leave unsubstantiated or repeat acks/nacks other than to spam the people subscribed to this repo.
💬 donaldevine commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839531293)
Concept NACK. There are no valid reasons to remove the code to enforce datacarrier limits unless the objective was to provide a potential attack vector.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839531293)
Concept NACK. There are no valid reasons to remove the code to enforce datacarrier limits unless the objective was to provide a potential attack vector.
💬 glozow commented on pull request "p2p: stop special-casing witness-stripped error for unconfirmed transactions":
(https://github.com/bitcoin/bitcoin/pull/32379#discussion_r2066937238)
Yep. Should say 'parent should be requested since witness stripped-transaction rejections shouldn't be cached at all'
(https://github.com/bitcoin/bitcoin/pull/32379#discussion_r2066937238)
Yep. Should say 'parent should be requested since witness stripped-transaction rejections shouldn't be cached at all'
💬 glozow commented on pull request "p2p: stop special-casing witness-stripped error for unconfirmed transactions":
(https://github.com/bitcoin/bitcoin/pull/32379#discussion_r2066935115)
I don't think you want to delete the entire comment, just the part about witness stripping
(https://github.com/bitcoin/bitcoin/pull/32379#discussion_r2066935115)
I don't think you want to delete the entire comment, just the part about witness stripping
💬 wizkid057 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839558518)
> > P2P encryption must be default and no more clear relay of txn or block data can happen
>
> [It's been default for a few releases now](https://github.com/bitcoin/bitcoin/pull/29347)
>
> Mandating it for peers would split the network for any nodes that don't support v2 transport. It's not going to happen in the next 5-10 years, maybe ever.
Sad reality is that we'll directly cause people grief from this.
> > 30 of which used a miner-based service to exceed the 83 byte limit. Not 30,
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839558518)
> > P2P encryption must be default and no more clear relay of txn or block data can happen
>
> [It's been default for a few releases now](https://github.com/bitcoin/bitcoin/pull/29347)
>
> Mandating it for peers would split the network for any nodes that don't support v2 transport. It's not going to happen in the next 5-10 years, maybe ever.
Sad reality is that we'll directly cause people grief from this.
> > 30 of which used a miner-based service to exceed the 83 byte limit. Not 30,
...
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2066957921)
Thanks for taking a look
I've taken your suggestion with the addition of `CTxMemPool*` as a parameter to `WaitNext`.
I think it's best to pass it as well, to make the method self contained and allow for testing with a custom mempool.
Also, if I use the mempool pointer in the active chainstate not the node, a miner test fails.
This happens because in `TestPackageSelection`, we construct a new mempool and update the node interfaces' mempool pointer, but we don't update the raw pointer stored
...
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2066957921)
Thanks for taking a look
I've taken your suggestion with the addition of `CTxMemPool*` as a parameter to `WaitNext`.
I think it's best to pass it as well, to make the method self contained and allow for testing with a custom mempool.
Also, if I use the mempool pointer in the active chainstate not the node, a miner test fails.
This happens because in `TestPackageSelection`, we construct a new mempool and update the node interfaces' mempool pointer, but we don't update the raw pointer stored
...
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2839569970)
- Updated to fix recent comment by @ryanofsky see [5fa54e2..c8a3fab](https://github.com/bitcoin/bitcoin/compare/77949ef5267e4de4e68a19560bab81e8d5fa54e2..c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6)
- Also fixed tidy issues
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2839569970)
- Updated to fix recent comment by @ryanofsky see [5fa54e2..c8a3fab](https://github.com/bitcoin/bitcoin/compare/77949ef5267e4de4e68a19560bab81e8d5fa54e2..c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6)
- Also fixed tidy issues