💬 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.
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924211960)
I believe the only way is through `importmulti`.
Versions without miniscript cannot sign for miniscript. I feel like that's obvious and we don't need a test for that.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924211960)
I believe the only way is through `importmulti`.
Versions without miniscript cannot sign for miniscript. I feel like that's obvious and we don't need a test for that.
💬 fjahr commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605486813)
Approach ACK
IMO the `check_` prefix on the benchmark file name is a bit odd, just `connectblock.cpp` should be fine.
And there is quite a bit of duplication in the file benchmarks file, I played around with it a bit and I would suggest something like this: https://github.com/fjahr/bitcoin/commit/12eab5df067915ed83f91bf0777fcd0189f02b85 feel free to use it.
And I think you can take this out of draft status :)
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2605486813)
Approach ACK
IMO the `check_` prefix on the benchmark file name is a bit odd, just `connectblock.cpp` should be fine.
And there is quite a bit of duplication in the file benchmarks file, I played around with it a bit and I would suggest something like this: https://github.com/fjahr/bitcoin/commit/12eab5df067915ed83f91bf0777fcd0189f02b85 feel free to use it.
And I think you can take this out of draft status :)
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924212782)
Miniscript is a subset of script, so this sentence is correct.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924212782)
Miniscript is a subset of script, so this sentence is correct.
🤔 instagibbs reviewed a pull request: "multi-peer orphan resolution followups"
(https://github.com/bitcoin/bitcoin/pull/31666#pullrequestreview-2565394877)
a couple documentation nits only
(https://github.com/bitcoin/bitcoin/pull/31666#pullrequestreview-2565394877)
a couple documentation nits only
💬 instagibbs commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1924193994)
documentation should note that the list is appended to
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1924193994)
documentation should note that the list is appended to
💬 instagibbs commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1924186528)
they are added as new invs IFF they are deemed good candidates for resolution
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1924186528)
they are added as new invs IFF they are deemed good candidates for resolution
👍 darosior approved a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2565484357)
re-ACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2565484357)
re-ACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924251495)
I don't think any part of this comment is relevant to the current code.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1924251495)
I don't think any part of this comment is relevant to the current code.