š¬ sipa commented on issue "[BTC signet v22.0] websocket not working as expect":
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024239736)
Bitcoin Core has no websocket support whatsoever.
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024239736)
Bitcoin Core has no websocket support whatsoever.
š l0rinc's pull request is ready for review: "mempool: Avoid needless vtx iteration during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827)
(https://github.com/bitcoin/bitcoin/pull/32827)
š¬ l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2177745211)
That change seems riskier than the rest, we can do it in a follow-up.
The PR is ready for review.
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2177745211)
That change seems riskier than the rest, we can do it in a follow-up.
The PR is ready for review.
š¬ maflcko commented on issue "[BTC signet v22.0] websocket not working as expect":
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024315066)
Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024315066)
Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?
š¬ l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177796421)
It seems to me the first historical tx that violates this new rule is https://mempool.space/tx/659135664894e50040830edb516a76f704fd2be409ecd8d1ea9916c002ab28a2 with 2585 sigops.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177796421)
It seems to me the first historical tx that violates this new rule is https://mempool.space/tx/659135664894e50040830edb516a76f704fd2be409ecd8d1ea9916c002ab28a2 with 2585 sigops.
š¬ l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177807221)
> Apologies if that came across as patronizing
> if you insistently derail a PR with irrelevant concerns
no, they're not irrelevant or silly, I expect better arguments than these
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177807221)
> Apologies if that came across as patronizing
> if you insistently derail a PR with irrelevant concerns
no, they're not irrelevant or silly, I expect better arguments than these
š¬ pinheadmz commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177826383)
Can we please ask a different reviewer for an opinion on this one thing to break the tie before anyone's feelings get hurt?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177826383)
Can we please ask a different reviewer for an opinion on this one thing to break the tie before anyone's feelings get hurt?
š¬ pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2177843490)
> This should be && instead of ||.
Yeah, my bad.
> .isNull() check is not really reliable either, because it will happily accept an empty array.
At the moment you could do:
```
./build/bin/bitcoin-cli -signet -datadir=/tmp/btc getdescriptoractivity '[]' '[]'
{
"activity": [
]
}
```
And that doesn't make sense either.
> I think just making sure we only execute the request.params[n].get_array().getValues() statements if we've first checked that !request.params[n].isNull() sho
...
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2177843490)
> This should be && instead of ||.
Yeah, my bad.
> .isNull() check is not really reliable either, because it will happily accept an empty array.
At the moment you could do:
```
./build/bin/bitcoin-cli -signet -datadir=/tmp/btc getdescriptoractivity '[]' '[]'
{
"activity": [
]
}
```
And that doesn't make sense either.
> I think just making sure we only execute the request.params[n].get_array().getValues() statements if we've first checked that !request.params[n].isNull() sho
...
š¬ sipa commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177844858)
@l0rinc We should certainly not gratuitously make changes the worsen performance, but I think there have been reasonable arguments given here:
* It results in simpler code changes, specifically ones that impact consensus logic less.
* The slowdown in negligible compared the overall cost of transaction validation. It's good to be aware that there is a slowdown, but also reasonable to dismiss it on the basis that it's entirely justifiable in the presence of a much more important concern (reducin
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177844858)
@l0rinc We should certainly not gratuitously make changes the worsen performance, but I think there have been reasonable arguments given here:
* It results in simpler code changes, specifically ones that impact consensus logic less.
* The slowdown in negligible compared the overall cost of transaction validation. It's good to be aware that there is a slowdown, but also reasonable to dismiss it on the basis that it's entirely justifiable in the presence of a much more important concern (reducin
...
š¬ darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177851038)
> Do we have data on how many main-chain transactions historically exceed the new threshold of 2500 legacy sigops?
Iād like to confirm that the new policy would block no real-world traffic and is purely a preventive DoS measure ahead of any future BIP-54 soft-fork activation.
I don't think past usage is necessarily the best predictor, i think it's more convincing to reason through what real-world traffic could look like. The BIP [goes through this reasoning](https://github.com/bitcoin/bips/b
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177851038)
> Do we have data on how many main-chain transactions historically exceed the new threshold of 2500 legacy sigops?
Iād like to confirm that the new policy would block no real-world traffic and is purely a preventive DoS measure ahead of any future BIP-54 soft-fork activation.
I don't think past usage is necessarily the best predictor, i think it's more convincing to reason through what real-world traffic could look like. The BIP [goes through this reasoning](https://github.com/bitcoin/bips/b
...
š¬ saravadeanil commented on issue "[BTC signet v22.0] websocket not working as expect":
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024436337)
I am trying to perform rawtx using websocket.
It think it works on bitcoin testnet & mainnet network with version v22.0.0.
I am wondering why it's not working with signet.
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024436337)
I am trying to perform rawtx using websocket.
It think it works on bitcoin testnet & mainnet network with version v22.0.0.
I am wondering why it's not working with signet.
š¬ sipa commented on issue "[BTC signet v22.0] websocket not working as expect":
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024444002)
Can you elaborate on what you're using? Because as I mentioned, Bitcoin Core has no websocket support. You may be using JSON-RPC, or ZMQ, or a P2P connection...
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024444002)
Can you elaborate on what you're using? Because as I mentioned, Bitcoin Core has no websocket support. You may be using JSON-RPC, or ZMQ, or a P2P connection...
š¬ saravadeanil commented on issue "[BTC signet v22.0] websocket not working as expect":
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024449821)
@sipa Sorry for the confusion, I am using zmqpubrawtx port.
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024449821)
@sipa Sorry for the confusion, I am using zmqpubrawtx port.
š brunoerg opened a pull request: "test: check P2SH sigop count for coinbase tx"
(https://github.com/bitcoin/bitcoin/pull/32850)
We currently do not test that `GetP2SHSigOpCount` returns 0 for coinbase transactions (see line L129 at https://corecheck.dev/mutation/src/consensus/tx_verify.cpp). This PR addresses it.
(https://github.com/bitcoin/bitcoin/pull/32850)
We currently do not test that `GetP2SHSigOpCount` returns 0 for coinbase transactions (see line L129 at https://corecheck.dev/mutation/src/consensus/tx_verify.cpp). This PR addresses it.
š¬ l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177866217)
Thanks for the response, @sipa.
Is it unreasonable to ask for a benchmark showing that the slowdown is indeed negligible in the worst case (given that it's meant to avoid an attack)?
Benchmarks were added for this exact scenario, if they're not representative (I can agree with that) - let's make them representative first. My hunch is also that the slowdown is minor, but I would prefer more than a "just trust me broā¢" here...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177866217)
Thanks for the response, @sipa.
Is it unreasonable to ask for a benchmark showing that the slowdown is indeed negligible in the worst case (given that it's meant to avoid an attack)?
Benchmarks were added for this exact scenario, if they're not representative (I can agree with that) - let's make them representative first. My hunch is also that the slowdown is minor, but I would prefer more than a "just trust me broā¢" here...
š¬ sipa commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177894356)
> Is it unreasonable to ask for a benchmark showing that the slowdown is indeed negligible in the worst case (given that it's meant to avoid an attack)?
Transactions can easily take milliseconds. A single signature check can take 10s of microseconds, and a single transaction input can do hundreds of those. The worst case is likely way worse than that still, which can easily be concluded without even thinking hard about it.
It's not unreasonable to ask for benchmarks when things aren't obvi
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177894356)
> Is it unreasonable to ask for a benchmark showing that the slowdown is indeed negligible in the worst case (given that it's meant to avoid an attack)?
Transactions can easily take milliseconds. A single signature check can take 10s of microseconds, and a single transaction input can do hundreds of those. The worst case is likely way worse than that still, which can easily be concluded without even thinking hard about it.
It's not unreasonable to ask for benchmarks when things aren't obvi
...
š¬ l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177901518)
Yes, I'm already running such a script, that's how I knew about the first coinjoin posted earlier.
Thanks for posting the result, I have one last question in this regard: I have of course read the bip-0054 reasoning, but given that this is meant to avoid an attack, I still wanted to understand how long it takes on a typical device to validate the remaining 2'500 signatures in the worst case? I'm asking because based on https://github.com/bitcoin/bitcoin/issues/32832 I want to know if I would
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177901518)
Yes, I'm already running such a script, that's how I knew about the first coinjoin posted earlier.
Thanks for posting the result, I have one last question in this regard: I have of course read the bip-0054 reasoning, but given that this is meant to avoid an attack, I still wanted to understand how long it takes on a typical device to validate the remaining 2'500 signatures in the worst case? I'm asking because based on https://github.com/bitcoin/bitcoin/issues/32832 I want to know if I would
...
š¬ l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177904988)
Thanks for the explanation, please resolve this comment.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177904988)
Thanks for the explanation, please resolve this comment.
š l0rinc approved a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2975960421)
Have a few remaining concerns, but removing my nack. Thanks for the explanations.
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2975960421)
Have a few remaining concerns, but removing my nack. Thanks for the explanations.
Concept ACK
š¬ darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177915783)
As i am retouching i re-read this commit message and i didn't find any such typo. Could you be more precised? (Although i don't plan on addressing if i don't retouch again.)
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177915783)
As i am retouching i re-read this commit message and i didn't find any such typo. Could you be more precised? (Although i don't plan on addressing if i don't retouch again.)