💬 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)
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2074860730)
Doesn't the presence of the original cache-usage-update indicate that the comment isn't enough?
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2074860730)
Doesn't the presence of the original cache-usage-update indicate that the comment isn't enough?
💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074890106)
Same for `NotifyWatchonlyChanged`: It doesn't look to be called anywhere, so can be removed.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074890106)
Same for `NotifyWatchonlyChanged`: It doesn't look to be called anywhere, so can be removed.
👍 maflcko approved a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2817225709)
re-review ACK 1c0d89358e 🎽
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK 1c0d89358e 🎽
PZKgfTbviYeE2sXUccefH
...
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2817225709)
re-review ACK 1c0d89358e 🎽
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK 1c0d89358e 🎽
PZKgfTbviYeE2sXUccefH
...
💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074911165)
nit in b6bb744a881d465d15b40413a2753ca4865e851a: Seems fine to fully remove this, given that it isn't needed anyway after commit 1111aecbb58d6e37d430d477ac43f52811fd97d9
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074911165)
nit in b6bb744a881d465d15b40413a2753ca4865e851a: Seems fine to fully remove this, given that it isn't needed anyway after commit 1111aecbb58d6e37d430d477ac43f52811fd97d9
💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074935902)
nit in a29101b56541d4bf72fcf69460e8eb2a8cc33979: I guess you wanted to leave some of those for follow-ups, but the `doc/descriptors.md` one seems in-scope?
```
$ git grep -i 'legacy wallet' 1c0d89358e12fc871e99c8304d5cb50965cf7b9d doc/descriptors.md doc/managing-wallets.md src/bitcoin-wallet.cpp src/wallet src/script/ test/
1c0d89358e12fc871e99c8304d5cb50965cf7b9d:doc/descriptors.md:- `importmulti` takes as input descriptors to import into a legacy wallet
1c0d89358e12fc871e99c8304d5cb509
...
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2074935902)
nit in a29101b56541d4bf72fcf69460e8eb2a8cc33979: I guess you wanted to leave some of those for follow-ups, but the `doc/descriptors.md` one seems in-scope?
```
$ git grep -i 'legacy wallet' 1c0d89358e12fc871e99c8304d5cb50965cf7b9d doc/descriptors.md doc/managing-wallets.md src/bitcoin-wallet.cpp src/wallet src/script/ test/
1c0d89358e12fc871e99c8304d5cb50965cf7b9d:doc/descriptors.md:- `importmulti` takes as input descriptors to import into a legacy wallet
1c0d89358e12fc871e99c8304d5cb509
...