💬 TheCharlatan commented on pull request "depends: patch libool out of libnatpmp/miniupnpc":
(https://github.com/bitcoin/bitcoin/pull/29298#discussion_r1465542113)
Can the ` $(package)_build_opts_darwin=LIBTOOL="$($(package)_libtool)"` line above (and same for miniupnpc) be removed now? I removed them and things compiled fine.
(https://github.com/bitcoin/bitcoin/pull/29298#discussion_r1465542113)
Can the ` $(package)_build_opts_darwin=LIBTOOL="$($(package)_libtool)"` line above (and same for miniupnpc) be removed now? I removed them and things compiled fine.
💬 morehouse commented on pull request "RBF: Require unconfirmed inputs to come from a single conflicting transaction":
(https://github.com/bitcoin/bitcoin/pull/29297#issuecomment-1908932220)
As another benefit, I think this change helps prevent replacement cycling attacks against LN. The attacker would be unable to make their preimage spend depend on a second unconfirmed transaction, and therefore couldn't evict their preimage spend.
(https://github.com/bitcoin/bitcoin/pull/29297#issuecomment-1908932220)
As another benefit, I think this change helps prevent replacement cycling attacks against LN. The attacker would be unable to make their preimage spend depend on a second unconfirmed transaction, and therefore couldn't evict their preimage spend.
💬 achow101 commented on pull request "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs":
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-1908951226)
ACK fa08291375ea00cfacd52956bcac7468824a0bf4
With #25273 now merged, I believe this now applies the anti-fee-sniping consistently. It won't apply it to any transactions that are created by calling `FundTransaction` (that should only affect `fundrawtransaction` and `walletcreatefundedpsbt`) since those transactions always have preset sequences. But that can be improved in the future and this will at least behave consistently.
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-1908951226)
ACK fa08291375ea00cfacd52956bcac7468824a0bf4
With #25273 now merged, I believe this now applies the anti-fee-sniping consistently. It won't apply it to any transactions that are created by calling `FundTransaction` (that should only affect `fundrawtransaction` and `walletcreatefundedpsbt`) since those transactions always have preset sequences. But that can be improved in the future and this will at least behave consistently.
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1465595068)
Oh, I thought the `recvbuf` was a growing-only buffer that was not cleared until the handshake was over.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1465595068)
Oh, I thought the `recvbuf` was a growing-only buffer that was not cleared until the handshake was over.
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1908992212)
> but I'm guessing that since [the utxoset has doubled in size over the last year](https://statoshi.info/d/000000009/unspent-transaction-output-set?orgId=1&refresh=10m&from=now-1y&to=now&viewPanel=8), these devices too will soon start to become unviable.
UTXO set has not doubled because of bare multisig: https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666471945
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1908992212)
> but I'm guessing that since [the utxoset has doubled in size over the last year](https://statoshi.info/d/000000009/unspent-transaction-output-set?orgId=1&refresh=10m&from=now-1y&to=now&viewPanel=8), these devices too will soon start to become unviable.
UTXO set has not doubled because of bare multisig: https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666471945
💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1908994332)
> UTXO set has not doubled because of bare multisig
we know it, unfortunately no mempool option for inscriptions is available at the moment, so even if Baremultisig is less used it remains spam for my node.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1908994332)
> UTXO set has not doubled because of bare multisig
we know it, unfortunately no mempool option for inscriptions is available at the moment, so even if Baremultisig is less used it remains spam for my node.
💬 furszy commented on pull request "init: settings, do not load auto-generated warning msg":
(https://github.com/bitcoin/bitcoin/pull/29301#discussion_r1465610725)
> In commit "init: settings, do not load auto-generated warning msg" ([987a1b5](https://github.com/bitcoin/bitcoin/commit/987a1b51eeb72c7fcb085470817a4fe85fcc3c7c))
>
> Not sure, but maybe this can be changed to std::string_view to avoid the need to allocate memory before main() runs.
As the settings map take strings for keys, wouldn't this change mean that we would need to static cast string_view to string to not create a copy during the push/erase methods call?
(https://github.com/bitcoin/bitcoin/pull/29301#discussion_r1465610725)
> In commit "init: settings, do not load auto-generated warning msg" ([987a1b5](https://github.com/bitcoin/bitcoin/commit/987a1b51eeb72c7fcb085470817a4fe85fcc3c7c))
>
> Not sure, but maybe this can be changed to std::string_view to avoid the need to allocate memory before main() runs.
As the settings map take strings for keys, wouldn't this change mean that we would need to static cast string_view to string to not create a copy during the push/erase methods call?
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909013326)
> > but I'm guessing that since [the utxoset has doubled in size over the last year](https://statoshi.info/d/000000009/unspent-transaction-output-set?orgId=1&refresh=10m&from=now-1y&to=now&viewPanel=8), these devices too will soon start to become unviable.
>
> UTXO set has not doubled because of bare multisig: [#28217 (comment)](https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666471945)
Yes, I know. Just because there isn't a perfect solution for all utxoset abuse doesn't mean
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909013326)
> > but I'm guessing that since [the utxoset has doubled in size over the last year](https://statoshi.info/d/000000009/unspent-transaction-output-set?orgId=1&refresh=10m&from=now-1y&to=now&viewPanel=8), these devices too will soon start to become unviable.
>
> UTXO set has not doubled because of bare multisig: [#28217 (comment)](https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666471945)
Yes, I know. Just because there isn't a perfect solution for all utxoset abuse doesn't mean
...
👍 TheCharlatan approved a pull request: "set `DEFAULT_PERMIT_BAREMULTISIG` to false"
(https://github.com/bitcoin/bitcoin/pull/28217#pullrequestreview-1842509290)
ACK 8672721deb06e66854a085c9994e99c99160b8a1
Close to the only current usage of P2MS is data embedding and meta protocols, of which the majority is through the Counterparty protocol. I am not familiar with the Counterparty protocol, so I might have gotten some points here wrong. The [Bitcoin Stamps](https://github.com/mikeinspace/stamps/blob/main/BitcoinStamps.md), which are mentioned throughout this pull request discussion, are built with Counterparty transactions. P2MS is one of three metho
...
(https://github.com/bitcoin/bitcoin/pull/28217#pullrequestreview-1842509290)
ACK 8672721deb06e66854a085c9994e99c99160b8a1
Close to the only current usage of P2MS is data embedding and meta protocols, of which the majority is through the Counterparty protocol. I am not familiar with the Counterparty protocol, so I might have gotten some points here wrong. The [Bitcoin Stamps](https://github.com/mikeinspace/stamps/blob/main/BitcoinStamps.md), which are mentioned throughout this pull request discussion, are built with Counterparty transactions. P2MS is one of three metho
...
💬 furszy commented on issue "Prune Node Rescan Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/29183#issuecomment-1909019274)
> What's the process for building the blockfilterindex on an already-synced pruned node? Enable blockfilterindex, reindex?
Yes. For further node support, please come to IRC or use bitcoin.stackexchange. This is a project tracking issue.
(https://github.com/bitcoin/bitcoin/issues/29183#issuecomment-1909019274)
> What's the process for building the blockfilterindex on an already-synced pruned node? Enable blockfilterindex, reindex?
Yes. For further node support, please come to IRC or use bitcoin.stackexchange. This is a project tracking issue.
💬 DoctorBuzz1 commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909020625)
> UTXO set has not doubled because of bare multisig: [#28217 (comment)](https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666471945)
UTXO has rapidly increased over the past year. It *started* with Inscriptions but is now primarily due to BRC-20 tokens. A static dust limit of 3 sat /vByte fees makes zero sense anymore, when those fees have only been seen like for a hot second since the spam started a year ago... https://statoshi.info/d/000000009/unspent-transaction-output-set?orgI
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909020625)
> UTXO set has not doubled because of bare multisig: [#28217 (comment)](https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666471945)
UTXO has rapidly increased over the past year. It *started* with Inscriptions but is now primarily due to BRC-20 tokens. A static dust limit of 3 sat /vByte fees makes zero sense anymore, when those fees have only been seen like for a hot second since the spam started a year ago... https://statoshi.info/d/000000009/unspent-transaction-output-set?orgI
...
💬 ns-xvrn commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909047847)
ACK 8672721deb06e66854a085c9994e99c99160b8a1
Tested locally, all unit and functional tests pass.
Transaction tested on mainnet using txid: `883b35fbd5a8b703574752b879a027ef81ae091b0bc61527309b0cf53760854f` (which worked fine without this change and confirmed using `getmempoolentry` and `getrawtransaction` after doing `sendrawtransaction` while it was still in the mempool). Output of `sendrawtransaction` after compiling and running this PR:
```
error code: -26
error message:
bare-multi
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909047847)
ACK 8672721deb06e66854a085c9994e99c99160b8a1
Tested locally, all unit and functional tests pass.
Transaction tested on mainnet using txid: `883b35fbd5a8b703574752b879a027ef81ae091b0bc61527309b0cf53760854f` (which worked fine without this change and confirmed using `getmempoolentry` and `getrawtransaction` after doing `sendrawtransaction` while it was still in the mempool). Output of `sendrawtransaction` after compiling and running this PR:
```
error code: -26
error message:
bare-multi
...
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909048780)
Since DrahtBot has incorrectly assumed one of my previous comment as ` A-C-K`, this comment is to correct it:
NACK
Reason: https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860915662
Apart from the reason shared in linked comment, other reasons why this pull request makes no sense:
1. Some nodes will keep using older versions of core and this is enough to relay bare mul
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909048780)
Since DrahtBot has incorrectly assumed one of my previous comment as ` A-C-K`, this comment is to correct it:
NACK
Reason: https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860915662
Apart from the reason shared in linked comment, other reasons why this pull request makes no sense:
1. Some nodes will keep using older versions of core and this is enough to relay bare mul
...
👍 ryanofsky approved a pull request: "validation: improve checkblockindex comments"
(https://github.com/bitcoin/bitcoin/pull/29299#pullrequestreview-1842689957)
Code review ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5. Thanks for figuring this issue out and fixing it. Would suggest change pr name from "improve comments" to "fix misleading comments" since previous comments were wrong about the reasons the conditions are needed.
I also think it would be good to split up the assert and make it stricter, so feel free to use the code from https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1908513362 to use in the second commit, if that help. (Wo
...
(https://github.com/bitcoin/bitcoin/pull/29299#pullrequestreview-1842689957)
Code review ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5. Thanks for figuring this issue out and fixing it. Would suggest change pr name from "improve comments" to "fix misleading comments" since previous comments were wrong about the reasons the conditions are needed.
I also think it would be good to split up the assert and make it stricter, so feel free to use the code from https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1908513362 to use in the second commit, if that help. (Wo
...
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909202976)
@DoctorBuzz1 you should make an issue/PR formally proposing your idea, I would be happy to test and review. This PR discussion is probably not the best place.
@ns-xvrn I'm also happy to help review and test any PR for #29285
@1440000bytes as was stated earlier, incremental progress is better than no progress. It sounds like you would only be happy with a perfect solution, which of course is impossible.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1909202976)
@DoctorBuzz1 you should make an issue/PR formally proposing your idea, I would be happy to test and review. This PR discussion is probably not the best place.
@ns-xvrn I'm also happy to help review and test any PR for #29285
@1440000bytes as was stated earlier, incremental progress is better than no progress. It sounds like you would only be happy with a perfect solution, which of course is impossible.
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1465818605)
I dislike using `auto` both here and the default. It could conceivably not be a type that supports all permission masks.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1465818605)
I dislike using `auto` both here and the default. It could conceivably not be a type that supports all permission masks.
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1465819385)
Feels weird to log this unconditionally. Maybe the option should just be unavailable (and implicitly error) on Windows, and hard-code setting it user-read-only here?
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1465819385)
Feels weird to log this unconditionally. Maybe the option should just be unavailable (and implicitly error) on Windows, and hard-code setting it user-read-only here?
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1465819696)
Missing a space in the indentation
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1465819696)
Missing a space in the indentation
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1465820310)
Feels like a hack
```suggestion
for perm in "440", "640", "444":
test_perm(perm)
```
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1465820310)
Feels like a hack
```suggestion
for perm in "440", "640", "444":
test_perm(perm)
```
📝 vostrnad opened a pull request: "Add a `-permitbarepubkey` option"
(https://github.com/bitcoin/bitcoin/pull/29309)
Fixes #29285 (adds the option but leaves the default policy unchanged)
No tests so far, if there's consensus to move forward with this I'll look into adding them.
Disclaimer: I don't care if this gets merged, personally I don't see any benefit of having this option (or `-permitbaremultisig`) and I would object to changing the default policy in this regard (similar to my objections to #28217). However, as I think someone would inevitably open a PR for this, I thought this might be a good op
...
(https://github.com/bitcoin/bitcoin/pull/29309)
Fixes #29285 (adds the option but leaves the default policy unchanged)
No tests so far, if there's consensus to move forward with this I'll look into adding them.
Disclaimer: I don't care if this gets merged, personally I don't see any benefit of having this option (or `-permitbaremultisig`) and I would object to changing the default policy in this regard (similar to my objections to #28217). However, as I think someone would inevitably open a PR for this, I thought this might be a good op
...