💬 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
...
💬 1440000bytes commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915317702)
> We are facing a problem in BTCPay: A large number of users have only 20GB of space to store the UTXO Set. Now that the UTXO Set is reaching 10GB and increasing, those users might have their node crashing sooner or later. So we are very interested into solutions to drop down the UTXO set size.
This pull request won't make any difference. Top 5 output types in UTXO set are P2PKH, P2WPKH, P2TR, P2SH and P2WSH.
https://txstats.com/d/000000054/utxo-set-repartition-by-output-type?orgId=1
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915317702)
> We are facing a problem in BTCPay: A large number of users have only 20GB of space to store the UTXO Set. Now that the UTXO Set is reaching 10GB and increasing, those users might have their node crashing sooner or later. So we are very interested into solutions to drop down the UTXO set size.
This pull request won't make any difference. Top 5 output types in UTXO set are P2PKH, P2WPKH, P2TR, P2SH and P2WSH.
https://txstats.com/d/000000054/utxo-set-repartition-by-output-type?orgId=1
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915336928)
> 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`, but a large transaction could exceed `maxtxfee`.
But in this example, if you typo'd a feerate orders of magnitude higher, your `maxtxf
...
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915336928)
> 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`, but a large transaction could exceed `maxtxfee`.
But in this example, if you typo'd a feerate orders of magnitude higher, your `maxtxf
...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470044262)
Hm this is a bit of a mess, you're right. The behavior in this PR makes sense in response to @ryanofsky comment: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446841611
The help text has also been modified in `src/rpc/util.cpp` (98031401c744ea3c2b62927fabf27c22b639c3cf) to always recommend `"jsonrpc":"2.0"`
I think you are illustrating an important issue though -- anyone who has used `bitcoin-cli help` (for ANY RPC command!) would have seen instructions to include `"jsonrpc":
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470044262)
Hm this is a bit of a mess, you're right. The behavior in this PR makes sense in response to @ryanofsky comment: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1446841611
The help text has also been modified in `src/rpc/util.cpp` (98031401c744ea3c2b62927fabf27c22b639c3cf) to always recommend `"jsonrpc":"2.0"`
I think you are illustrating an important issue though -- anyone who has used `bitcoin-cli help` (for ANY RPC command!) would have seen instructions to include `"jsonrpc":
...
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1470045451)
I believe you're correct that this code is not reachable in a functional test, but the reason is that when PV3C(TxB) is invoked, we'll notice that it is a v3 child and that it has an in-package descendant, and fail further down (https://github.com/bitcoin/bitcoin/pull/28948/commits/965a23f33404b5e3cac41b7c4e17298e96ff25bf#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R118-R120).
There is a test for this topology in the functional tests, see `mempool_accept_v3.py` here:
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1470045451)
I believe you're correct that this code is not reachable in a functional test, but the reason is that when PV3C(TxB) is invoked, we'll notice that it is a v3 child and that it has an in-package descendant, and fail further down (https://github.com/bitcoin/bitcoin/pull/28948/commits/965a23f33404b5e3cac41b7c4e17298e96ff25bf#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R118-R120).
There is a test for this topology in the functional tests, see `mempool_accept_v3.py` here:
...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470059890)
This is a good catch but unfortunately I think we need to stick with this behavior for backwards compatibility. If we get a request that does not have the exact key-value pair `"jsonrpc":"2.0"` we have to resort to legacy behavior. In that case, the user has submitted an empty batch and what they get back is an empty vector.
The closest 2.0 example I can think of is this:
```
--> [{"jsonrpc":"2.0", "id":""}]
<-- [{'jsonrpc': '2.0', 'error': {'code': -32600, 'message': 'Missing method'},
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1470059890)
This is a good catch but unfortunately I think we need to stick with this behavior for backwards compatibility. If we get a request that does not have the exact key-value pair `"jsonrpc":"2.0"` we have to resort to legacy behavior. In that case, the user has submitted an empty batch and what they get back is an empty vector.
The closest 2.0 example I can think of is this:
```
--> [{"jsonrpc":"2.0", "id":""}]
<-- [{'jsonrpc': '2.0', 'error': {'code': -32600, 'message': 'Missing method'},
...