💬 mzumsande commented on issue "Verify AssumeUTXO snapshot hashes during full validation as well":
(https://github.com/bitcoin/bitcoin/issues/32377#issuecomment-2839206613)
Depending on what we do if the hash doesn't match, wouldn't this be effectively be reintroducing something similar to checkpoints (which we just got rid of in #31649)?
(https://github.com/bitcoin/bitcoin/issues/32377#issuecomment-2839206613)
Depending on what we do if the hash doesn't match, wouldn't this be effectively be reintroducing something similar to checkpoints (which we just got rid of in #31649)?
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066761183)
It creates two outputs to demonstrate that you can have more than one.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066761183)
It creates two outputs to demonstrate that you can have more than one.
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066764753)
Maybe better for a followup to refactor this test. The "bad" examples are nice, but the "good" examples can me simplified by adding a `check_mempool_happy` helper that doesn't require all fields to match.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066764753)
Maybe better for a followup to refactor this test. The "bad" examples are nice, but the "good" examples can me simplified by adding a `check_mempool_happy` helper that doesn't require all fields to match.
🤔 rkrux reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2803611140)
I'm done with review ea751ce.
The `wallet_backwards_compatibility.py` test is failing. Guessing it's because of the silent merge conflict #32369, I don't see the exact error though in the CI.
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2803611140)
I'm done with review ea751ce.
The `wallet_backwards_compatibility.py` test is failing. Guessing it's because of the silent merge conflict #32369, I don't see the exact error though in the CI.
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2066493272)
Wondering why this was `SIGHASH_DEFAULT` before as I don't see anything Taproot specific here.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2066493272)
Wondering why this was `SIGHASH_DEFAULT` before as I don't see anything Taproot specific here.
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2066725643)
+1 for adding the `[[nodiscard]]` here!
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2066725643)
+1 for adding the `[[nodiscard]]` here!
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2066681881)
This looked slightly odd to me at first glance. Interesting that an incomplete (not signed as per the `SignPSBTInput`) PSBT input is fine here at this stage because if it's incomplete then the below `if` check would accordingly not update the number of inputs signed variable below.
Only if the error was due to `sighash_mismatch`, it needs to be returned right away just like the behaviour prior to this change.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2066681881)
This looked slightly odd to me at first glance. Interesting that an incomplete (not signed as per the `SignPSBTInput`) PSBT input is fine here at this stage because if it's incomplete then the below `if` check would accordingly not update the number of inputs signed variable below.
Only if the error was due to `sighash_mismatch`, it needs to be returned right away just like the behaviour prior to this change.
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2066767532)
It appears that the logic of the removal of unnecessary transactions has become more fine-grained as it checks the `sighash` of each input now instead of treating all the inputs with the same sighash as done previously.
Overall seems better to me than it was earlier, I've not thought of any adverse consequence of it yet.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r2066767532)
It appears that the logic of the removal of unnecessary transactions has become more fine-grained as it checks the `sighash` of each input now instead of treating all the inputs with the same sighash as done previously.
Overall seems better to me than it was earlier, I've not thought of any adverse consequence of it yet.
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066768297)
It's probably OK for node operators who explicitly set this option to be "confronted" with the change, if they didn't check the release note.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066768297)
It's probably OK for node operators who explicitly set this option to be "confronted" with the change, if they didn't check the release note.
💬 eragmus commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839257006)
Concept ACK
Arbitrary data enjoyers will continue regardless if this is not done, but in more harmful ways. Adhering Bitcoin Core to reality, then, through this change, will benefit the Bitcoin network. It is a simple change, yet effectively resolves the downsides that otherwise will continue to exist and can even get worse (already explained by others).
The pro-free-market, pro-fee-market, pro-freedom, harm-reduction reasoning behind this change should have been understood and acted upon man
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839257006)
Concept ACK
Arbitrary data enjoyers will continue regardless if this is not done, but in more harmful ways. Adhering Bitcoin Core to reality, then, through this change, will benefit the Bitcoin network. It is a simple change, yet effectively resolves the downsides that otherwise will continue to exist and can even get worse (already explained by others).
The pro-free-market, pro-fee-market, pro-freedom, harm-reduction reasoning behind this change should have been understood and acted upon man
...
💬 instagibbs commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066771612)
right it already has coverage in https://github.com/bitcoin/bitcoin/pull/32359/files/47f9565c15faec0ce35a17c68ade85af1dc41677..8fd09d622d161eb554f080a52735832119cb35e5#diff-a99d72c0ed66c256169e92327e04ab223e71f5ef598e14aac0ff44dc2a1194daR351
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066771612)
right it already has coverage in https://github.com/bitcoin/bitcoin/pull/32359/files/47f9565c15faec0ce35a17c68ade85af1dc41677..8fd09d622d161eb554f080a52735832119cb35e5#diff-a99d72c0ed66c256169e92327e04ab223e71f5ef598e14aac0ff44dc2a1194daR351
💬 l0rinc commented on issue "Verify AssumeUTXO snapshot hashes during full validation as well":
(https://github.com/bitcoin/bitcoin/issues/32377#issuecomment-2839266243)
Yes, we already have the "checkpoints", we're just ignoring them.
The difference is that we're also validating the checkpoints themselves - giving the assumeutxo scenario more credence ("every IBD verifies these hashes").
(https://github.com/bitcoin/bitcoin/issues/32377#issuecomment-2839266243)
Yes, we already have the "checkpoints", we're just ignoring them.
The difference is that we're also validating the checkpoints themselves - giving the assumeutxo scenario more credence ("every IBD verifies these hashes").
🤔 glozow reviewed a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2804052489)
Concept ACK, code looks correct to me. In the past, this limit may have discouraged (i.e. proactively) usage but that ship has sailed. It now has the additional negative effect of pushing usage away from the public network, which is a serious problem worth addressing. If someone has something that is, say, 100B and it's more expensive to use an inscription than to use an OP_RETURN + pay a miner privately, they'd pay a miner. Being able to get rid of `-datacarriersize` on tests is a nice plus.
...
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2804052489)
Concept ACK, code looks correct to me. In the past, this limit may have discouraged (i.e. proactively) usage but that ship has sailed. It now has the additional negative effect of pushing usage away from the public network, which is a serious problem worth addressing. If someone has something that is, say, 100B and it's more expensive to use an inscription than to use an OP_RETURN + pay a miner privately, they'd pay a miner. Being able to get rid of `-datacarriersize` on tests is a nice plus.
...
💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2066785147)
In commit "interfaces: refactor: move `waitNext` implementation to miner" (a5145ba0fc3339392ff71bf20613812e0d8c8efc)
This change generally looks good, but I would suggest tweaking this commit and the next one to pass Chainman& and KernelNotification& parameters directly instead of NodeContext& because NodeContext exposes a lot of node state that this code shouldn't need access to and also brings in dependencies this code shouldn't need to depend on.
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2066785147)
In commit "interfaces: refactor: move `waitNext` implementation to miner" (a5145ba0fc3339392ff71bf20613812e0d8c8efc)
This change generally looks good, but I would suggest tweaking this commit and the next one to pass Chainman& and KernelNotification& parameters directly instead of NodeContext& because NodeContext exposes a lot of node state that this code shouldn't need access to and also brings in dependencies this code shouldn't need to depend on.
💬 Sjors commented on issue "rfc: only relay transactions to v2 encrypted peers":
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2839285577)
Indeed this change would be less radical than a default-off #29618. I meant in reference to a default-on version of that.
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2839285577)
Indeed this change would be less radical than a default-off #29618. I meant in reference to a default-on version of that.
💬 bkkarki21 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839310664)
Concept NACK.
My node is not a cloud storage for arbitrary data
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839310664)
Concept NACK.
My node is not a cloud storage for arbitrary data
💬 wizkid057 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839348282)
2. Getting it into a block
> i. this is already trivial with LibreRelay and such, there hasn't been a need to contact miners directly in recent years
> ii. even going directly to a miner requires little sophistication. There's already almost no deterrence against this attack. I doubt many, or any, companies that offer this service run a full suite of virus and other content scanners to check for matches.
Everyone here overestimates the market penetration and actual usage of such thing
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2839348282)
2. Getting it into a block
> i. this is already trivial with LibreRelay and such, there hasn't been a need to contact miners directly in recent years
> ii. even going directly to a miner requires little sophistication. There's already almost no deterrence against this attack. I doubt many, or any, companies that offer this service run a full suite of virus and other content scanners to check for matches.
Everyone here overestimates the market penetration and actual usage of such thing
...
💬 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
...