🤔 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
...
💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912291528)
Let's not divert from the subject. Fee estimation techniques and tradeoffs are beside the point.
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912291528)
Let's not divert from the subject. Fee estimation techniques and tradeoffs are beside the point.
💬 michaelfolkson commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912300846)
> Let's not divert from the subject. Fee estimation techniques and tradeoffs are beside the point.
Just pointing out your inconsistency @GregTonoski. If you don't want people to have some of these high fee rate transactions in their mempools so they can do better fee estimation you should filter out these transactions from mined blocks in your fee estimation data. You want them to keep their heads in the sand that some of these transactions will be mined and hence you should keep your head in
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1912300846)
> Let's not divert from the subject. Fee estimation techniques and tradeoffs are beside the point.
Just pointing out your inconsistency @GregTonoski. If you don't want people to have some of these high fee rate transactions in their mempools so they can do better fee estimation you should filter out these transactions from mined blocks in your fee estimation data. You want them to keep their heads in the sand that some of these transactions will be mined and hence you should keep your head in
...
📝 maflcko opened a pull request: "fuzz: Print coverage summary after run_once"
(https://github.com/bitcoin/bitcoin/pull/29329)
This can be used to quickly check the coverage effects of a code change or qa-assets change.
(https://github.com/bitcoin/bitcoin/pull/29329)
This can be used to quickly check the coverage effects of a code change or qa-assets change.
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467853130)
not sure I like this suggested text. anyone else want to give it a shot?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1467853130)
not sure I like this suggested text. anyone else want to give it a shot?
💬 maflcko commented on pull request "fuzz: Print coverage summary after run_once":
(https://github.com/bitcoin/bitcoin/pull/29329#issuecomment-1912328327)
Can be quickly tested with: `$ ./test/fuzz/test_runner.py ..folder.. addition_overflow multiplication_overflow`
(https://github.com/bitcoin/bitcoin/pull/29329#issuecomment-1912328327)
Can be quickly tested with: `$ ./test/fuzz/test_runner.py ..folder.. addition_overflow multiplication_overflow`