💬 brunoerg commented on pull request "fuzz: coinselection: cover `SetBumpFeeDiscount`":
(https://github.com/bitcoin/bitcoin/pull/31806#issuecomment-2743340366)
Rebased
(https://github.com/bitcoin/bitcoin/pull/31806#issuecomment-2743340366)
Rebased
💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2007568411)
thx, maybe in a follow-up. With two acks, this seems close to merge
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2007568411)
thx, maybe in a follow-up. With two acks, this seems close to merge
💬 janb84 commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2007572387)
> thx, maybe in a follow-up. With two acks, this seems close to merge
Agreed ! Lets not invalidate the ACKS over this.
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2007572387)
> thx, maybe in a follow-up. With two acks, this seems close to merge
Agreed ! Lets not invalidate the ACKS over this.
💬 darosior commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743368308)
> The complexity of doing this is likely the reason that no one has added any Tapscript tests to JSON script tests until this PR.
I think the reason no one has added more tests is because the Tapscript logic is already extensively covered through the JSON script unit tests (in additional to the extensive functional test). See this unit test (which works with [this JSON file](https://raw.githubusercontent.com/bitcoin-core/qa-assets/refs/heads/main/unit_test_data/script_assets_test.json) from t
...
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743368308)
> The complexity of doing this is likely the reason that no one has added any Tapscript tests to JSON script tests until this PR.
I think the reason no one has added more tests is because the Tapscript logic is already extensively covered through the JSON script unit tests (in additional to the extensive functional test). See this unit test (which works with [this JSON file](https://raw.githubusercontent.com/bitcoin-core/qa-assets/refs/heads/main/unit_test_data/script_assets_test.json) from t
...
💬 flack commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2007610573)
nit: challange => challenge
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2007610573)
nit: challange => challenge
🤔 furszy reviewed a pull request: "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2705977157)
Code reviewed, thanks for changing the approach. Looking good with a few nuances.
There are two more places where we call `AddWalletDescriptor` that haven't been considered:
1) `AddKey()` in `wallet_tests.cpp`.
2) `CreateSyncedWallet()` in `wallet/test/util.cpp`
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2705977157)
Code reviewed, thanks for changing the approach. Looking good with a few nuances.
There are two more places where we call `AddWalletDescriptor` that haven't been considered:
1) `AddKey()` in `wallet_tests.cpp`.
2) `CreateSyncedWallet()` in `wallet/test/util.cpp`
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007600350)
nano nit: maybe call it `spkm_res` to clarify that this is not the spkm anymore.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007600350)
nano nit: maybe call it `spkm_res` to clarify that this is not the spkm anymore.
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007597399)
not for this PR but there is no need to call `GetDescriptorScriptPubKeyMan()` here, can just return `AddWalletDescriptor()` output.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007597399)
not for this PR but there is no need to call `GetDescriptorScriptPubKeyMan()` here, can just return `AddWalletDescriptor()` output.
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007604057)
We are no longer asserting that `spk_manager!=nullptr`.
Could unify these lines + the missing check:
```c++
auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal));
assert(spk_manager);
```
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007604057)
We are no longer asserting that `spk_manager!=nullptr`.
Could unify these lines + the missing check:
```c++
auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal));
assert(spk_manager);
```
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007619742)
Same as above; would be good to not mix coding styles. No jump line after the condition statement.
And the `res` variable is shadowing another variable in this function. Would be better to rename it to `res_spkm` or `spkm_res`.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007619742)
Same as above; would be good to not mix coding styles. No jump line after the condition statement.
And the `res` variable is shadowing another variable in this function. Would be better to rename it to `res_spkm` or `spkm_res`.
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007630950)
The `res` variable is already used in this function, by declaring it here again you are shadowing the other one. Would be better to rename it to `res_spkm` or `spkm_res`.
Also, missing whitespace after `if`.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007630950)
The `res` variable is already used in this function, by declaring it here again you are shadowing the other one. Would be better to rename it to `res_spkm` or `spkm_res`.
Also, missing whitespace after `if`.
💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007611055)
Would be good to follow the same code style we have in this function. No jump line after the condition statement.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007611055)
Would be good to follow the same code style we have in this function. No jump line after the condition statement.
💬 sipa commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743492906)
I do understand the desire to have accessible simple unit test cases for tapscript, and more testing certainly doesn't hurt. However, making things like this possible and easy to write was the goal of the taproot test framework, though), which has advantages like supporting combinations with non-trivial script trees, adding auto-generated signatures, integrating it all into transactions and blocks and running through the entire stack, plus having the ability to export the resulting cases to JSON
...
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743492906)
I do understand the desire to have accessible simple unit test cases for tapscript, and more testing certainly doesn't hurt. However, making things like this possible and easy to write was the goal of the taproot test framework, though), which has advantages like supporting combinations with non-trivial script trees, adding auto-generated signatures, integrating it all into transactions and blocks and running through the entire stack, plus having the ability to export the resulting cases to JSON
...
💬 BrandonOdiwuor commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2007717777)
fixed
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2007717777)
fixed
💬 virtu commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2743564244)
ACK 4c8e9b4f35be4104002ece15b6f72c360756f5d9
Tested on Linux and MacOS.
Nice work. I'm looking to integrating some UTXO sats on my website, so it's nice to see people working on conveniently extracting this data.
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2743564244)
ACK 4c8e9b4f35be4104002ece15b6f72c360756f5d9
Tested on Linux and MacOS.
Nice work. I'm looking to integrating some UTXO sats on my website, so it's nice to see people working on conveniently extracting this data.
💬 virtu commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2743579116)
Great work!
Curious about policy on tooling since I'm currently working on UTXO analysis: it would be nice to refactor the (1) reading of the compressed UTXO format and (2) storing it in SQL format into separate components, so there's a reference for reading dumped UTXO sets people can readily use (e.g. `import CompressedUTXOReader from contrib/utxo-tools`). Or should code like that not be a part of Core?
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2743579116)
Great work!
Curious about policy on tooling since I'm currently working on UTXO analysis: it would be nice to refactor the (1) reading of the compressed UTXO format and (2) storing it in SQL format into separate components, so there's a reference for reading dumped UTXO sets people can readily use (e.g. `import CompressedUTXOReader from contrib/utxo-tools`). Or should code like that not be a part of Core?
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2743639950)
Force-pushed to address @furszy's feedback
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2743639950)
Force-pushed to address @furszy's feedback
✅ maflcko closed an issue: "Check if the wallet already contains the descriptor GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/issues/32013)
(https://github.com/bitcoin/bitcoin/issues/32013)
💬 maflcko commented on issue "Check if the wallet already contains the descriptor GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/issues/32013#issuecomment-2743685449)
Let's move the discussion to the pull request
(https://github.com/bitcoin/bitcoin/issues/32013#issuecomment-2743685449)
Let's move the discussion to the pull request
⚠️ dergoegge opened an issue: "wallet: migratewallet crashes "Assertion `m_wallet_flags == 0' failed""
(https://github.com/bitcoin/bitcoin/issues/32111)
`migratewallet` crashes on malformed `wallet.dat`s which is unlikely to happen with wallets created by old Bitcoin Core versions but we should probably return an error to the user instead of crashing.
<details>
<summary>Base64 encoded `wallet.dat` that causes a crash</summary>
```
AAAAAAEAAAAAAAAAYjEFAAkAAAAAEAAAAAkAAAAAAAADAAAAAAAAAAAAAAAAAAAAIAAAAHsA+AEBAwEAQ0PCvWvWAgAAAAAAAAAAAAIAAAAAAAAAIAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
...
(https://github.com/bitcoin/bitcoin/issues/32111)
`migratewallet` crashes on malformed `wallet.dat`s which is unlikely to happen with wallets created by old Bitcoin Core versions but we should probably return an error to the user instead of crashing.
<details>
<summary>Base64 encoded `wallet.dat` that causes a crash</summary>
```
AAAAAAEAAAAAAAAAYjEFAAkAAAAAEAAAAAkAAAAAAAADAAAAAAAAAAAAAAAAAAAAIAAAAHsA+AEBAwEAQ0PCvWvWAgAAAAAAAAAAAAIAAAAAAAAAIAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
...