✅ fanquake closed an issue: "Clang 14 emits `-Wunreachable-code` warnings"
(https://github.com/bitcoin/bitcoin/issues/29334)
(https://github.com/bitcoin/bitcoin/issues/29334)
💬 fanquake commented on issue "Clang 14 emits `-Wunreachable-code` warnings":
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1914514298)
Closing as upgrade compiler / pass a flag to disable the false positive / don't pass error flags to turn known false positives into compile failures.
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1914514298)
Closing as upgrade compiler / pass a flag to disable the false positive / don't pass error flags to turn known false positives into compile failures.
👍 TheCharlatan approved a pull request: "refactor: Fix prevector iterator concept issues"
(https://github.com/bitcoin/bitcoin/pull/29275#pullrequestreview-1848469672)
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547
Could the `bitdeque` iterator also get `std::contiguous_iterator_tag`?
(https://github.com/bitcoin/bitcoin/pull/29275#pullrequestreview-1848469672)
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547
Could the `bitdeque` iterator also get `std::contiguous_iterator_tag`?
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469469780)
Done. Would be nice to do this in a pre-commit hook or something, because it's unlikely I'm going to remember doing this every time.
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469469780)
Done. Would be nice to do this in a pre-commit hook or something, because it's unlikely I'm going to remember doing this every time.
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469469850)
Done in both places.
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469469850)
Done in both places.
💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914521350)
> Why would you not use a `CKey` to hold a private key?
My point was not "CKey shouldn't be used for private keys." My point is `CKey` (or at least my understanding of it) is meant for a *specific* kind(s) of "private key" i.e. a private key that represents a private/public key pair and is intended to be used in that context. It also seems we want to know where the key came from and ensure that it is valid, hence the only ways to load a secret key are the `DecodeSecret` method, which decodes
...
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914521350)
> Why would you not use a `CKey` to hold a private key?
My point was not "CKey shouldn't be used for private keys." My point is `CKey` (or at least my understanding of it) is meant for a *specific* kind(s) of "private key" i.e. a private key that represents a private/public key pair and is intended to be used in that context. It also seems we want to know where the key came from and ensure that it is valid, hence the only ways to load a secret key are the `DecodeSecret` method, which decodes
...
💬 t-bast commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1914523198)
> (eg how do we handle 0conf ln channels? I need to make a delving post to discuss this)
I'd be curious to see your delvingbitcoin post around that, as I believe this shouldn't be too much of an issue (but let's discuss it on a dedicated post instead of here).
> 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.
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1914523198)
> (eg how do we handle 0conf ln channels? I need to make a delving post to discuss this)
I'd be curious to see your delvingbitcoin post around that, as I believe this shouldn't be too much of an issue (but let's discuss it on a dedicated post instead of here).
> 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.
...
💬 maflcko commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1914527368)
I'd say they are fine, if there is no better way. With global functions and variables, I think the only way to mock them is also via a global approach. Here, adding the `assert(params!=main)` (https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1446512814) should be fine.
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1914527368)
I'd say they are fine, if there is no better way. With global functions and variables, I think the only way to mock them is also via a global approach. Here, adding the `assert(params!=main)` (https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1446512814) should be fine.
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914527731)
> > It's more similar to how we handle Tor v3 and I2P private keys.
>
> I did a quick grep for `CKey` and `<key.h>` and didn't see anything related to Tor or I2P. Can you provide an example of what you mean here, because I'm not sure I fully understand your point.
See #29229: they don't use `CKey`, but instead are stored as plain text. They're similar in the sense that they're used as an identity key on the network and that we store them in the data dir. But we can't use `CKey` for them be
...
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914527731)
> > It's more similar to how we handle Tor v3 and I2P private keys.
>
> I did a quick grep for `CKey` and `<key.h>` and didn't see anything related to Tor or I2P. Can you provide an example of what you mean here, because I'm not sure I fully understand your point.
See #29229: they don't use `CKey`, but instead are stored as plain text. They're similar in the sense that they're used as an identity key on the network and that we store them in the data dir. But we can't use `CKey` for them be
...
💬 fanquake commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1914540362)
On the `std::format` front, looks like as of the next Xcode release (15.3), Apple is [adding support for `std::format`](https://developer.apple.com/documentation/xcode-release-notes/xcode-15_3-release-notes#C++-Standard-Library):
> The following new features have been implemented:
> P0645 - Text formatting (std::format)
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1914540362)
On the `std::format` front, looks like as of the next Xcode release (15.3), Apple is [adding support for `std::format`](https://developer.apple.com/documentation/xcode-release-notes/xcode-15_3-release-notes#C++-Standard-Library):
> The following new features have been implemented:
> P0645 - Text formatting (std::format)
💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914547962)
> they don't use CKey, but instead are stored as plain text. They're similar in the sense that they're used as an identity key on the network and that we store them in the data dir.
Why can't we do the same for `StratumV2` identity?
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914547962)
> they don't use CKey, but instead are stored as plain text. They're similar in the sense that they're used as an identity key on the network and that we store them in the data dir.
Why can't we do the same for `StratumV2` identity?
👍 fanquake approved a pull request: "depends: patch libool out of libnatpmp/miniupnpc"
(https://github.com/bitcoin/bitcoin/pull/29298#pullrequestreview-1848504702)
ACK 5b9d5bf866506b22270598aa2dc1269bc02e38e2
Guix Build (aarch64):
```bash
8b32a57c3b091605070673f6338768f6ee3d5731a553ae96b426827afaf8b8f1 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/SHA256SUMS.part
02a85cac626984fb8d0ea14b449af6ea916c9a3f9f9e3d68137505f5cddded60 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/bitcoin-5b9d5bf86650-aarch64-linux-gnu-debug.tar.gz
f504943f0cea5bfa512a8677c8d0d1968eabcfc5a17d3fc7b810e098d7303019 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/bitco
...
(https://github.com/bitcoin/bitcoin/pull/29298#pullrequestreview-1848504702)
ACK 5b9d5bf866506b22270598aa2dc1269bc02e38e2
Guix Build (aarch64):
```bash
8b32a57c3b091605070673f6338768f6ee3d5731a553ae96b426827afaf8b8f1 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/SHA256SUMS.part
02a85cac626984fb8d0ea14b449af6ea916c9a3f9f9e3d68137505f5cddded60 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/bitcoin-5b9d5bf86650-aarch64-linux-gnu-debug.tar.gz
f504943f0cea5bfa512a8677c8d0d1968eabcfc5a17d3fc7b810e098d7303019 guix-build-5b9d5bf86650/output/aarch64-linux-gnu/bitco
...
💬 maflcko commented on pull request "refactor: Fix prevector iterator concept issues":
(https://github.com/bitcoin/bitcoin/pull/29275#issuecomment-1914560861)
> Could the `bitdeque` iterator also get `std::contiguous_iterator_tag`?
No. As opposed to [std::vector](https://en.cppreference.com/w/cpp/container/vector), the elements of a deque are not stored contiguously: typical implementations use a sequence of individually allocated fixed-size arrays, with additional bookkeeping, which means indexed access to deque must perform two pointer dereferences, compared to vector's indexed access which performs only one. (https://en.cppreference.com/w/cpp/co
...
(https://github.com/bitcoin/bitcoin/pull/29275#issuecomment-1914560861)
> Could the `bitdeque` iterator also get `std::contiguous_iterator_tag`?
No. As opposed to [std::vector](https://en.cppreference.com/w/cpp/container/vector), the elements of a deque are not stored contiguously: typical implementations use a sequence of individually allocated fixed-size arrays, with additional bookkeeping, which means indexed access to deque must perform two pointer dereferences, compared to vector's indexed access which performs only one. (https://en.cppreference.com/w/cpp/co
...
🚀 fanquake merged a pull request: "depends: patch libool out of libnatpmp/miniupnpc"
(https://github.com/bitcoin/bitcoin/pull/29298)
(https://github.com/bitcoin/bitcoin/pull/29298)
💬 ismaelsadeeq commented on pull request "doc: update `BroadcastTransaction` comment":
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1469501085)
Yes I agree with your point.
Reverting back with your initial suggestion.
Thanks @stickies-v
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1469501085)
Yes I agree with your point.
Reverting back with your initial suggestion.
Thanks @stickies-v
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914568318)
Why a convert a key to a plain text hex string if you can just store the bytes? `CKey` also takes care to not leave key material dangling in memory in temporary variables.
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914568318)
Why a convert a key to a plain text hex string if you can just store the bytes? `CKey` also takes care to not leave key material dangling in memory in temporary variables.
💬 maflcko commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1914568330)
Not sure if there is any value in `std::format`, because it is locale dependent. Seems better to stick to tinyformat?
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1914568330)
Not sure if there is any value in `std::format`, because it is locale dependent. Seems better to stick to tinyformat?
💬 NicolasDorier commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914570114)
Concept ACK.
However, note that from the time this PR run on a node, its UTXO Set hash will start to differ from any peer that didn't start using this PR at the same time.
This can be solved by cleaning up the UTXO Set when the node starts. Unsure how expensive would it be.
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914570114)
Concept ACK.
However, note that from the time this PR run on a node, its UTXO Set hash will start to differ from any peer that didn't start using this PR at the same time.
This can be solved by cleaning up the UTXO Set when the node starts. Unsure how expensive would it be.
💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914583600)
My question was why not handle StratumV2 keys the same way we are handling Tor and I2P identity keys and not use `CKey` at all. This avoids needing to add generic `Serialize/Deserialize` methods to `CKey` just for this specific use case, and also avoids needing to go write a specific SV2 key serialization format and corresponding `DecodeSV2Secret` method for `CKey`.
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914583600)
My question was why not handle StratumV2 keys the same way we are handling Tor and I2P identity keys and not use `CKey` at all. This avoids needing to add generic `Serialize/Deserialize` methods to `CKey` just for this specific use case, and also avoids needing to go write a specific SV2 key serialization format and corresponding `DecodeSV2Secret` method for `CKey`.
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1469517392)
I dont understand why you said it is not a wallet option.
The `OptionsCategory` of this startup option is `WALLET`.
And its only used in the wallet.
Can you please expand on your comment.
Thanks
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1469517392)
I dont understand why you said it is not a wallet option.
The `OptionsCategory` of this startup option is `WALLET`.
And its only used in the wallet.
Can you please expand on your comment.
Thanks