💬 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
...
💬 maflcko commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2743692730)
> after this fix improved performance of importdescriptor part refs #32013.
If this is a performance improvement, please explain exact steps to reproduce the speedup and how much it is.
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2743692730)
> after this fix improved performance of importdescriptor part refs #32013.
If this is a performance improvement, please explain exact steps to reproduce the speedup and how much it is.
⚠️ dergoegge opened an issue: "wallet: migratewallet crashes "Assertion `legacy_spkm' failed""
(https://github.com/bitcoin/bitcoin/issues/32112)
`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/32112)
`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
...
💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2743737637)
> The long_term_fee_rate and fee_rate are the same for all UTXOs in a single coin selection attempt
Agree. I think that's a little confusing the way the C++ algorithm is structured and treats fee_rate and long_term_fee_rate per UTXO instead of just passing in the fee_rate params as a per sort argument.
> so if two UTXOs have the same fee, their inputs have the same weight
Right, since fee is derived from fee_rate and weight, and fee_rate is guaranteed to be the same, the only true variable i
...
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2743737637)
> The long_term_fee_rate and fee_rate are the same for all UTXOs in a single coin selection attempt
Agree. I think that's a little confusing the way the C++ algorithm is structured and treats fee_rate and long_term_fee_rate per UTXO instead of just passing in the fee_rate params as a per sort argument.
> so if two UTXOs have the same fee, their inputs have the same weight
Right, since fee is derived from fee_rate and weight, and fee_rate is guaranteed to be the same, the only true variable i
...
📝 ajtowns opened a pull request: "fuzz: enable running fuzz test cases in Debug mode"
(https://github.com/bitcoin/bitcoin/pull/32113)
When building with
BUILD_FOR_FUZZING OFF
BUILD_FUZZ_BINARY ON
CMAKE_BUILD_TYPE Debug
allow the fuzz binary to execute given test cases (without actual fuzzing) to make it easier to reproduce fuzz test failures in a more normal debug environment.
(https://github.com/bitcoin/bitcoin/pull/32113)
When building with
BUILD_FOR_FUZZING OFF
BUILD_FUZZ_BINARY ON
CMAKE_BUILD_TYPE Debug
allow the fuzz binary to execute given test cases (without actual fuzzing) to make it easier to reproduce fuzz test failures in a more normal debug environment.
💬 ajtowns commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2743752847)
Related historical PRs: #31191 #24472
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2743752847)
Related historical PRs: #31191 #24472
💬 maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2743792938)
I don't think the current patch will work. `G_FUZZING` influences more than just the behavior of the asserts/assumes. For example:
* POW checks are different
* The task runner is different
* The random seeding is different
So with this patch it looks like you are instead opting into non-reproducible fuzz behavior.
In debug mode the performance doesn't matter, so a possible alternative fix could be to just make G_FUZZING a runtime bool again?
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2743792938)
I don't think the current patch will work. `G_FUZZING` influences more than just the behavior of the asserts/assumes. For example:
* POW checks are different
* The task runner is different
* The random seeding is different
So with this patch it looks like you are instead opting into non-reproducible fuzz behavior.
In debug mode the performance doesn't matter, so a possible alternative fix could be to just make G_FUZZING a runtime bool again?
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2743809558)
ReACK 1601906941fa559ebbee7898453fa77f4606ad38
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2743809558)
ReACK 1601906941fa559ebbee7898453fa77f4606ad38
📝 instagibbs opened a pull request: "test: Add encodable PUSHDATA1 examples to feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32114)
Inspired by discussion in https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743492906 I made an example adding coverage I think is missing, with some extra commentary that might help future contributors (including myself when I forget how it works again).
Open for suggestions how we can make it more welcoming beyond this.
cc @darosior @EthanHeilman @sipa
(https://github.com/bitcoin/bitcoin/pull/32114)
Inspired by discussion in https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743492906 I made an example adding coverage I think is missing, with some extra commentary that might help future contributors (including myself when I forget how it works again).
Open for suggestions how we can make it more welcoming beyond this.
cc @darosior @EthanHeilman @sipa
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743854492)
@sipa
This PR doesn't contain any improvements to the python functional tests because they are excellent and did not require modifications to write clear tapscript tests. This PR is not intended to replace more complex tests. It is only intended to make simple unit tests easy to write and read.
In defense of simple unittests for tapscript:
1. **JSON script_test is the lowest effort way to add a test** Adding a JSON script_test for everything but tapscript is trivially easy. Given huma
...
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743854492)
@sipa
This PR doesn't contain any improvements to the python functional tests because they are excellent and did not require modifications to write clear tapscript tests. This PR is not intended to replace more complex tests. It is only intended to make simple unit tests easy to write and read.
In defense of simple unittests for tapscript:
1. **JSON script_test is the lowest effort way to add a test** Adding a JSON script_test for everything but tapscript is trivially easy. Given huma
...
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743856578)
@darosior
The static test cases are valuable, but they are multistep process to create and are hard to read. Imagine you write some code and get a test failure.
Would you rather the test failure be:
```JSON
{"tx": "f705d6e8019870958e85d1d8f94aa6d74746ba974db0f5ccae49a49b32dcada4e19de4eb5ecb00000000925977cc01f9875c000000000016001431d2b00cd4687ceb34008d9894de84062def14aa05406346", "prevouts": ["b4eae1010000000022512039f7e9232896f8100485e38afa652044f855e734a13b840a3f220cbd5d911ad5"],
...
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743856578)
@darosior
The static test cases are valuable, but they are multistep process to create and are hard to read. Imagine you write some code and get a test failure.
Would you rather the test failure be:
```JSON
{"tx": "f705d6e8019870958e85d1d8f94aa6d74746ba974db0f5ccae49a49b32dcada4e19de4eb5ecb00000000925977cc01f9875c000000000016001431d2b00cd4687ceb34008d9894de84062def14aa05406346", "prevouts": ["b4eae1010000000022512039f7e9232896f8100485e38afa652044f855e734a13b840a3f220cbd5d911ad5"],
...
💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2743883179)
reACK 1601906941fa559ebbee7898453fa77f4606ad38
> Dropping the TxGraphImpl destructor body then causes segfaults (rather than a nice assertion failure).
Right, I got myself turned around on this thinking it was harder to achieve. I'd suggest adding coverage here or in a quick follow-up.
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2743883179)
reACK 1601906941fa559ebbee7898453fa77f4606ad38
> Dropping the TxGraphImpl destructor body then causes segfaults (rather than a nice assertion failure).
Right, I got myself turned around on this thinking it was harder to achieve. I'd suggest adding coverage here or in a quick follow-up.
💬 instagibbs commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743888829)
> Would you rather the test failure be:
If you "walk" backwards to the functional test via the `comment` field it's ok, but I agree I would rather the tests cases be declared in the functional test rather than the dumped qa-assets version which lacks the context.
With regards to debugging, I found both this and https://github.com/bitcoin/bitcoin/pull/32114 similar in pain to review/fix, but I admit I've gotten used to gdb-attaching to bitcoind for functional tests regardless.
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743888829)
> Would you rather the test failure be:
If you "walk" backwards to the functional test via the `comment` field it's ok, but I agree I would rather the tests cases be declared in the functional test rather than the dumped qa-assets version which lacks the context.
With regards to debugging, I found both this and https://github.com/bitcoin/bitcoin/pull/32114 similar in pain to review/fix, but I admit I've gotten used to gdb-attaching to bitcoind for functional tests regardless.
💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2743896375)
> 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?
If we do that, I think it would make most sense to move some routines or (new) classes to the func
...
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2743896375)
> 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?
If we do that, I think it would make most sense to move some routines or (new) classes to the func
...