💬 TheCharlatan commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3063826306)
Are you running this on an older machine, or are missing some of the crypto accelerator features? Your reported runtimes are about an order of magnitude longer than mine and even our CI machines report the tests running much faster.
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3063826306)
Are you running this on an older machine, or are missing some of the crypto accelerator features? Your reported runtimes are about an order of magnitude longer than mine and even our CI machines report the tests running much faster.
💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201745084)
Remove duplicate definition of `m_cost_of_change`.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201745084)
Remove duplicate definition of `m_cost_of_change`.
💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201834598)
I don’t think so.
`max_spendable` starts at 0 and then sums the effective values of the generated UTXO (see line 258). As `max_spendable` grows, it limits the amount available to the remaining UTXOs as they are generated.
`+ group_pos.size() - max_output_groups` ensures that there is always at least 1 sat left to be assigned to the current UTXO.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201834598)
I don’t think so.
`max_spendable` starts at 0 and then sums the effective values of the generated UTXO (see line 258). As `max_spendable` grows, it limits the amount available to the remaining UTXOs as they are generated.
`+ group_pos.size() - max_output_groups` ensures that there is always at least 1 sat left to be assigned to the current UTXO.
💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201817713)
We already have the [`coinselection_bnb` fuzz target](https://github.com/bitcoin/bitcoin/blob/23e15d40b96a23112fba24cece5fe513ae1a2877/src/wallet/test/fuzz/coinselection.cpp#L347C1-L351C1) which runs BnB on UTXO pools of up to 10,000 UTXOs and validates that if BnB finds a solution that it falls in the solution window.
In contrast to the existing fuzz test, this fuzz target generates inputs that have solutions and don’t have solution. It then asserts that when the brute force search indicates t
...
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201817713)
We already have the [`coinselection_bnb` fuzz target](https://github.com/bitcoin/bitcoin/blob/23e15d40b96a23112fba24cece5fe513ae1a2877/src/wallet/test/fuzz/coinselection.cpp#L347C1-L351C1) which runs BnB on UTXO pools of up to 10,000 UTXOs and validates that if BnB finds a solution that it falls in the solution window.
In contrast to the existing fuzz test, this fuzz target generates inputs that have solutions and don’t have solution. It then asserts that when the brute force search indicates t
...
💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201729545)
The invariant you describe does not generally hold true. It would be possible that running BnB on the _same UTXO set_ with the same target at two different feerates would result in an input set with fewer inputs at the lower feerate than the result at the higher feerate due the difference in available combinations after factoring in fees (unless we are talking about two different UTXO sets that match in effective values at the different feerates).
We test for switch between consolidatory and th
...
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201729545)
The invariant you describe does not generally hold true. It would be possible that running BnB on the _same UTXO set_ with the same target at two different feerates would result in an input set with fewer inputs at the lower feerate than the result at the higher feerate due the difference in available combinations after factoring in fees (unless we are talking about two different UTXO sets that match in effective values at the different feerates).
We test for switch between consolidatory and th
...
💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201750551)
`change_fee` and `cost_of_change` are used in different calculations relating to the solution window of BnB and the waste calculation and in reality relate to the current feerate. Therefore it was instead chosen to fuzz the input and output size used to calculate them, but base them on the same feerate used for other calculations in the context.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201750551)
`change_fee` and `cost_of_change` are used in different calculations relating to the solution window of BnB and the waste calculation and in reality relate to the current feerate. Therefore it was instead chosen to fuzz the input and output size used to calculate them, but base them on the same feerate used for other calculations in the context.
💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201863280)
`max_spendable` is the sum of effective values of the generated UTXOs and they are being generated by being picking values from the gap that is left between `max_spendable` and `MAX_MONEY`.
Example:
- `MAX_MONEY` is 100.
- `max_output_groups` is 3.
1. The effective value for the first UTXO _A_ is generated randomly between `(1, MAX_MONEY + group_pos.size() - max_spendable - max_output_groups)`, i.e. (1, 100 + 0 - 0 - 3) = (1, 97).
Let’s say it’s 43. `max_spendable` becomes 43.
2. The effect
...
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2201863280)
`max_spendable` is the sum of effective values of the generated UTXOs and they are being generated by being picking values from the gap that is left between `max_spendable` and `MAX_MONEY`.
Example:
- `MAX_MONEY` is 100.
- `max_output_groups` is 3.
1. The effective value for the first UTXO _A_ is generated randomly between `(1, MAX_MONEY + group_pos.size() - max_spendable - max_output_groups)`, i.e. (1, 100 + 0 - 0 - 3) = (1, 97).
Let’s say it’s 43. `max_spendable` becomes 43.
2. The effect
...
💬 Prabhat1308 commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2201901206)
I think these all functions can be deleted. For `enum WalletFeature` , would we want to keep it for historical purpose ? or delete it and document this elsewhere .
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2201901206)
I think these all functions can be deleted. For `enum WalletFeature` , would we want to keep it for historical purpose ? or delete it and document this elsewhere .
💬 ajtowns commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3063912248)
> Are you running this on an older machine, or are missing some of the crypto accelerator features? Your reported runtimes are about an order of magnitude longer than mine and even our CI machines report the tests running much faster.
Hmm, it's a bit old I guess; 2019-era high-end desktop cpu, and running in a vm under qemu. It's also a debug build, with the extra checks that implies.
CI failures seem to indicate my cmake code doesn't work with something other than ninja as the test conduc
...
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3063912248)
> Are you running this on an older machine, or are missing some of the crypto accelerator features? Your reported runtimes are about an order of magnitude longer than mine and even our CI machines report the tests running much faster.
Hmm, it's a bit old I guess; 2019-era high-end desktop cpu, and running in a vm under qemu. It's also a debug build, with the extra checks that implies.
CI failures seem to indicate my cmake code doesn't work with something other than ninja as the test conduc
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201921789)
```diff
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 58ee99e10bf..b824b6b939e 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -409,11 +409,11 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage)
FUZZ_TARGET(txorphanage_sim)
{
SeedRandomStateForTest(SeedRand::ZEROS);
- // This is a comphehensive simulation fuzz test, which runs through a scenario involving up to 16 transactions
- // (which may have simple
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201921789)
```diff
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 58ee99e10bf..b824b6b939e 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -409,11 +409,11 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage)
FUZZ_TARGET(txorphanage_sim)
{
SeedRandomStateForTest(SeedRand::ZEROS);
- // This is a comphehensive simulation fuzz test, which runs through a scenario involving up to 16 transactions
- // (which may have simple
...
💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2201944039)
I don't think we need it either, for the history we could still browse back this PR in the repo.
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2201944039)
I don't think we need it either, for the history we could still browse back this PR in the repo.
🤔 hebasto reviewed a pull request: "Enable `-Werror=dev` in CI & Guix"
(https://github.com/bitcoin/bitcoin/pull/32937#pullrequestreview-3012113611)
My another Guix build:
```
x86_64
f73822833d8c3ff2b2dda2ddc5e16cb1794aa4eb299b1f468fa8dbadfd6b2310 guix-build-8f766f39df3e/output/aarch64-linux-gnu/SHA256SUMS.part
35783f79defa8b4032b78443a28e88a9c11314470fc177afa9e21d0e613bd937 guix-build-8f766f39df3e/output/aarch64-linux-gnu/bitcoin-8f766f39df3e-aarch64-linux-gnu-debug.tar.gz
b0345a28e82514d51cb363d798d950e44e630ba357a5e522537e1a7b4a37a151 guix-build-8f766f39df3e/output/aarch64-linux-gnu/bitcoin-8f766f39df3e-aarch64-linux-gnu.tar.gz
e
...
(https://github.com/bitcoin/bitcoin/pull/32937#pullrequestreview-3012113611)
My another Guix build:
```
x86_64
f73822833d8c3ff2b2dda2ddc5e16cb1794aa4eb299b1f468fa8dbadfd6b2310 guix-build-8f766f39df3e/output/aarch64-linux-gnu/SHA256SUMS.part
35783f79defa8b4032b78443a28e88a9c11314470fc177afa9e21d0e613bd937 guix-build-8f766f39df3e/output/aarch64-linux-gnu/bitcoin-8f766f39df3e-aarch64-linux-gnu-debug.tar.gz
b0345a28e82514d51cb363d798d950e44e630ba357a5e522537e1a7b4a37a151 guix-build-8f766f39df3e/output/aarch64-linux-gnu/bitcoin-8f766f39df3e-aarch64-linux-gnu.tar.gz
e
...
👍 hebasto approved a pull request: "Enable `-Werror=dev` in CI & Guix"
(https://github.com/bitcoin/bitcoin/pull/32937#pullrequestreview-3012133349)
ACK 8f766f39df3e312f79f461b2accc2f9c90fa6338, tested on Ubuntu 24.04.
(https://github.com/bitcoin/bitcoin/pull/32937#pullrequestreview-3012133349)
ACK 8f766f39df3e312f79f461b2accc2f9c90fa6338, tested on Ubuntu 24.04.
🚀 hebasto merged a pull request: "Enable `-Werror=dev` in CI & Guix"
(https://github.com/bitcoin/bitcoin/pull/32937)
(https://github.com/bitcoin/bitcoin/pull/32937)
📝 hebasto opened a pull request: "build: Explicitly set Qt's `AUTO{MOC,RCC,UIC}` property per target"
(https://github.com/bitcoin/bitcoin/pull/32951)
The codebase, which is compiled, consists of both files in the source tree and additional files generated during the build process. These generated files include headers such as `bitcoin-build-config.h`, headers generated for tests and benchmarks from data files, and files produced by Qt's tools, such as `moc`, `rcc` and `uic`.
When using Makefile or Ninja generators, CMake 3.31 and later provides a [convenient](https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497) builtin b
...
(https://github.com/bitcoin/bitcoin/pull/32951)
The codebase, which is compiled, consists of both files in the source tree and additional files generated during the build process. These generated files include headers such as `bitcoin-build-config.h`, headers generated for tests and benchmarks from data files, and files produced by Qt's tools, such as `moc`, `rcc` and `uic`.
When using Makefile or Ninja generators, CMake 3.31 and later provides a [convenient](https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497) builtin b
...
🤔 hodlinator reviewed a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-3010366559)
Light post-merge ACK
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-3010366559)
Light post-merge ACK
💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201918072)
nit: Missing `AssertLockHeld` in pre-existing `TxId` overload?
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201918072)
nit: Missing `AssertLockHeld` in pre-existing `TxId` overload?
💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201874252)
nit: There's 7 extra spaces in function prototypes for `GetRequestable` (this is 1 of 3 occurrences). This is due to changes to the code before the scripted-diff not accounting for a loss of `Variant` from the previous lines return value.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201874252)
nit: There's 7 extra spaces in function prototypes for `GetRequestable` (this is 1 of 3 occurrences). This is due to changes to the code before the scripted-diff not accounting for a loss of `Variant` from the previous lines return value.
💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201848886)
Thanks for taking this suggestion, including renaming the field!
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201848886)
Thanks for taking this suggestion, including renaming the field!
💬 hebasto commented on pull request "build: Explicitly set Qt's `AUTO{MOC,RCC,UIC}` property per target":
(https://github.com/bitcoin/bitcoin/pull/32951#issuecomment-3064055257)
My Guix build:
```
aarch64
6ab7ae50b219efd4675878dabfc783e0335bc67af31ad8bdfeb4d6317b66a054 guix-build-d38eca989aeb/output/aarch64-linux-gnu/SHA256SUMS.part
0ab47d80a7cc03eb654c5ad3b80b7a7ca7f0e02a92b072f21a395ff2508c1ee1 guix-build-d38eca989aeb/output/aarch64-linux-gnu/bitcoin-d38eca989aeb-aarch64-linux-gnu-debug.tar.gz
5fb1183124482b3c49411b8981743e9dba2c959b935a67e7ca938f5aad677ba3 guix-build-d38eca989aeb/output/aarch64-linux-gnu/bitcoin-d38eca989aeb-aarch64-linux-gnu.tar.gz
3eb65847
...
(https://github.com/bitcoin/bitcoin/pull/32951#issuecomment-3064055257)
My Guix build:
```
aarch64
6ab7ae50b219efd4675878dabfc783e0335bc67af31ad8bdfeb4d6317b66a054 guix-build-d38eca989aeb/output/aarch64-linux-gnu/SHA256SUMS.part
0ab47d80a7cc03eb654c5ad3b80b7a7ca7f0e02a92b072f21a395ff2508c1ee1 guix-build-d38eca989aeb/output/aarch64-linux-gnu/bitcoin-d38eca989aeb-aarch64-linux-gnu-debug.tar.gz
5fb1183124482b3c49411b8981743e9dba2c959b935a67e7ca938f5aad677ba3 guix-build-d38eca989aeb/output/aarch64-linux-gnu/bitcoin-d38eca989aeb-aarch64-linux-gnu.tar.gz
3eb65847
...