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