💬 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
🤔 glozow reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1848564888)
ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
Did some light code review of the `P2PConnection` functions, mutation testing of `EncryptedP2PState`, and manually changing other functional tests to use v2 connections.
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1848564888)
ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
Did some light code review of the `P2PConnection` functions, mutation testing of `EncryptedP2PState`, and manually changing other functional tests to use v2 connections.
💬 russeree commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914628448)
> and are thus provably unspendable about 50% of the time as the data doesn't match a valid secp point.
Sorry for not understanding this, but why 50%? I thought compressed pubkeys covered almost everything on the X axis because the y is generated?
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914628448)
> and are thus provably unspendable about 50% of the time as the data doesn't match a valid secp point.
Sorry for not understanding this, but why 50%? I thought compressed pubkeys covered almost everything on the X axis because the y is generated?
👍 stickies-v approved a pull request: "doc: update `BroadcastTransaction` comment"
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1848610213)
ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1848610213)
ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea
💬 theDavidCoen commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1914655681)
> @theDavidCoen "Default off everything" is not at all how Bitcoin Core development works. If it did, the node would by default not communicate with the network (`-networkactive`), broadcast wallet transactions (`-walletbroadcast`), persist the mempool between restarts (`-persistmempool`) etc. It would also mean you can just rename an option (e.g. `-permitbaremultisig` to `-disablebaremultisig`) to force the opposite default.
>
> How it actually (usually) works is the default parameters are c
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1914655681)
> @theDavidCoen "Default off everything" is not at all how Bitcoin Core development works. If it did, the node would by default not communicate with the network (`-networkactive`), broadcast wallet transactions (`-walletbroadcast`), persist the mempool between restarts (`-persistmempool`) etc. It would also mean you can just rename an option (e.g. `-permitbaremultisig` to `-disablebaremultisig`) to force the opposite default.
>
> How it actually (usually) works is the default parameters are c
...