💬 Tech1k commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2852636062)
NACK. While I agree that this change can be beneficial by allowing data-heavy applications consolidate what would have been dust outputs into a single OP_RETURN script, I don’t support removing the arbitrary limit entirely. Instead, I’d favor towards increasing the limit or enabling OP_RETURN chaining, provided appropriate fees are enforced as this change may lead to increased spam.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2852636062)
NACK. While I agree that this change can be beneficial by allowing data-heavy applications consolidate what would have been dust outputs into a single OP_RETURN script, I don’t support removing the arbitrary limit entirely. Instead, I’d favor towards increasing the limit or enabling OP_RETURN chaining, provided appropriate fees are enforced as this change may lead to increased spam.
💬 andrewtoth commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2074362360)
I don't think we should add an assert here to explain. The comment right above says `Create the coin in the parent cache`.
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2074362360)
I don't think we should add an assert here to explain. The comment right above says `Create the coin in the parent cache`.
💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852675818)
> I don't see why the first commit is necessary. Couldn't you just mock the time so that it's fixed and the number of derivation rounds always stays at the default value?
Maybe I'm not thinking creatively enough, but I think because the timing takes place entirely inside of `EncryptWallet()->EncryptMasterKey`, from the `wallet_encrypt.cpp` benchmark, I could only mock the clock so that it's static during the derivation "benchmark"[^1] runs, so the difference between the end time and the start
...
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852675818)
> I don't see why the first commit is necessary. Couldn't you just mock the time so that it's fixed and the number of derivation rounds always stays at the default value?
Maybe I'm not thinking creatively enough, but I think because the timing takes place entirely inside of `EncryptWallet()->EncryptMasterKey`, from the `wallet_encrypt.cpp` benchmark, I could only mock the clock so that it's static during the derivation "benchmark"[^1] runs, so the difference between the end time and the start
...
💬 kevkevinpal commented on pull request "fuzz: Remove unused TimeoutExpired catch in fuzz runner":
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2852734496)
crACK [fa48040](https://github.com/bitcoin/bitcoin/pull/32388/commits/fa4804009ceba96926edd7f55ea22442ebdc665d)
Agreed the error message would be wrong but also not needed anymore
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2852734496)
crACK [fa48040](https://github.com/bitcoin/bitcoin/pull/32388/commits/fa4804009ceba96926edd7f55ea22442ebdc665d)
Agreed the error message would be wrong but also not needed anymore
💬 furszy commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852755475)
> so the difference between the end time and the start time of a run will be 0 which will result in dividing by 0:
Yes, it seems much more straightforward to address that inside the encryption function (which could arguably be considered a 'fix', since we shouldn’t be dividing by zero regardless of whether the clock time is messed up) than to add a test-only field to the wallet and another function arg just just for this.
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852755475)
> so the difference between the end time and the start time of a run will be 0 which will result in dividing by 0:
Yes, it seems much more straightforward to address that inside the encryption function (which could arguably be considered a 'fix', since we shouldn’t be dividing by zero regardless of whether the clock time is messed up) than to add a test-only field to the wallet and another function arg just just for this.
💬 petertodd commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2852767981)
ACK 664ae315f4965ef39d333dc915c034fd6181c8aa
Manually tested that the `-datacarriersize` limit does in fact match on a two OP_Return output transaction of the expected size, and that the resulting transaction was propagated to other nodes as expected.
Of course, I still (weakly) prefer my version. But this is acceptable too.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2852767981)
ACK 664ae315f4965ef39d333dc915c034fd6181c8aa
Manually tested that the `-datacarriersize` limit does in fact match on a two OP_Return output transaction of the expected size, and that the resulting transaction was propagated to other nodes as expected.
Of course, I still (weakly) prefer my version. But this is acceptable too.
💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852807611)
> since we shouldn’t be dividing by zero regardless of whether the clock time is messed up
AFAIK, `std::chrono::steady_clock`, unlike the system clock, cannot fail to move forward, and this will never happen.
> than to add a test-only field to the wallet and another function arg just just for this.
I agree that adding a test-only field and arg is very bad, and would like to avoid it, but it seems to me worse to imbue secret meaning into the benchmark by handling a time difference of 0
...
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852807611)
> since we shouldn’t be dividing by zero regardless of whether the clock time is messed up
AFAIK, `std::chrono::steady_clock`, unlike the system clock, cannot fail to move forward, and this will never happen.
> than to add a test-only field to the wallet and another function arg just just for this.
I agree that adding a test-only field and arg is very bad, and would like to avoid it, but it seems to me worse to imbue secret meaning into the benchmark by handling a time difference of 0
...
🤔 shahsb reviewed a pull request: "miner: don't needlessly append dummy OP_0 to bip34"
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-2816730864)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-2816730864)
Concept ACK
💬 bubelov commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2853205353)
The mailing list discussion vastly overestimates VC. If they succeed, they could raise even more money for further attacks, because it will be seen as a proof of their ability to influence the reference implementation. However, if the general sentiment remains hostile, securing funds for abusive blockchain use will become much harder.
Concept NACK.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2853205353)
The mailing list discussion vastly overestimates VC. If they succeed, they could raise even more money for further attacks, because it will be seen as a proof of their ability to influence the reference implementation. However, if the general sentiment remains hostile, securing funds for abusive blockchain use will become much harder.
Concept NACK.
💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2853240905)
Refactored to address @furszy feedback to avoid adding a test-only parameter to `CWallet`, and added a somewhat opinionated refactor (https://github.com/bitcoin/bitcoin/pull/31774/commits/7176b26cde7cbaffdd92af9c25f85f8e5233e78a) of `CWallet::EncryptMasterKey()` since I was touching the iteration measuring stuff anyways. I don't like the idea of using a `for` loop that iterates once with `0` and once with `1`, but it's nearly 50% the LOC, I believe it is easier to understand how the weighted ave
...
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2853240905)
Refactored to address @furszy feedback to avoid adding a test-only parameter to `CWallet`, and added a somewhat opinionated refactor (https://github.com/bitcoin/bitcoin/pull/31774/commits/7176b26cde7cbaffdd92af9c25f85f8e5233e78a) of `CWallet::EncryptMasterKey()` since I was touching the iteration measuring stuff anyways. I don't like the idea of using a `for` loop that iterates once with `0` and once with `1`, but it's nearly 50% the LOC, I believe it is easier to understand how the weighted ave
...
💬 vasild commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2074738962)
Now I see! Multisig is another way to create "same_txid, different_wtxid" even though it does not fiddle with the `s` value. And so, both transactions are relayable. Cool. Best would be to have a function that produces those two transactions. That would need the private keys. I guess better be separate from `create_malleated_version()`.
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2074738962)
Now I see! Multisig is another way to create "same_txid, different_wtxid" even though it does not fiddle with the `s` value. And so, both transactions are relayable. Cool. Best would be to have a function that produces those two transactions. That would need the private keys. I guess better be separate from `create_malleated_version()`.
🤔 stratospher reviewed a pull request: "scripted-diff: adapt script error constant names in feature_taproot.py"
(https://github.com/bitcoin/bitcoin/pull/32415#pullrequestreview-2816937496)
ACK b5f580c. liked the consistency in script error names.
(https://github.com/bitcoin/bitcoin/pull/32415#pullrequestreview-2816937496)
ACK b5f580c. liked the consistency in script error names.
💬 maflcko commented on pull request "[WIP] rpc: add `clearmempool` command for regtest mode":
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2853377552)
> > What about invalidating the block, recording all mempool txs via `getrawmempool`, stopping the node, deleting `mempool.dat` and restarting?
>
> The wallets will reinsert the transactions unless you prevent them from doing so, but then you don't know what transactions to remove from the wallet. Restarting the node mid-test is also slow and annoying.
I think the idea was to "record"/remember the mempool txids to be abandoned later.
A third alternative could be to just backup the walle
...
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2853377552)
> > What about invalidating the block, recording all mempool txs via `getrawmempool`, stopping the node, deleting `mempool.dat` and restarting?
>
> The wallets will reinsert the transactions unless you prevent them from doing so, but then you don't know what transactions to remove from the wallet. Restarting the node mid-test is also slow and annoying.
I think the idea was to "record"/remember the mempool txids to be abandoned later.
A third alternative could be to just backup the walle
...
💬 maflcko commented on pull request "spam: trick Drahtbot into rendering a scammy link":
(https://github.com/bitcoin/bitcoin/pull/32422#issuecomment-2853433463)
Yeah, I think this probably fixed earlier as a side-effect of commit https://github.com/maflcko/DrahtBot/commit/b1871ab63bf6e2a1f8e8de7fc4c39bd56a52fe9d already. Of course it will never be 100% accurate, but it being able to find typos such as https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2065717959 seems minimally positive to me. Though, I can also drop the feature again, no strong opinion.
(https://github.com/bitcoin/bitcoin/pull/32422#issuecomment-2853433463)
Yeah, I think this probably fixed earlier as a side-effect of commit https://github.com/maflcko/DrahtBot/commit/b1871ab63bf6e2a1f8e8de7fc4c39bd56a52fe9d already. Of course it will never be 100% accurate, but it being able to find typos such as https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2065717959 seems minimally positive to me. Though, I can also drop the feature again, no strong opinion.
💬 aviv57 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2853437459)
Concept ack, I believe in the long run, we will benefit from less damage to the UTXO set by ordinals, inscription, etc.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2853437459)
Concept ack, I believe in the long run, we will benefit from less damage to the UTXO set by ordinals, inscription, etc.
💬 Eunovo commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2853439571)
> Can you share the setup for these benchmarks so I can test them? Thanks!
OS is Ubuntu 24.04.2 LTS
Output from `lspcu`
```
Architecture: x86_64
CPU op-mode(s): 32-bit, 64-bit
Address sizes: 48 bits physical, 48 bits virtual
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: AuthenticAMD
Model name: AMD Ryzen 9 8945HS w/ Radeon 780M Graphics
CPU family:
...
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2853439571)
> Can you share the setup for these benchmarks so I can test them? Thanks!
OS is Ubuntu 24.04.2 LTS
Output from `lspcu`
```
Architecture: x86_64
CPU op-mode(s): 32-bit, 64-bit
Address sizes: 48 bits physical, 48 bits virtual
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: AuthenticAMD
Model name: AMD Ryzen 9 8945HS w/ Radeon 780M Graphics
CPU family:
...
💬 maflcko commented on pull request "descriptors: taproot partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2853470218)
Could turn into draft while CI is red?
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2853470218)
Could turn into draft while CI is red?
💬 laanwj commented on issue "rpc: actually deprecate `rpcuser` & `rpcpass`":
(https://github.com/bitcoin/bitcoin/issues/29240#issuecomment-2853477663)
> One thing with cookie auth, that currently needs hacks, is allowing cookie access for other users. https://github.com/bitcoin/bitcoin/pull/28167
Well this one was merged, we can nuke them now!
No, to be fair, i'm kind of divided on this issue too-i don't think it actually hurts to allow `rpcuser`/`rpcpasswd` to configure one account for easy single-user-single-application setups, yes it's less secure but this is all about local files and APIs, often if hackers get access to your server you'r
...
(https://github.com/bitcoin/bitcoin/issues/29240#issuecomment-2853477663)
> One thing with cookie auth, that currently needs hacks, is allowing cookie access for other users. https://github.com/bitcoin/bitcoin/pull/28167
Well this one was merged, we can nuke them now!
No, to be fair, i'm kind of divided on this issue too-i don't think it actually hurts to allow `rpcuser`/`rpcpasswd` to configure one account for easy single-user-single-application setups, yes it's less secure but this is all about local files and APIs, often if hackers get access to your server you'r
...
💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2853490844)
> Could probably also remove the proxy functions we have in the test framework for those RPC commands. Look [at this line](https://github.com/bitcoin/bitcoin/blob/fc6346dbc8dc3db40aad4079210332b5f8b332ed/test/functional/test_framework/test_node.py#L979) for example (same for importpubkey, importprivkey and addmultisigaddress).
See https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2063194316
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2853490844)
> Could probably also remove the proxy functions we have in the test framework for those RPC commands. Look [at this line](https://github.com/bitcoin/bitcoin/blob/fc6346dbc8dc3db40aad4079210332b5f8b332ed/test/functional/test_framework/test_node.py#L979) for example (same for importpubkey, importprivkey and addmultisigaddress).
See https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2063194316
💬 maflcko commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2853501498)
For reference, BDB removal https://github.com/bitcoin/bitcoin/pull/31250 was merged (for 30.0)
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2853501498)
For reference, BDB removal https://github.com/bitcoin/bitcoin/pull/31250 was merged (for 30.0)