👍 instagibbs approved a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2803748588)
ACK 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72
Commits could be squashed
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2803748588)
ACK 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72
Commits could be squashed
💬 instagibbs commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066599744)
micro-nit: `0.05000026` is pretty magical, would be better to be computed if possible
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066599744)
micro-nit: `0.05000026` is pretty magical, would be better to be computed if possible
💬 instagibbs commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066594778)
what's this doubling doing?
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066594778)
what's this doubling doing?
💬 instagibbs commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066638321)
nit sanity check
```Suggestion
tx.vout[0].scriptPubKey = CScript([OP_RETURN, b"\xff" * (data_len + 1)])
assert_greater_than(tx.get_vsize(), int(MAX_STANDARD_TX_WEIGHT / 4))
```
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066638321)
nit sanity check
```Suggestion
tx.vout[0].scriptPubKey = CScript([OP_RETURN, b"\xff" * (data_len + 1)])
assert_greater_than(tx.get_vsize(), int(MAX_STANDARD_TX_WEIGHT / 4))
```
💬 instagibbs commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066626406)
sanity check
```Suggestion
tx.vout[0].scriptPubKey = CScript([OP_RETURN, b"\xff" * (data_len)])
assert_equal(tx.get_vsize(), int(MAX_STANDARD_TX_WEIGHT / 4))
```
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066626406)
sanity check
```Suggestion
tx.vout[0].scriptPubKey = CScript([OP_RETURN, b"\xff" * (data_len)])
assert_equal(tx.get_vsize(), int(MAX_STANDARD_TX_WEIGHT / 4))
```
🤔 jamesob reviewed a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2803827620)
Concept ACK
Relaxing Core's standardness rules makes it less likely that some users will eschew its P2P relay network in favor of "private" mempool submission systems that are more permissive but (debatably) less censorship resistant.
While I think this outcome is probably inevitable, any change that brings Core's policy closer to the consensus rules is likely to slow this eventuality, and so is probably worth doing.
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2803827620)
Concept ACK
Relaxing Core's standardness rules makes it less likely that some users will eschew its P2P relay network in favor of "private" mempool submission systems that are more permissive but (debatably) less censorship resistant.
While I think this outcome is probably inevitable, any change that brings Core's policy closer to the consensus rules is likely to slow this eventuality, and so is probably worth doing.
💬 jamesob commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066646065)
Can't remember how this has been handled in the past, but simply removing these options will cause existing invocations/configurations that use these options to error on start. Might be better to show a deprecation message and set new defaults, but might also be unnecessary if similar provisions haven't been made in the past.
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2066646065)
Can't remember how this has been handled in the past, but simply removing these options will cause existing invocations/configurations that use these options to error on start. Might be better to show a deprecation message and set new defaults, but might also be unnecessary if similar provisions haven't been made in the past.
📝 darosior opened a pull request: "p2p: stop special-casing witness-stripped error for unconfirmed transactions"
(https://github.com/bitcoin/bitcoin/pull/32379)
Opening as draft to facilitate discussions, this has some issues.
This special-case was introduced in #18044 to avoid hindering relay of witness in adversarial situations where most connections on the network were still using txid-based relay. This is not a concern anymore. Since it is expensive to detect, get rid of the special case.
This may however cause issues for orphan resolution in adversarial situations, since fetching the parent is done using its txid. Greg Sanders pointed out to
...
(https://github.com/bitcoin/bitcoin/pull/32379)
Opening as draft to facilitate discussions, this has some issues.
This special-case was introduced in #18044 to avoid hindering relay of witness in adversarial situations where most connections on the network were still using txid-based relay. This is not a concern anymore. Since it is expensive to detect, get rid of the special case.
This may however cause issues for orphan resolution in adversarial situations, since fetching the parent is done using its txid. Greg Sanders pointed out to
...
💬 darosior commented on pull request "p2p: stop special-casing witness-stripped error for unconfirmed transactions":
(https://github.com/bitcoin/bitcoin/pull/32379#discussion_r2066694246)
It seems this comment was wrong in the first place. The parent is requested by txid, so the unstripped wtxid doesn't matter. The only reason that this was working is because WITNESS_STRIPPED was special cased as a failure and we didn't add the wtxid of the witness-stripped transaction (i.e. its txid) to the reject filters. Not special-casing this issue prevents requesting the parent since now its txid would be in the reject filter.
(https://github.com/bitcoin/bitcoin/pull/32379#discussion_r2066694246)
It seems this comment was wrong in the first place. The parent is requested by txid, so the unstripped wtxid doesn't matter. The only reason that this was working is because WITNESS_STRIPPED was special cased as a failure and we didn't add the wtxid of the witness-stripped transaction (i.e. its txid) to the reject filters. Not special-casing this issue prevents requesting the parent since now its txid would be in the reject filter.
💬 glozow commented on pull request "p2p: stop special-casing witness-stripped error for unconfirmed transactions":
(https://github.com/bitcoin/bitcoin/pull/32379#issuecomment-2839156365)
> This may however cause issues for orphan resolution in adversarial situations, since fetching the parent is done using its txid. Greg Sanders pointed out to me on IRC this could be an issue for package relay.
Just want to point out this is one of the motivations for (real) package relay, so we can get rid of txid-based fetching. https://github.com/bitcoin/bips/blob/master/bip-0331.mediawiki#handle-orphans-better
Today however, you'd be able to block (opportunistic) "package relay" by sen
...
(https://github.com/bitcoin/bitcoin/pull/32379#issuecomment-2839156365)
> This may however cause issues for orphan resolution in adversarial situations, since fetching the parent is done using its txid. Greg Sanders pointed out to me on IRC this could be an issue for package relay.
Just want to point out this is one of the motivations for (real) package relay, so we can get rid of txid-based fetching. https://github.com/bitcoin/bips/blob/master/bip-0331.mediawiki#handle-orphans-better
Today however, you'd be able to block (opportunistic) "package relay" by sen
...
💬 fjahr commented on pull request "RFC: Accept non-std transactions in Testnet4 by default again":
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2839189354)
> In that case, the same setup that worked for testnet (getting a direct connection to a miner that is willing to support your consensus-valid but non-standard transactions) also works for mainnet, and they haven't shot themselves in the foot.
I didn't mean that they setup this connection to a miner on purpose, I meant that they just happened to be connect to a miner by chance (which I think is much more likely to happend on testnet than on mainnet).
> If you want to do your testing in sit
...
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2839189354)
> In that case, the same setup that worked for testnet (getting a direct connection to a miner that is willing to support your consensus-valid but non-standard transactions) also works for mainnet, and they haven't shot themselves in the foot.
I didn't mean that they setup this connection to a miner on purpose, I meant that they just happened to be connect to a miner by chance (which I think is much more likely to happend on testnet than on mainnet).
> If you want to do your testing in sit
...
💬 mzumsande commented on issue "rfc: only relay transactions to v2 encrypted peers":
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2839194234)
> Related, but more radical: https://github.com/bitcoin/bitcoin/issues/29618
This proposal sounds more radical to me, assuming it's meant to be the default and not optional like #29618. Once >90% of all nodes would implement it, v1 nodes wouldn't percolate the network graph anymore, and the remaining v1 nodes (bitcoin core and alternative implementations) would become second-class citizens that wouldn't see most transactions anymore before they are included in a block.
Another aspect is that t
...
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2839194234)
> Related, but more radical: https://github.com/bitcoin/bitcoin/issues/29618
This proposal sounds more radical to me, assuming it's meant to be the default and not optional like #29618. Once >90% of all nodes would implement it, v1 nodes wouldn't percolate the network graph anymore, and the remaining v1 nodes (bitcoin core and alternative implementations) would become second-class citizens that wouldn't see most transactions anymore before they are included in a block.
Another aspect is that t
...
💬 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.