💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2605307972)
+ the `UCharCast` ones
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2605307972)
+ the `UCharCast` ones
💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924113469)
> Absolutely, nobody recommended that.
You literally [suggested it one day ago](https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1922316363) and when @Eunovo said [he was going to follow through with it](https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2604687152) you gave him a thumbs up. That's what started this whole argument.
> Great, can we back that by actual measurements?
As I said, I think 50/50 is fine so I won't do those measurements. Unless @Eunovo wants to
...
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924113469)
> Absolutely, nobody recommended that.
You literally [suggested it one day ago](https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1922316363) and when @Eunovo said [he was going to follow through with it](https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2604687152) you gave him a thumbs up. That's what started this whole argument.
> Great, can we back that by actual measurements?
As I said, I think 50/50 is fine so I won't do those measurements. Unless @Eunovo wants to
...
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924120812)
> You literally https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1922316363
Read it again, there was no randomness suggested, rather the opposite, a well thought out investigation.
> likely than not that we will see more taproot adoption rather than less
Exactly, so why 50/50 and not 80/20?
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924120812)
> You literally https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1922316363
Read it again, there was no randomness suggested, rather the opposite, a well thought out investigation.
> likely than not that we will see more taproot adoption rather than less
Exactly, so why 50/50 and not 80/20?
💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924125286)
> Exactly, so why 50/50 and not 80/20?
Why 80/20?
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924125286)
> Exactly, so why 50/50 and not 80/20?
Why 80/20?
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924126425)
Exactly!
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924126425)
Exactly!
💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924128794)
> Read it again, there was no randomness suggested
What investigation? You suggested to pick a random block from mainnet and @Eunovo followed up with this saying "Block 861848 for example".
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924128794)
> Read it again, there was no randomness suggested
What investigation? You suggested to pick a random block from mainnet and @Eunovo followed up with this saying "Block 861848 for example".
💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924131171)
> > Exactly, so why 50/50 and not 80/20?
>
> Why 80/20?
I hope you get the point. I can ask you why why why for anything you suggest too (why do you want to look at the past? Why do you want to look at the past year/6 months etc) and this PR will not go anywhere which would be a shame. This is a pointless debate.
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924131171)
> > Exactly, so why 50/50 and not 80/20?
>
> Why 80/20?
I hope you get the point. I can ask you why why why for anything you suggest too (why do you want to look at the past? Why do you want to look at the past year/6 months etc) and this PR will not go anywhere which would be a shame. This is a pointless debate.
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924131994)
Please read what I wrote, there was no random block suggestion.
All I'm saying is let's back up our choices by data instead of random values guided by feelings.
Did not expect that to be a controversial statement...
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924131994)
Please read what I wrote, there was no random block suggestion.
All I'm saying is let's back up our choices by data instead of random values guided by feelings.
Did not expect that to be a controversial statement...
💬 theStack commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924134722)
> Also, won't this hit the `assert(spks.size() == 0);` added by the next commit?
Was also wondering about this. IIUC, we can't directly `continue` here, but would need execute the "`// Remove the scriptPubKeys from our current set`" loop below for also for skipped output scripts, as otherwise the set isn't empty and the assertion throws. Would be good to have test coverage for the skipping scenario, but I presume that isn't possible, as we'd need to intentionally modify the behaviour of `Infe
...
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924134722)
> Also, won't this hit the `assert(spks.size() == 0);` added by the next commit?
Was also wondering about this. IIUC, we can't directly `continue` here, but would need execute the "`// Remove the scriptPubKeys from our current set`" loop below for also for skipped output scripts, as otherwise the set isn't empty and the assertion throws. Would be good to have test coverage for the skipping scenario, but I presume that isn't possible, as we'd need to intentionally modify the behaviour of `Infe
...
💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924139948)
> Please read what I wrote, there was no random block suggestion.
Of course and you confirmed it with the engagement in the comment.
> All I'm saying is let's back up our choices by data instead of random values guided by feelings.
What data? Your suggestion is as arbitrary as mine as long as you have not made an actual suggestion of which data you want to use and put in the work to run the analysis. When have done that I can degrade that effort just like you degrade my comments here by
...
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924139948)
> Please read what I wrote, there was no random block suggestion.
Of course and you confirmed it with the engagement in the comment.
> All I'm saying is let's back up our choices by data instead of random values guided by feelings.
What data? Your suggestion is as arbitrary as mine as long as you have not made an actual suggestion of which data you want to use and put in the work to run the analysis. When have done that I can degrade that effort just like you degrade my comments here by
...
💬 pinheadmz commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924154132)
Ok guys, cool it please. I recommend a 24 hour break on this thread, or take it off github.
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1924154132)
Ok guys, cool it please. I recommend a 24 hour break on this thread, or take it off github.
💬 mzumsande commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605397889)
> I'm trying to use some real Mainnet blocks here instead.
How would that be feasible if we don't have the UTXO set that this block is validated on?
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605397889)
> I'm trying to use some real Mainnet blocks here instead.
How would that be feasible if we don't have the UTXO set that this block is validated on?
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605410942)
A similar attempt I had (based on your previous comment): https://github.com/bitcoin/bitcoin/pull/31699/commits/d90a0b8c90cd662bd2588e7df5fa6f641eebe3ba
But we can of course just copy the structure to make sure the benchmarks measure something real and not something completely made up (happens all the time with benchmarks and tests).
There is no perfect benchmark, but I'd like to investigate if we can do better than what we have currently.
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605410942)
A similar attempt I had (based on your previous comment): https://github.com/bitcoin/bitcoin/pull/31699/commits/d90a0b8c90cd662bd2588e7df5fa6f641eebe3ba
But we can of course just copy the structure to make sure the benchmarks measure something real and not something completely made up (happens all the time with benchmarks and tests).
There is no perfect benchmark, but I'd like to investigate if we can do better than what we have currently.
💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605423826)
> How would that be feasible if we don't have the UTXO set that this block is validated on?
@mzumsande Copying the block structure could be feasible
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605423826)
> How would that be feasible if we don't have the UTXO set that this block is validated on?
@mzumsande Copying the block structure could be feasible
💬 brunoerg commented on pull request "descriptor: check whitespace in pubkeys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2605428948)
Forgot to fix a test, force-pushed doing it. Now CI is happy!
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2605428948)
Forgot to fix a test, force-pushed doing it. Now CI is happy!
💬 achow101 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_r1924184950)
> Is that redundant after [34c33a7](https://github.com/bitcoin/bitcoin/commit/34c33a747128e1c26b1931ac3fa0c4ed6b6a7bb7)?
No. There are other non-PSBT callers of `ProduceSignature` which may still pass `SIGHASH_DEFAULT` to legacy input types, notably `signrawtranssaction`.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1924184950)
> Is that redundant after [34c33a7](https://github.com/bitcoin/bitcoin/commit/34c33a747128e1c26b1931ac3fa0c4ed6b6a7bb7)?
No. There are other non-PSBT callers of `ProduceSignature` which may still pass `SIGHASH_DEFAULT` to legacy input types, notably `signrawtranssaction`.
💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1924194259)
Done
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1924194259)
Done
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r1924194604)
Added a check for the size.
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r1924194604)
Added a check for the size.
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2605456917)
Will add decoding test vectors.
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2605456917)
Will add decoding test vectors.
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924202539)
Should, because loading a legacy wallet will add the P2WPKH script to `mapScripts` for all keys, regardless of pre or post segwit. This is done in memory only, on loading.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924202539)
Should, because loading a legacy wallet will add the P2WPKH script to `mapScripts` for all keys, regardless of pre or post segwit. This is done in memory only, on loading.