👍 shaavan approved a pull request: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1845820349)
Code Review ACK 8c067ef1c67a1053127a10b6312bcf71da446445
Notes:
- The Unserialise function first reads the compression flag from the stream and then updates the actual keydata.
- The Serialise function first writes the compression flag, followed by the keydata
- The test appropriately verifies the added code for both the compressed and uncompressed keys.
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1845820349)
Code Review ACK 8c067ef1c67a1053127a10b6312bcf71da446445
Notes:
- The Unserialise function first reads the compression flag from the stream and then updates the actual keydata.
- The Serialise function first writes the compression flag, followed by the keydata
- The test appropriately verifies the added code for both the compressed and uncompressed keys.
🤔 maflcko reviewed a pull request: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1845873682)
How is this different from `CPrivKey`?
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1845873682)
How is this different from `CPrivKey`?
💬 maflcko commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1467728211)
nit: In C++, a `for` loop can be used to execute the same code block with different values.
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1467728211)
nit: In C++, a `for` loop can be used to execute the same code block with different values.
💬 maflcko commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1467729703)
Not sure. IIUC the assumption is that keydata should not hold invalid keys. Can you explain why this assumption should be broken?
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1467729703)
Not sure. IIUC the assumption is that keydata should not hold invalid keys. Can you explain why this assumption should be broken?
💬 theStack commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1467741563)
Have to admit that I'm lacking knowledge here about the concrete differences between `FillableSigningProvider` and `FlatSigningProvider`, especially about the possible benefits of the latter in this PR. AFAIR, I chose the fillable provider as it provides a nice interface (`.AddKey()`), which the flat one doesn't. The following patch works:
```diff
diff --git a/src/bench/sign_transaction.cpp b/src/bench/sign_transaction.cpp
index 8cb5e63882..8bd2ecbcd0 100644
--- a/src/bench/sign_transaction.
...
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1467741563)
Have to admit that I'm lacking knowledge here about the concrete differences between `FillableSigningProvider` and `FlatSigningProvider`, especially about the possible benefits of the latter in this PR. AFAIR, I chose the fillable provider as it provides a nice interface (`.AddKey()`), which the flat one doesn't. The following patch works:
```diff
diff --git a/src/bench/sign_transaction.cpp b/src/bench/sign_transaction.cpp
index 8cb5e63882..8bd2ecbcd0 100644
--- a/src/bench/sign_transaction.
...
🤔 sdaftuar reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1845786642)
I haven't yet reviewed the tests -- still plan to do that.
FYI, despite my earlier comment suggesting we resurrect #27018, after giving this further thought I realized we don't need to worry about the issue of 0-fee transactions in the mempool at all until we get to the point that we're doing package relay/validation from the p2p network, and potentially even then we may not need such behavior (as there's just a free-relay concern that is easy to mitigate). So in the interests of narrowing t
...
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1845786642)
I haven't yet reviewed the tests -- still plan to do that.
FYI, despite my earlier comment suggesting we resurrect #27018, after giving this further thought I realized we don't need to worry about the issue of 0-fee transactions in the mempool at all until we get to the point that we're doing package relay/validation from the p2p network, and potentially even then we may not need such behavior (as there's just a free-relay concern that is easy to mitigate). So in the interests of narrowing t
...
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467745903)
Yes I think this can be scrapped now -- `ApplyV3Rules` only operates on a single transaction and its in-mempool parents; correct validation for packages should now all happen in `PackageV3Checks`.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467745903)
Yes I think this can be scrapped now -- `ApplyV3Rules` only operates on a single transaction and its in-mempool parents; correct validation for packages should now all happen in `PackageV3Checks`.
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467692141)
My view is that we'll end up reworking things substantially in order to enable package RBF, but I agree with the code comment suggestion.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467692141)
My view is that we'll end up reworking things substantially in order to enable package RBF, but I agree with the code comment suggestion.
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467676423)
Agreed, that makes sense. Something like
```
bool has_mempool_sibling{false};
...
has_mempool_sibling = (parent->GetCountWithDescendants() > 1);
```
?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467676423)
Agreed, that makes sense. Something like
```
bool has_mempool_sibling{false};
...
has_mempool_sibling = (parent->GetCountWithDescendants() > 1);
```
?
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467674203)
For consistency with the rest of the code relating to the v3 rules, perhaps we should use the V3_ANCESTOR_LIMIT variable here, and use the `static_assert` like in `ApplyV3Rules` to indicate to code readers that this only works because the limit is 1?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467674203)
For consistency with the rest of the code relating to the v3 rules, perhaps we should use the V3_ANCESTOR_LIMIT variable here, and use the `static_assert` like in `ApplyV3Rules` to indicate to code readers that this only works because the limit is 1?
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467759890)
@sdaftuar do you mean for https://github.com/bitcoin/bitcoin/pull/28984 or for generalized package RBF? I'm thinking shorter term here.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467759890)
@sdaftuar do you mean for https://github.com/bitcoin/bitcoin/pull/28984 or for generalized package RBF? I'm thinking shorter term here.
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467762633)
I fuzzed the PR with `ApplyV3Rules` turned off for `AcceptMultipleTransactions` cases and didn't hit any issues, so I believe sufficient validation is contained within `PackageV3Checks` :partying_face:
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467762633)
I fuzzed the PR with `ApplyV3Rules` turned off for `AcceptMultipleTransactions` cases and didn't hit any issues, so I believe sufficient validation is contained within `PackageV3Checks` :partying_face:
💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1912215341)
@Sjors I've made some on #28122 to reflect the changes in the BIP + a few naming cleanups on the functions. Should be good to go for a rebase here, if you're still working on this.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1912215341)
@Sjors I've made some on #28122 to reflect the changes in the BIP + a few naming cleanups on the functions. Should be good to go for a rebase here, if you're still working on this.
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467780296)
Oh right, yes I was thinking longer term.. never mind!
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467780296)
Oh right, yes I was thinking longer term.. never mind!
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1912245634)
> @luke-jr
>
> > I'm not aware of any legitimate usage of bare multisig, so this should be a costless way to filter more spam. Knots has had this policy for years.
>
> OpenTimestamps has previously used bare multisig for timestamp transactions, as it's a bit more efficient than OP_Return. The current OpenTimestamps Server does not, as when I wrote it I was expecting bare multisig to be made non-standard. But as that didn't happen, I should look into adding it again.
>
> Concept NACK wit
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1912245634)
> @luke-jr
>
> > I'm not aware of any legitimate usage of bare multisig, so this should be a costless way to filter more spam. Knots has had this policy for years.
>
> OpenTimestamps has previously used bare multisig for timestamp transactions, as it's a bit more efficient than OP_Return. The current OpenTimestamps Server does not, as when I wrote it I was expecting bare multisig to be made non-standard. But as that didn't happen, I should look into adding it again.
>
> Concept NACK wit
...
💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912246898)
> @GregTonoski: So what are you using to do fee estimation and assessing what fee you need to pay to get your transaction into an upcoming block? (...)
I'm using the data from a few recent blocks to find out median of fees of transactions that were included. I also consider broadcasting a duplicated transaction with higher fee if the first one wouldn't get in a block for too long.
Besides, nobody uses fee estimation by Bitcoin Core in my circles.
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912246898)
> @GregTonoski: So what are you using to do fee estimation and assessing what fee you need to pay to get your transaction into an upcoming block? (...)
I'm using the data from a few recent blocks to find out median of fees of transactions that were included. I also consider broadcasting a duplicated transaction with higher fee if the first one wouldn't get in a block for too long.
Besides, nobody uses fee estimation by Bitcoin Core in my circles.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467798131)
I don't think this is ever relevant in v3, even with future changes.
If your mempool parent has another mempool child, it doesn't matter if you have {RBF, sibling eviction, package RBF} within a package. Enumerating the possibilities in 1p1c:
```suggestion
// The mempool or in-package parent cannot have any other in-mempool children. Since this is
// a package, we don't need to check whether this can be a to-be-replaced sibling.
// In a package, there m
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467798131)
I don't think this is ever relevant in v3, even with future changes.
If your mempool parent has another mempool child, it doesn't matter if you have {RBF, sibling eviction, package RBF} within a package. Enumerating the possibilities in 1p1c:
```suggestion
// The mempool or in-package parent cannot have any other in-mempool children. Since this is
// a package, we don't need to check whether this can be a to-be-replaced sibling.
// In a package, there m
...
⚠️ maflcko opened an issue: "assumeutxo: nTx and nChainTx in RPC results are off"
(https://github.com/bitcoin/bitcoin/issues/29328)
AssumeUtxo mocks the `nTx` value of headers after the last background block and before the snapshot header with `nTx=1`. This means that the `getblockheader` and `getchaintxstats` RPCs return the wrong `nTx` or `txcount` fields for those blocks.
Not sure if anything should be done about this?
(https://github.com/bitcoin/bitcoin/issues/29328)
AssumeUtxo mocks the `nTx` value of headers after the last background block and before the snapshot header with `nTx=1`. This means that the `getblockheader` and `getchaintxstats` RPCs return the wrong `nTx` or `txcount` fields for those blocks.
Not sure if anything should be done about this?
💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1467815704)
But if there's only one name here, it's unclear what name the bool type is for? Or we're assuming it can only be used as a positional param at that point?
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1467815704)
But if there's only one name here, it's unclear what name the bool type is for? Or we're assuming it can only be used as a positional param at that point?
💬 michaelfolkson commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912282572)
> I use the data from a few recent blocks to find out median of fees of transactions that were included. I also consider broadcasting a duplicated transaction with higher fee if the first one doesn't get in a block for too long.
>
> Besides, nobody uses fee estimation by Bitcoin Core in my circles.
Ok fair enough. That is a criticism of the Bitcoin Core wallet's fee estimation. But the "data from a few recent blocks" you are using for fee estimation includes transactions that have been subm
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912282572)
> I use the data from a few recent blocks to find out median of fees of transactions that were included. I also consider broadcasting a duplicated transaction with higher fee if the first one doesn't get in a block for too long.
>
> Besides, nobody uses fee estimation by Bitcoin Core in my circles.
Ok fair enough. That is a criticism of the Bitcoin Core wallet's fee estimation. But the "data from a few recent blocks" you are using for fee estimation includes transactions that have been subm
...