💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467910132)
Deleted
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467910132)
Deleted
💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1467912714)
Do `CKey::Check()` on `keydata[0]`?
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1467912714)
Do `CKey::Check()` on `keydata[0]`?
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467934024)
If we're focusing on small packages for now, can we just disable it, rename the function something "obviously" single txn related, and then in future updates we can reintroduce the check as an optimization if we like?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467934024)
If we're focusing on small packages for now, can we just disable it, rename the function something "obviously" single txn related, and then in future updates we can reintroduce the check as an optimization if we like?
💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1912459938)
> I need to read and write two private keys to/from disk
Not familiar with the stratumv2 spec, but this seems odd to me and also not a good fit for `CKey`. FWIW, there is `CPrivKey` for producing a serialized OpenSSL private key
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1912459938)
> I need to read and write two private keys to/from disk
Not familiar with the stratumv2 spec, but this seems odd to me and also not a good fit for `CKey`. FWIW, there is `CPrivKey` for producing a serialized OpenSSL private key
💬 josibake commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1912466568)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1912466568)
Concept ACK
💬 luke-jr commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1912524472)
Hmm, it's still weird, but I guess it's not terrible then.
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1912524472)
Hmm, it's still weird, but I guess it's not terrible then.
💬 achow101 commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1912534383)
> Why was my last comment to Peter Todd's comment, DELETED? Where can I find the reasoning for such deletion?
Your comment was deleted because it was off topic, consisted of insults, and contributed nothing to the discussion. Continue to make such comments and you will be blocked from this repo.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1912534383)
> Why was my last comment to Peter Todd's comment, DELETED? Where can I find the reasoning for such deletion?
Your comment was deleted because it was off topic, consisted of insults, and contributed nothing to the discussion. Continue to make such comments and you will be blocked from this repo.
💬 furszy commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1468041930)
> Have to admit that I'm lacking knowledge here about the concrete differences between FillableSigningProvider and FlatSigningProvider, especially about the possible benefits of the latter in this PR.
`FillableSigningProvider` is just the legacy class and contains legacy scripts limitations (thus #28307). It does not affect the current form of this PR but, because this benchmark uses segwit v0 and v1 outputs, it could cause issues in the future.
> Is there anything more that could be chang
...
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1468041930)
> Have to admit that I'm lacking knowledge here about the concrete differences between FillableSigningProvider and FlatSigningProvider, especially about the possible benefits of the latter in this PR.
`FillableSigningProvider` is just the legacy class and contains legacy scripts limitations (thus #28307). It does not affect the current form of this PR but, because this benchmark uses segwit v0 and v1 outputs, it could cause issues in the future.
> Is there anything more that could be chang
...
💬 theuni commented on pull request "depends: patch libool out of libnatpmp/miniupnpc":
(https://github.com/bitcoin/bitcoin/pull/29298#discussion_r1468047664)
Yep, thanks for catching this. Updated and squashed.
(https://github.com/bitcoin/bitcoin/pull/29298#discussion_r1468047664)
Yep, thanks for catching this. Updated and squashed.
💬 theuni commented on pull request "Avoid non-self-contained Windows header":
(https://github.com/bitcoin-core/gui/pull/789#issuecomment-1912605551)
ACK 8023640a71a10ec54a6a8e6b95a29d07f7be218d. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless.
(https://github.com/bitcoin-core/gui/pull/789#issuecomment-1912605551)
ACK 8023640a71a10ec54a6a8e6b95a29d07f7be218d. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless.
💬 theuni commented on pull request "depends: patch libool out of libnatpmp/miniupnpc":
(https://github.com/bitcoin/bitcoin/pull/29298#issuecomment-1912611289)
Removed one more missed instance of `LIBTOOL`.
(https://github.com/bitcoin/bitcoin/pull/29298#issuecomment-1912611289)
Removed one more missed instance of `LIBTOOL`.
💬 vostrnad commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1912628778)
@shaavan The section "Problems of P2PK" seems completely wrong to me:
1. P2TR also directly exposes a public key in its output, and all other output types expose it at spend time.
2. P2PK outputs actually have a smaller footprint than P2PKH over their lifecycle because there's no pubkey hash overhead.
3. Exposing public keys has nothing to do with linkage of transactions, and again, all output types do it.
There are problems with P2PK, just not these ones. Did you perhaps use ChatGPT to
...
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1912628778)
@shaavan The section "Problems of P2PK" seems completely wrong to me:
1. P2TR also directly exposes a public key in its output, and all other output types expose it at spend time.
2. P2PK outputs actually have a smaller footprint than P2PKH over their lifecycle because there's no pubkey hash overhead.
3. Exposing public keys has nothing to do with linkage of transactions, and again, all output types do it.
There are problems with P2PK, just not these ones. Did you perhaps use ChatGPT to
...
💬 TheBlueMatt commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1912632386)
> I think that pattern-matching / imbuing v3 is doable (that sounds like a concept ack for v3?). Assuming sibling eviction in v3, so far, it seems like it applies nicely to existing commitment transactions without any modifications. On the LN side, the requirement is no other unconfirmed inputs in the child, implying no batching.
Yes, though as I mention above I do think we may need to figure out a few additional specific details around v3 (eg how do we handle 0conf ln channels? I need to mak
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1912632386)
> I think that pattern-matching / imbuing v3 is doable (that sounds like a concept ack for v3?). Assuming sibling eviction in v3, so far, it seems like it applies nicely to existing commitment transactions without any modifications. On the LN side, the requirement is no other unconfirmed inputs in the child, implying no batching.
Yes, though as I mention above I do think we may need to figure out a few additional specific details around v3 (eg how do we handle 0conf ln channels? I need to mak
...
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1468118212)
Done, removed the comment.
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1468118212)
Done, removed the comment.
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1468118277)
Done
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1468118277)
Done
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1468118426)
Removed it.
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1468118426)
Removed it.
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1468119307)
I think it makes sense to leave them in to make sure that everything still matches previous versions.
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1468119307)
I think it makes sense to leave them in to make sure that everything still matches previous versions.
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1912659433)
I've added a commit that renames `nVersion` to just `version`. Note that it is not a scripted diff since a bunch of different variables share the same name.
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1912659433)
I've added a commit that renames `nVersion` to just `version`. Note that it is not a scripted diff since a bunch of different variables share the same name.
🚀 hebasto merged a pull request: "Avoid non-self-contained Windows header"
(https://github.com/bitcoin-core/gui/pull/789)
(https://github.com/bitcoin-core/gui/pull/789)
💬 sr-gi commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1912675545)
> This PR now only removes adjusted time from validation code while keeping all the code for the warning in place as is. A follow-up can change the warning (as it has many problems) but this PR will focus on the removal of adjusted time from consensus.
>
> Re-implementing the same warning seemed unnecessary (even if it was less code) and prone to bugs.
Does this mean that the adjusted time is now being collected and used exclusively to raise the warning?
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1912675545)
> This PR now only removes adjusted time from validation code while keeping all the code for the warning in place as is. A follow-up can change the warning (as it has many problems) but this PR will focus on the removal of adjusted time from consensus.
>
> Re-implementing the same warning seemed unnecessary (even if it was less code) and prone to bugs.
Does this mean that the adjusted time is now being collected and used exclusively to raise the warning?