👍 dergoegge approved a pull request: "fuzz: Print coverage summary after run_once"
(https://github.com/bitcoin/bitcoin/pull/29329#pullrequestreview-1848840989)
ACK fab97d81ce3740509dbbe9270ca67a1b65b00c72
(https://github.com/bitcoin/bitcoin/pull/29329#pullrequestreview-1848840989)
ACK fab97d81ce3740509dbbe9270ca67a1b65b00c72
💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914863218)
> With Stratum v2 we do use the key. We need its pubkey, perform (Elligator-Swift) ECDH with it, etc. That's what a CKey is for.
Your 1st objection to using `CPrivKey` was that `CPrivKey` is always meant to be used in conjunction with a public key (implying that this SV2 key is not), but it sounds like we will also always be using a public key with the SV2 key, so I don't understand your 1st objection to using `CPrivKey` or why we would need a "dummy public key."
Your second point makes se
...
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914863218)
> With Stratum v2 we do use the key. We need its pubkey, perform (Elligator-Swift) ECDH with it, etc. That's what a CKey is for.
Your 1st objection to using `CPrivKey` was that `CPrivKey` is always meant to be used in conjunction with a public key (implying that this SV2 key is not), but it sounds like we will also always be using a public key with the SV2 key, so I don't understand your 1st objection to using `CPrivKey` or why we would need a "dummy public key."
Your second point makes se
...
📝 maflcko opened a pull request: "rpc: Do not wait for headers inside loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/29345)
While the `loadtxoutset` default 10 minute timeout is convenient when it is sufficient, it may cause hassle where it is not. For example:
* When P2P connections are missing, it seems better to abort early than wait for the timeout.
* When the 10 minute timeout is not sufficient, the RPC will have to be called again, so a check or loop is needed outside the RPC either way. So might as well remove the loop inside the RPC.
(https://github.com/bitcoin/bitcoin/pull/29345)
While the `loadtxoutset` default 10 minute timeout is convenient when it is sufficient, it may cause hassle where it is not. For example:
* When P2P connections are missing, it seems better to abort early than wait for the timeout.
* When the 10 minute timeout is not sufficient, the RPC will have to be called again, so a check or loop is needed outside the RPC either way. So might as well remove the loop inside the RPC.
💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1914922108)
The RPCs should probably be fixed to never return fake values.
It is pretty easy to tell when the `nTx` value is fake. You can just call [`IsValid()`](https://github.com/bitcoin/bitcoin/blob/478ac185be49feffd1231366c07e06068683f941/src/chain.h#L306), and if it returns true, the `nTx` value is real, and if it returns false, the value is fake.
It is less straightforward to determine when the `nChainTx` is fake, but we could introduce a helper function to do that which could be used by these
...
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1914922108)
The RPCs should probably be fixed to never return fake values.
It is pretty easy to tell when the `nTx` value is fake. You can just call [`IsValid()`](https://github.com/bitcoin/bitcoin/blob/478ac185be49feffd1231366c07e06068683f941/src/chain.h#L306), and if it returns true, the `nTx` value is real, and if it returns false, the value is fake.
It is less straightforward to determine when the `nChainTx` is fake, but we could introduce a helper function to do that which could be used by these
...
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914963270)
> it sounds like we will also always be using a public key with the SV2 key,
Yes, but we just derive the public key from the private key. There's no need to store it and check it. If it got corrupted, you just make a new one.
> what you are doing now is also incorrect and confusing in that you are serializing and deserializing 32 bytes with no indication it is meant for an `XOnlyPublicKey`
We don't make that distinction for Taproot wallet keys either. Any compressed key can be added to
...
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914963270)
> it sounds like we will also always be using a public key with the SV2 key,
Yes, but we just derive the public key from the private key. There's no need to store it and check it. If it got corrupted, you just make a new one.
> what you are doing now is also incorrect and confusing in that you are serializing and deserializing 32 bytes with no indication it is meant for an `XOnlyPublicKey`
We don't make that distinction for Taproot wallet keys either. Any compressed key can be added to
...
📝 Sjors opened a pull request: "Stratum v2 Noise Protocol"
(https://github.com/bitcoin/bitcoin/pull/29346)
Parent PR #28983
This is the first part of the Stratum v2 Template Provider change that can be a standalone pull request.
The Noise Protocol Framework is defined here: https://noiseprotocol.org/noise.html
It's quite similar to BIP324. The main differences are explained here, including why Stratum v2 can't use BIP234 (yet): https://delvingbitcoin.org/t/stratum-v2-noise-protocol-bip324-nuggets/413
The implementation is based on revision 38, 2018-07-11 (most recent at the time of writin
...
(https://github.com/bitcoin/bitcoin/pull/29346)
Parent PR #28983
This is the first part of the Stratum v2 Template Provider change that can be a standalone pull request.
The Noise Protocol Framework is defined here: https://noiseprotocol.org/noise.html
It's quite similar to BIP324. The main differences are explained here, including why Stratum v2 can't use BIP234 (yet): https://delvingbitcoin.org/t/stratum-v2-noise-protocol-bip324-nuggets/413
The implementation is based on revision 38, 2018-07-11 (most recent at the time of writin
...
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1915048388)
I opened a (draft) pull request for the Noise part of this: #29346
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1915048388)
I opened a (draft) pull request for the Noise part of this: #29346
💬 mzumsande commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1915053025)
> I don't know if this is true, but if it is true, it seems like `GuessVerificationProgress` could just report progress based on block heights instead of transaction counts
If we can do that, we should get rid of nChainTx altogether. I think that its only other use is for `HaveNumChainTxs` which doesn't use its value but checks whether it is 0. That would also save some memory and eliminate problems like #29258.
However, just using block heights would be very inaccurate because the first ~
...
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1915053025)
> I don't know if this is true, but if it is true, it seems like `GuessVerificationProgress` could just report progress based on block heights instead of transaction counts
If we can do that, we should get rid of nChainTx altogether. I think that its only other use is for `HaveNumChainTxs` which doesn't use its value but checks whether it is 0. That would also save some memory and eliminate problems like #29258.
However, just using block heights would be very inaccurate because the first ~
...
🚀 fanquake merged a pull request: "fuzz: Print coverage summary after run_once"
(https://github.com/bitcoin/bitcoin/pull/29329)
(https://github.com/bitcoin/bitcoin/pull/29329)
💬 mzumsande commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1915076242)
> Would this be a hard fork because it would be loosening the conditions for validation?
I don't think so, since I don't think the value of `nChainTx` is used in validation (or do you see a spot where it is?). It's used for logging (progress estimation), and whether it is 0 is used in `HaveNumChainTxs`. The latter could become problematic in case of an overflow, since if the `unsigned int` would overflow exactly to zero (which is a bit unlikely given that there are a few thousand txns in each
...
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1915076242)
> Would this be a hard fork because it would be loosening the conditions for validation?
I don't think so, since I don't think the value of `nChainTx` is used in validation (or do you see a spot where it is?). It's used for logging (progress estimation), and whether it is 0 is used in `HaveNumChainTxs`. The latter could become problematic in case of an overflow, since if the `unsigned int` would overflow exactly to zero (which is a bit unlikely given that there are a few thousand txns in each
...
💬 maflcko commented on pull request "ci: Use LLVM 17.0.6 & DEBUG=1 in depends for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27495#issuecomment-1915093662)
lgtm ACK 8531e1e7312efe66204dc8ce56f21e44de99e956
(https://github.com/bitcoin/bitcoin/pull/27495#issuecomment-1915093662)
lgtm ACK 8531e1e7312efe66204dc8ce56f21e44de99e956
💬 russeree commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1915126021)
^^^ The above was the line that I was referencing. Thank you.
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1915126021)
^^^ The above was the line that I was referencing. Thank you.
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1915132178)
Pushed up one WIP commit. This almost works how we want it too:
* Depends builds using system clang, i.e `make -C depends/ HOST=arm64-apple-darwin -j17`. Tested on x86_64 Ubuntu with `Ubuntu clang version 17.0.6 (3)`.
* Depends builds with overriden `CC` & `CXX`, i.e `make -C depends/ HOST=arm64-apple-darwin CC=clang-17 CXX=clang++-17`. See the CI (which fails during `make deploy` due to no `otool`), also tested on aarch64 Fedora with `clang version 17.0.6 (Fedora 17.0.6-4.fc40)`.
* Guix buil
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1915132178)
Pushed up one WIP commit. This almost works how we want it too:
* Depends builds using system clang, i.e `make -C depends/ HOST=arm64-apple-darwin -j17`. Tested on x86_64 Ubuntu with `Ubuntu clang version 17.0.6 (3)`.
* Depends builds with overriden `CC` & `CXX`, i.e `make -C depends/ HOST=arm64-apple-darwin CC=clang-17 CXX=clang++-17`. See the CI (which fails during `make deploy` due to no `otool`), also tested on aarch64 Fedora with `clang version 17.0.6 (Fedora 17.0.6-4.fc40)`.
* Guix buil
...
🚀 fanquake merged a pull request: "ci: Use LLVM 17.0.6 & DEBUG=1 in depends for MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27495)
(https://github.com/bitcoin/bitcoin/pull/27495)
💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1915158550)
> If we can do that, we should get rid of nChainTx altogether.
Oh, I don't think we should use block heights instead of _real_ nChainTx values, because as you say using block heights would be very inaccurate. I'm just suggesting we could use block heights instead of _fake_ nChainTx values, and we would not be worse off because currently the fake nChainTx values are set to block heights. Also probably it probably be good to estimate the number of transactions in BLOCK_ASSUMED_VALID blocks by i
...
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1915158550)
> If we can do that, we should get rid of nChainTx altogether.
Oh, I don't think we should use block heights instead of _real_ nChainTx values, because as you say using block heights would be very inaccurate. I'm just suggesting we could use block heights instead of _fake_ nChainTx values, and we would not be worse off because currently the fake nChainTx values are set to block heights. Also probably it probably be good to estimate the number of transactions in BLOCK_ASSUMED_VALID blocks by i
...
👍 stickies-v approved a pull request: "test: Treat msg_version.relay as unsigned, Remove `struct` packing in messages.py"
(https://github.com/bitcoin/bitcoin/pull/29067#pullrequestreview-1849240494)
ACK 55556a64a8e4e6238f990cf66295c3b9594c2c3d
I agree with the rationale in the PR description. The code with `to_bytes` and `from_bytes` is easier to understand without having to consult documentation. In that philosophy, I would also be okay with `signed=False` being explicit in these calls, since the default value (`False`) is not immediately obvious imo, and quite important.
However, it's not something that we enforce, so maybe it's not a good idea. It's also a very quick thing to look up
...
(https://github.com/bitcoin/bitcoin/pull/29067#pullrequestreview-1849240494)
ACK 55556a64a8e4e6238f990cf66295c3b9594c2c3d
I agree with the rationale in the PR description. The code with `to_bytes` and `from_bytes` is easier to understand without having to consult documentation. In that philosophy, I would also be okay with `signed=False` being explicit in these calls, since the default value (`False`) is not immediately obvious imo, and quite important.
However, it's not something that we enforce, so maybe it's not a good idea. It's also a very quick thing to look up
...
💬 theStack commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915193167)
> > 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 derived? Also many of the pruned outpoints are not compressed pubkeys but instead uncompressed.
In my own non-cryptographer words: for about half of field elements x, there exists no field element y such that the secp256k1 equation $y^2 = x^3 + 7$ hol
...
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915193167)
> > 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 derived? Also many of the pruned outpoints are not compressed pubkeys but instead uncompressed.
In my own non-cryptographer words: for about half of field elements x, there exists no field element y such that the secp256k1 equation $y^2 = x^3 + 7$ hol
...
💬 ajtowns commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915195487)
> > 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 derived? Also many of the pruned outpoints are not compressed pubkeys but instead uncompressed.
The formula is `y^2 = x^3 + 7`, so you derive `x^3+7` for any `x`, but only half of those will be valid squares -- every number `0 < i < p/2` squares to th
...
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915195487)
> > 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 derived? Also many of the pruned outpoints are not compressed pubkeys but instead uncompressed.
The formula is `y^2 = x^3 + 7`, so you derive `x^3+7` for any `x`, but only half of those will be valid squares -- every number `0 < i < p/2` squares to th
...
💬 LarryRuane commented on pull request "bitcoin-cli help detail to show full help for all RPCs":
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1915200978)
> I dont think this is needed
I agree this is not needed, but does every change have to be "needed"? Can't it just be an improvement? This would be helpful to me personally, and by extension, I would expect to others as well.
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1915200978)
> I dont think this is needed
I agree this is not needed, but does every change have to be "needed"? Can't it just be an improvement? This would be helpful to me personally, and by extension, I would expect to others as well.
🚀 achow101 merged a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748)
(https://github.com/bitcoin/bitcoin/pull/24748)