💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201680165)
In commit "optimization: migrate fixed-size obfuscation from `std::vector<std::byte>` to `uint64_t`" (342bb224bb96b09d9950a144e62e60ede8710191)
IMO, it is haphazard for this code to define a `KEY_SIZE` constant and avoid hardcoding key sizes most places, but not define a key type and literally hardcode `uint64_t` and `Uint64` in other places.
It would seem better to either hardcode the key size consistently or not hardcode it. Would suggest not hardcoding it:
```diff
diff --git a/src/o
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201680165)
In commit "optimization: migrate fixed-size obfuscation from `std::vector<std::byte>` to `uint64_t`" (342bb224bb96b09d9950a144e62e60ede8710191)
IMO, it is haphazard for this code to define a `KEY_SIZE` constant and avoid hardcoding key sizes most places, but not define a key type and literally hardcode `uint64_t` and `Uint64` in other places.
It would seem better to either hardcode the key size consistently or not hardcode it. Would suggest not hardcoding it:
```diff
diff --git a/src/o
...
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201798917)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
This method does not seem safe to be a public method since unlike the other public methods, it will gives different results depending on whether the this is big or little endian machine. It seems not very useful to expose and like a potential footgun.
Would suggest making this private since it only seems to be called by tests, and if the tests are important would recommend
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201798917)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
This method does not seem safe to be a public method since unlike the other public methods, it will gives different results depending on whether the this is big or little endian machine. It seems not very useful to expose and like a potential footgun.
Would suggest making this private since it only seems to be called by tests, and if the tests are important would recommend
...
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201593284)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
Would suggest some changes to make usage of this Xor() function clearer:
- Instead taking separate span and size values then ignoring the size of the span, just take a span and use its actual size.
- Add an assert to ensure that size is not greater than 8 bytes.
- Rename from Xor to XorWord to make it obvious that this only useful for dealing with short spans not arbitrar
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201593284)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
Would suggest some changes to make usage of this Xor() function clearer:
- Instead taking separate span and size values then ignoring the size of the span, just take a span and use its actual size.
- Add an assert to ensure that size is not greater than 8 bytes.
- Rename from Xor to XorWord to make it obvious that this only useful for dealing with short spans not arbitrar
...
💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201705480)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
This seems more like a runtime deserialization error than something that is probably a programming error. Would suggest throwing `std::ios_base::failure` instead of `logic_error`.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2201705480)
In commit "refactor: encapsulate `vector`/`array` keys into `Obfuscation`" (74e33e7793bfee10a8aac3503b5c0a6db75aa787)
This seems more like a runtime deserialization error than something that is probably a programming error. Would suggest throwing `std::ios_base::failure` instead of `logic_error`.
💬 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