👍 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)
💬 mzumsande commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1915247542)
I see, I misunderstood. Still might also be worth thinking about getting rid of `nChainTx` in the long run (which has been discussed before in #13875), but that is off-topic here.
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1915247542)
I see, I misunderstood. Still might also be worth thinking about getting rid of `nChainTx` in the long run (which has been discussed before in #13875), but that is off-topic here.
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469976331)
Since `newVersion` is an `int64_t`, I think this still needs to be cast to `uint32_t`.
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469976331)
Since `newVersion` is an `int64_t`, I think this still needs to be cast to `uint32_t`.
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469976552)
Done
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469976552)
Done
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469976658)
Done
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469976658)
Done
💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1915255193)
> * [CURRENT_VERSION](https://github.com/bitcoin/bitcoin/blob/3a41d8ca8481ce7f85b1c9a9638d63e8e5fa5285/src/primitives/transaction.h#L299) should probably also change to `uint32_t`
>
> * CI failes [here](https://github.com/bitcoin/bitcoin/blob/3a41d8ca8481ce7f85b1c9a9638d63e8e5fa5285/src/test/fuzz/util.cpp#L46-L48) due to UB, can be fixed by changing to `ConsumeIntegral<uint32_t>()`
Updated these.
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1915255193)
> * [CURRENT_VERSION](https://github.com/bitcoin/bitcoin/blob/3a41d8ca8481ce7f85b1c9a9638d63e8e5fa5285/src/primitives/transaction.h#L299) should probably also change to `uint32_t`
>
> * CI failes [here](https://github.com/bitcoin/bitcoin/blob/3a41d8ca8481ce7f85b1c9a9638d63e8e5fa5285/src/test/fuzz/util.cpp#L46-L48) due to UB, can be fixed by changing to `ConsumeIntegral<uint32_t>()`
Updated these.
🤔 instagibbs reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1849118641)
reviewed through 46eacf922b40b0e5edd03b8dfff8002e65a3ede5
with some test questions and nits. I checked the major topological checks have test coverage, ran fuzzer for quite a while with `ApplyV3Rules` disabled in package context to check that `PackageV3Checks` is sufficient for package context.
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1849118641)
reviewed through 46eacf922b40b0e5edd03b8dfff8002e65a3ede5
with some test questions and nits. I checked the major topological checks have test coverage, ran fuzzer for quite a while with `ApplyV3Rules` disabled in package context to check that `PackageV3Checks` is sufficient for package context.
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469920329)
this doesn't have functional or unit test coverage, and this also seems difficult to hit in functional tests.
I think it can be hit in unit tests by creating the necessary in-mempool parent-child chain, then calling `PackageV3Checks` with just the single tx.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469920329)
this doesn't have functional or unit test coverage, and this also seems difficult to hit in functional tests.
I think it can be hit in unit tests by creating the necessary in-mempool parent-child chain, then calling `PackageV3Checks` with just the single tx.
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469908727)
this doesn't have coverage in functional or unit tests, though I'm not sure this can be hit?
TxA(in mempool) -> TxB -> TxC
this is the minimal topology, right? However, this condition will trigger failure at https://github.com/bitcoin/bitcoin/pull/28948/files#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R53
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469908727)
this doesn't have coverage in functional or unit tests, though I'm not sure this can be hit?
TxA(in mempool) -> TxB -> TxC
this is the minimal topology, right? However, this condition will trigger failure at https://github.com/bitcoin/bitcoin/pull/28948/files#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R53
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469979561)
maybe use `TX_MAX_STANDARD_VERSION` instead
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469979561)
maybe use `TX_MAX_STANDARD_VERSION` instead
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469896676)
1005 is pretty magical, though I realize we have no way of actually computing vsize of a child not in mempool. sad!
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469896676)
1005 is pretty magical, though I realize we have no way of actually computing vsize of a child not in mempool. sad!
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469866501)
```suggestion
std::optional<std::string> SingleV3Rules(const CTransactionRef& ptx,
```
can leave a comment saying that it can *also* be called on transactions in packages, as an optimization.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1469866501)
```suggestion
std::optional<std::string> SingleV3Rules(const CTransactionRef& ptx,
```
can leave a comment saying that it can *also* be called on transactions in packages, as an optimization.
💬 achow101 commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915276592)
> Or is there a use case for caring about the total fees you pay AND caring about the sats per vbyte you pay that I'm not understanding?
I think it makes sense to have both. I could have a `maxtxfee` of the upper bound of what I'm willing to pay ever, and also set `maxfeerate` to something reasonable just to make sure I don't typo a feerate and accidentally pay a feerate orders of magnitude higher than intended. At `maxfeerate`, a small transaction can still pay a smaller fee than `maxtxfee`,
...
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915276592)
> Or is there a use case for caring about the total fees you pay AND caring about the sats per vbyte you pay that I'm not understanding?
I think it makes sense to have both. I could have a `maxtxfee` of the upper bound of what I'm willing to pay ever, and also set `maxfeerate` to something reasonable just to make sure I don't typo a feerate and accidentally pay a feerate orders of magnitude higher than intended. At `maxfeerate`, a small transaction can still pay a smaller fee than `maxtxfee`,
...
💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1915302735)
> 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.
If bitcoin core is not generating this key, then it seems reasonable to have the pubkey along with it so that it can be verified. If bitcoin core *is* generating this key, I don't see why we are writing it to disk unencrypted. Seems like a bad idea. Also it seems like these keys can be ephemeral if we can just make a new one, so writing it to d
...
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1915302735)
> 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.
If bitcoin core is not generating this key, then it seems reasonable to have the pubkey along with it so that it can be verified. If bitcoin core *is* generating this key, I don't see why we are writing it to disk unencrypted. Seems like a bad idea. Also it seems like these keys can be ephemeral if we can just make a new one, so writing it to d
...
💬 mzumsande commented on pull request "redeclare nChainTx to use uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29331#issuecomment-1915311442)
> It should be possible to fake a large `nChainTx` in a test using assume utxo
It's annoying that each test doing something like that would need to add an entry to the regtest chainparams, maybe my suggestion [here](https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1907059722) to allow that per RPC could make sense (not necessarily in this PR).
(https://github.com/bitcoin/bitcoin/pull/29331#issuecomment-1915311442)
> It should be possible to fake a large `nChainTx` in a test using assume utxo
It's annoying that each test doing something like that would need to add an entry to the regtest chainparams, maybe my suggestion [here](https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1907059722) to allow that per RPC could make sense (not necessarily in this PR).
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470021935)
> do we need a separate `_send_lock` for every P2PConnection? benchmarks look slightly faster with a global `_send_lock` (like `p2p_lock`).
Umm, that sounds a bit strange to me. My intuition would be that having a global lock should be slower, especially if multiple connections are run at the same time, given they would be racing to acquire that lock every time a message needs to be sent (?)
w.r.t wether or not a deadlock is possible, I think @mzumsande may be able to tell you better given
...
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470021935)
> do we need a separate `_send_lock` for every P2PConnection? benchmarks look slightly faster with a global `_send_lock` (like `p2p_lock`).
Umm, that sounds a bit strange to me. My intuition would be that having a global lock should be slower, especially if multiple connections are run at the same time, given they would be racing to acquire that lock every time a message needs to be sent (?)
w.r.t wether or not a deadlock is possible, I think @mzumsande may be able to tell you better given
...