💬 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?
💬 stickies-v commented on pull request "doc: update `BroadcastTransaction` comment":
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1468138472)
I don't understand why we'd delete it entirely either. It belongs together with the next sentence. I suppose it can be deduced from it too, but the current approach doesn't seem like an improvement to me. I still think my suggested phrasing works best from what I've seen so far. I don't think it's ambiguous, it just doesn't specify which RPC is called, which is irrelevant to the rest of the documentation and as such we shouldn't make it look like it is.
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1468138472)
I don't understand why we'd delete it entirely either. It belongs together with the next sentence. I suppose it can be deduced from it too, but the current approach doesn't seem like an improvement to me. I still think my suggested phrasing works best from what I've seen so far. I don't think it's ambiguous, it just doesn't specify which RPC is called, which is irrelevant to the rest of the documentation and as such we shouldn't make it look like it is.
📝 brunoerg opened a pull request: "addrman: delete addresses that don't belong to the supported networks"
(https://github.com/bitcoin/bitcoin/pull/29330)
This PR adds a new flag `-cleanupaddrman`. When initializing a node with this flag, it will delete addresses from addrman (both new and tried tables) that do not belong to the supported networks (e.g. `-onlynet`). Deleting addresses that do not belong to the supported networks can avoid a lot of unnecessary addrman `Select` calls (especially when turning from clearnet + Tor/I2P/CJDNS to Tor/I2P/CJDNS). This flag is not enabled by default.
(https://github.com/bitcoin/bitcoin/pull/29330)
This PR adds a new flag `-cleanupaddrman`. When initializing a node with this flag, it will delete addresses from addrman (both new and tried tables) that do not belong to the supported networks (e.g. `-onlynet`). Deleting addresses that do not belong to the supported networks can avoid a lot of unnecessary addrman `Select` calls (especially when turning from clearnet + Tor/I2P/CJDNS to Tor/I2P/CJDNS). This flag is not enabled by default.
💬 stickies-v commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1912691682)
> Does this mean that the adjusted time is now being collected and used exclusively to raise the warning?
This was the case before the force-push, too. In this current approach, the diff is much smaller so review can focus more on whether we're okay removing network-adjusted time, instead of on the adjusted time calculation refactoring.
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1912691682)
> Does this mean that the adjusted time is now being collected and used exclusively to raise the warning?
This was the case before the force-push, too. In this current approach, the diff is much smaller so review can focus more on whether we're okay removing network-adjusted time, instead of on the adjusted time calculation refactoring.
💬 brunoerg commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1912698266)
cc: @mzumsande
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1912698266)
cc: @mzumsande
📝 russeree opened a pull request: "redeclare nChainTx to use uint64_t"
(https://github.com/bitcoin/bitcoin/pull/29331)
Following up on https://github.com/bitcoin/bitcoin/issues/29258 the type of ```nChainTx``` has now been changed to a ```uint64_t``` allowing for the counting of TXs well into the future. The note about changing the type in 2024 was also removed. This passes all extended tests.
(https://github.com/bitcoin/bitcoin/pull/29331)
Following up on https://github.com/bitcoin/bitcoin/issues/29258 the type of ```nChainTx``` has now been changed to a ```uint64_t``` allowing for the counting of TXs well into the future. The note about changing the type in 2024 was also removed. This passes all extended tests.