💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1465280323)
In commit "[refactor] Make MainSignals RAII styled" (dddaa962cfbeb96a8148535e74e1b53c9c717376)
Maybe unintended diff moving this line
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1465280323)
In commit "[refactor] Make MainSignals RAII styled" (dddaa962cfbeb96a8148535e74e1b53c9c717376)
Maybe unintended diff moving this line
👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1841942054)
Code review ACK bdd985af62ff9e94490ecc701d6063eaab172add. Letting signals be optional and moving more initialization to AppInitMain seem like nice changes among other cleanups.
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1841942054)
Code review ACK bdd985af62ff9e94490ecc701d6063eaab172add. Letting signals be optional and moving more initialization to AppInitMain seem like nice changes among other cleanups.
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1908780342)
> > Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
>
> what do we mean by ravaging?
@moonsettler low-resource devices tend to struggle with IBD once the utxoset no longer fits in memory. My own experience with this comes from helping users sync Raspberry Pi 4 devices for a couple of years while working at Start9. (The Raspberry Pi is especially bad because the bridge connecting external USB disks to the CPU doesn't work very well.) Once you
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1908780342)
> > Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
>
> what do we mean by ravaging?
@moonsettler low-resource devices tend to struggle with IBD once the utxoset no longer fits in memory. My own experience with this comes from helping users sync Raspberry Pi 4 devices for a couple of years while working at Start9. (The Raspberry Pi is especially bad because the bridge connecting external USB disks to the CPU doesn't work very well.) Once you
...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1908785694)
@ryanofsky thanks again for thorough review. I addressed all your comments, and rebased on master to fix the "tidy" CI failure. The one commit I am not fully confident in is 50debf781dbc4ac73cd1c0138ed9d405ea60eacb "rpc: use move semantics in JSONRPCReplyObj()" which is the commit I added to address
https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446816704
and
https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446847073
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1908785694)
@ryanofsky thanks again for thorough review. I addressed all your comments, and rebased on master to fix the "tidy" CI failure. The one commit I am not fully confident in is 50debf781dbc4ac73cd1c0138ed9d405ea60eacb "rpc: use move semantics in JSONRPCReplyObj()" which is the commit I added to address
https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446816704
and
https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446847073
👍 TheCharlatan approved a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1842217136)
Re-ACK f822ac9a24d684937f1258da89812e99c4b205ba
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1842217136)
Re-ACK f822ac9a24d684937f1258da89812e99c4b205ba
💬 chrisguida commented on issue "Prune Node Rescan Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/29183#issuecomment-1908843014)
Excellent, thanks!!
(https://github.com/bitcoin/bitcoin/issues/29183#issuecomment-1908843014)
Excellent, thanks!!
💬 chrisguida commented on issue "Prune Node Rescan Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/29183#issuecomment-1908844984)
What's the process for building the blockfilterindex on an already-synced pruned node? Enable blockfilterindex, reindex?
(https://github.com/bitcoin/bitcoin/issues/29183#issuecomment-1908844984)
What's the process for building the blockfilterindex on an already-synced pruned node? Enable blockfilterindex, reindex?
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1465515386)
I updated the function `JSONRPCReplyObj` to expect plain ol' `UniValue` for both response and error, and added `std::move()` where it seemed applicable. Tidy didn't like `std::move()` here because `objError` is already a `const`, so my understanding is that the error message will continue to be passed by reference through the end of `JSONRPCReplyObj` but my understanding is limited
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1465515386)
I updated the function `JSONRPCReplyObj` to expect plain ol' `UniValue` for both response and error, and added `std::move()` where it seemed applicable. Tidy didn't like `std::move()` here because `objError` is already a `const`, so my understanding is that the error message will continue to be passed by reference through the end of `JSONRPCReplyObj` but my understanding is limited
👍 willcl-ark approved a pull request: "doc: clarify `BroadcastTransaction` comment"
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1842344725)
ACK cca20d94b787d0881a61509445f4827f913e051d
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1842344725)
ACK cca20d94b787d0881a61509445f4827f913e051d
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1908900398)
Updated bdd985af62ff9e94490ecc701d6063eaab172add -> 536429372dfd9759dc8f089962f3a15a94b54751 ([noGlobalSignals_10](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_10) -> [noGlobalSignals_11](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_10..noGlobalSignals_11))
* Addresesd @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1465280323), reverting unintended d
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1908900398)
Updated bdd985af62ff9e94490ecc701d6063eaab172add -> 536429372dfd9759dc8f089962f3a15a94b54751 ([noGlobalSignals_10](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_10) -> [noGlobalSignals_11](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_10..noGlobalSignals_11))
* Addresesd @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1465280323), reverting unintended d
...
💬 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.