💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2174327111)
this list is incomplete. Also, I wonder how this should be maintained. There needs to be an automated check that the list is up-to-date, like for the other list.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2174327111)
this list is incomplete. Also, I wonder how this should be maintained. There needs to be an automated check that the list is up-to-date, like for the other list.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174491505)
I don't think there is any point in trying to micro-optimize this.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174491505)
I don't think there is any point in trying to micro-optimize this.
💬 Muniru0 commented on pull request "docs: add guidance on initialism capitalisation in PascalCase identifiers.":
(https://github.com/bitcoin/bitcoin/pull/32720#issuecomment-3018221235)
> Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
aaah, forgot. thanks
(https://github.com/bitcoin/bitcoin/pull/32720#issuecomment-3018221235)
> Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
aaah, forgot. thanks
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174500469)
> Asking because of
This is unrelated. Most sigops counted here are not counted in the old rule. In any case this change cannot lead to miners create invalid blocks.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174500469)
> Asking because of
This is unrelated. Most sigops counted here are not counted in the old rule. In any case this change cannot lead to miners create invalid blocks.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174503497)
Don't think that matters much, but i'll take your suggestion.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174503497)
Don't think that matters much, but i'll take your suggestion.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174515133)
In real usage `AreInputsStandard` is always called with a warm up cache so what you check here is not representative.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174515133)
In real usage `AreInputsStandard` is always called with a warm up cache so what you check here is not representative.
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174523500)
> In any case this change cannot lead to miners creating invalid blocks.
I know, what I was asking is how we expect the non-standardness of blocks to change because of this PR, i.e. how many blocks are usually mined that contradict the new rules and how we expect this PR to change that in time.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174523500)
> In any case this change cannot lead to miners creating invalid blocks.
I know, what I was asking is how we expect the non-standardness of blocks to change because of this PR, i.e. how many blocks are usually mined that contradict the new rules and how we expect this PR to change that in time.
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174531192)
The current implementation is ~66% slower, nothing "micro" about that. Very often we can make code simpler (i.e. having fewer moving parts) AND faster at the same time. This repeats work currently, so it's not as simple as it can get - what I'm suggesting is to explore that since the redundancy has a measurable cost.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174531192)
The current implementation is ~66% slower, nothing "micro" about that. Very often we can make code simpler (i.e. having fewer moving parts) AND faster at the same time. This repeats work currently, so it's not as simple as it can get - what I'm suggesting is to explore that since the redundancy has a measurable cost.
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174535364)
We're doing needless work in this case - calling the second sigop count validation when we already know that we could exit early. This was the argument you gave against `return sigops <= MAX_TX_LEGACY_SIGOPS`. I would be fine with either, but this in-between needs a proper explanation in my opinion.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174535364)
We're doing needless work in this case - calling the second sigop count validation when we already know that we could exit early. This was the argument you gave against `return sigops <= MAX_TX_LEGACY_SIGOPS`. I would be fine with either, but this in-between needs a proper explanation in my opinion.
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174536803)
Can you update the benchmark to make it representative?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174536803)
Can you update the benchmark to make it representative?
💬 Sjors commented on pull request "p2p: add more bad ports":
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2174547249)
Trying to open that port sounds like a great way to be flagged as malware.
(https://github.com/bitcoin/bitcoin/pull/32826#discussion_r2174547249)
Trying to open that port sounds like a great way to be flagged as malware.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174547626)
Sure, dropped all unnecessary temporaries in `emplace_back`s from my unit test.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174547626)
Sure, dropped all unnecessary temporaries in `emplace_back`s from my unit test.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174547674)
Sure, done.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174547674)
Sure, done.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174547736)
Sure. I updated all usage of `BOOST_CHECK` that was checking for anything else than a boolean return value in my unit test to instead use `BOOST_CHECK_GT`, `BOOST_CHECK_LT` and `BOOST_CHECK_EQUAL`.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174547736)
Sure. I updated all usage of `BOOST_CHECK` that was checking for anything else than a boolean return value in my unit test to instead use `BOOST_CHECK_GT`, `BOOST_CHECK_LT` and `BOOST_CHECK_EQUAL`.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174554324)
> Otherwise it's harder to tell if it fails for the right reasons...
This is demonstrated through bounds checking. We check that `AreInputsStandard` passes, and check that it then fails by adding one more identical input.
> i.e. that we could still mine these non-standard blocks?
We would not mine blocks with such transactions anymore, this is the whole point of this PR.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174554324)
> Otherwise it's harder to tell if it fails for the right reasons...
This is demonstrated through bounds checking. We check that `AreInputsStandard` passes, and check that it then fails by adding one more identical input.
> i.e. that we could still mine these non-standard blocks?
We would not mine blocks with such transactions anymore, this is the whole point of this PR.
💬 optout21 commented on pull request "mempool: Avoid needless vtx iteration in `removeForBlock` during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2174566343)
Couldn't this also go inside the above `if`? Can this call be omitted if `txs_removed_for_block` is empty?
If yes, the declaration of `txs_removed_for_block` could also go inside the `if`.
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2174566343)
Couldn't this also go inside the above `if`? Can this call be omitted if `txs_removed_for_block` is empty?
If yes, the declaration of `txs_removed_for_block` could also go inside the `if`.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174574795)
This is unrelated. My code above is building the P2SH redeem script, i.e. the program being executed. You are referring to a specific instance of a scriptSig spending a multisig wrapped into a P2SH. The example you link pushes 2 signatures plus an empty push because of the off-by-one CHECKMULTISIG bug.
Besides, your code suggestion is strictly functionally equivalent as `OP_0` pushes an empty vector on the stack. This diff will also result in the very same program since `OP_0` is `0x00` and p
...
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174574795)
This is unrelated. My code above is building the P2SH redeem script, i.e. the program being executed. You are referring to a specific instance of a scriptSig spending a multisig wrapped into a P2SH. The example you link pushes 2 signatures plus an empty push because of the off-by-one CHECKMULTISIG bug.
Besides, your code suggestion is strictly functionally equivalent as `OP_0` pushes an empty vector on the stack. This diff will also result in the very same program since `OP_0` is `0x00` and p
...
💬 TheCharlatan commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174575315)
> non-standardness of blocks to change because of this PR
I don't get what you are asking here. I don't think there is such a thing as "non-standardness of blocks".
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174575315)
> non-standardness of blocks to change because of this PR
I don't get what you are asking here. I don't think there is such a thing as "non-standardness of blocks".
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174579276)
Added a comment, thanks.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174579276)
Added a comment, thanks.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3018377738)
@Sjors `AreInputsStandard` is always called with a warm cache. The benchmark is not. Additional `AccessCoin` calls to fetch coins from the cache do not introduce any meaningful cost.
@Sjors @l0rinc please let's avoid rushing to performance conclusions on the basis of a microbenchmark that is not representative of how functions are actually used in reality. This PR does not introduce any meaningful slowdown.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3018377738)
@Sjors `AreInputsStandard` is always called with a warm cache. The benchmark is not. Additional `AccessCoin` calls to fetch coins from the cache do not introduce any meaningful cost.
@Sjors @l0rinc please let's avoid rushing to performance conclusions on the basis of a microbenchmark that is not representative of how functions are actually used in reality. This PR does not introduce any meaningful slowdown.