💬 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`
🤔 mzumsande reviewed a pull request: "fuzz: Test headers pre-sync through p2p interface"
(https://github.com/bitcoin/bitcoin/pull/28043#pullrequestreview-1846077000)
just fyi, I started reviewing at some point, my problem was that I don't like test-specific / mocking code in production code (especially the consensus-critical parts), so I wasn't really comfortable (concept)-acking it.
On the other hand, I didn't see a better way to do it and fuzzing is great obviously...
As a result, I was undecided and didn't write anything. Maybe it'd be worth to have a general discussion about the extent of mocking / test-specific production code we want as a project,
...
(https://github.com/bitcoin/bitcoin/pull/28043#pullrequestreview-1846077000)
just fyi, I started reviewing at some point, my problem was that I don't like test-specific / mocking code in production code (especially the consensus-critical parts), so I wasn't really comfortable (concept)-acking it.
On the other hand, I didn't see a better way to do it and fuzzing is great obviously...
As a result, I was undecided and didn't write anything. Maybe it'd be worth to have a general discussion about the extent of mocking / test-specific production code we want as a project,
...
👍 dergoegge approved a pull request: "fuzz: Print coverage summary after run_once"
(https://github.com/bitcoin/bitcoin/pull/29329#pullrequestreview-1846088199)
tACK fa74d2d54f3a102221fb24790c0dfb5c60758535
(https://github.com/bitcoin/bitcoin/pull/29329#pullrequestreview-1846088199)
tACK fa74d2d54f3a102221fb24790c0dfb5c60758535
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1912341078)
For your consideration, I have pushed a variant with some recommended changes here: https://github.com/luke-jr/bitcoin/commits/rpccookieperms-26%2Bknots/
Feel free to cherry-pick or squash into your PR as you deem best.
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1912341078)
For your consideration, I have pushed a variant with some recommended changes here: https://github.com/luke-jr/bitcoin/commits/rpccookieperms-26%2Bknots/
Feel free to cherry-pick or squash into your PR as you deem best.
💬 luke-jr commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1912353847)
Concept NACK to restricting it to only manual connections. This will cause random breakage when automatic peering happens to connect to a manual peer first.
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1912353847)
Concept NACK to restricting it to only manual connections. This will cause random breakage when automatic peering happens to connect to a manual peer first.
🚀 fanquake merged a pull request: "fuzz: also set MSAN_SYMBOLIZER_PATH"
(https://github.com/bitcoin/bitcoin/pull/29327)
(https://github.com/bitcoin/bitcoin/pull/29327)
💬 mzumsande commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1912368151)
> Concept NACK to restricting it to only manual connections. This will cause random breakage when automatic peering happens to connect to a manual peer first.
did you see #28895? We don't make automatic connections to manually specified peers anymore.
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1912368151)
> Concept NACK to restricting it to only manual connections. This will cause random breakage when automatic peering happens to connect to a manual peer first.
did you see #28895? We don't make automatic connections to manually specified peers anymore.