💬 maflcko commented on pull request "test: Check that reindex with prune wipes blk files":
(https://github.com/bitcoin/bitcoin/pull/31696#discussion_r1924889643)
thx, used an exact match here on the filenames

💬 maflcko commented on pull request "[RFC] Align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2606602410)
Could turn into draft while the CI is failing?

💬 maflcko commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2606605540)
Could turn into draft while the CI is failing?

💬 maflcko commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2606608189)
Could turn into draft while the CI is failing?

💬 maflcko commented on pull request "doc: add a section in the fuzzing documentation about using MSan":
(https://github.com/bitcoin/bitcoin/pull/31704#issuecomment-2606618347)
Could also add a note that in most cases valgrind is the easier option, as it doesn't require a full build with a different config?

💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows, OpenBSD)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2606681443)
Trivial rebase after #31701.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1924972609)
I'm not sure what you mean here.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2606689957)
> Is there a reason we are introducing this redundancy of being able retrieve the same data in two ways (gettarget and getmininginfo)?

The redundancy was already there with `getdifficulty`. That said, I don't personally need `gettarget` so I could drop it if people prefer.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1924973251)
Will fix typo if I need to retouch.

💬 Sjors commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r1924976381)
Pfew. Separate from this PR I think it would be good if we had DSL that defines the PSBT spec and the constraints for every possible field. Maybe @ryanofsky has ideas?

💬 Sjors 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_r1924990788)
It might make things easier to follow if those callers pass the right flag as well. If it can be done without making this PR too huge.

💬 Sjors 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_r1924993691)
In other words: for all functions that take a sighash argument, you either pass the correct sighash or nothing.

💬 l0rinc commented on pull request "optimization: `CheckBlock` input duplicate detection":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1925006647)
Fixed, thanks

💬 l0rinc commented on pull request "optimization: `CheckBlock` input duplicate detection":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2606765689)
I pushed a new version that is based on https://github.com/bitcoin/bitcoin/pull/31699 - to separate the testing/benching from the consensus change. This will remain in draft to gather comments.

----

I've also retriggered an IBD (simulated via multiple ` -reindex-chainstate` runs for stability).
Here I've compared the performance of this PR after applying my other IBD optimizations (https://github.com/bitcoin/bitcoin/pull/31144 and https://github.com/bitcoin/bitcoin/pull/31645) to measure
...

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1925023954)
Ok, I suppose we could drop the code that does that eventually, but not here.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1925034229)
> Miniscript is a language for writing (a subset of) Bitcoin Scripts

https://bitcoin.sipa.be/miniscript/

So when I read "containing miniscript" I interpret that as containing a descriptor using the miniscript language. But these are legacy wallets, which don't have descriptors, so they can only contain script itself.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1925037538)
That's not going to obvious for most people looking at this in the future, so I think it's worth a comment (a test might be overkill).

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1925038708)
But then the script just disappears, shouldn't we warn about that or even abort?

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2606802006)
> it looks like this only happens with powershell, not with the classic windows shell

In that case I wouldn't worry about it.

I'll retest.

💬 willcl-ark commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2606831902)
This does not seem to fully work for me on a [Unifi Cloud Gateway Ultra](https://techspecs.ui.com/unifi/unifi-cloud-gateways/ucg-ultra?subcategory=all-unifi-cloud-gateways). It does appear to get an ipv4 mapping, but then fails whilst trying to get an ipv6 version:

```log
2025-01-22T09:42:37Z [net] portmap: gateway [IPv4]: 10.0.0.1
2025-01-22T09:42:37Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 8444 from gateway 10.0.0.1
2025-01-22T09:42:37Z [net] pcp: Internal address after conne
...

💬 fanquake commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2606846188)
> The redundancy was already there with getdifficulty. That said, I don't personally need gettarget so I could drop it if people prefer.

Some redundancy already existing isn't a reason to make things worse? We don't usually add new RPCs for fun.