👍 hebasto approved a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160)
ACK 0440c406eedbc16fa1ce6c77a802b7ea60a79057, the [Guix-built](https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2456981291) `test_bitcoin.exe`, `bitcoind.exe`, `bitcoin-qt.exe`, and the installer have been tested on Windows 11 Pro 23H2.
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160)
ACK 0440c406eedbc16fa1ce6c77a802b7ea60a79057, the [Guix-built](https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2456981291) `test_bitcoin.exe`, `bitcoind.exe`, `bitcoin-qt.exe`, and the installer have been tested on Windows 11 Pro 23H2.
💬 brunoerg commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2457018427)
Are you still working on it?
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2457018427)
Are you still working on it?
💬 ryanofsky commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457030352)
> I understand your concerns, but did you actually encounter them in reality?
Yes, I've never written or run a fuzz test because they require separate setup and because I'm reasonably happy writing unit tests. If I could run fuzz tests in my default build, I'd be more likely to write fuzz tests as an alternative to unit tests. I understand fuzz tests are not always an alternative to unit tests, but I've written a lot of unit tests where I'm just handcrafting inputs to test edge cases, and the
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457030352)
> I understand your concerns, but did you actually encounter them in reality?
Yes, I've never written or run a fuzz test because they require separate setup and because I'm reasonably happy writing unit tests. If I could run fuzz tests in my default build, I'd be more likely to write fuzz tests as an alternative to unit tests. I understand fuzz tests are not always an alternative to unit tests, but I've written a lot of unit tests where I'm just handcrafting inputs to test edge cases, and the
...
🤔 noelportillo reviewed a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2415492049)
The only way to make the money
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2415492049)
The only way to make the money
💬 sipa commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457048406)
@ryanofsky As long as we're talking about libfuzzer-based fuzzing (which is what we mostly use in this project, though not exclusively), the `-fsanitize=fuzzer` option does two things:
1. It instruments the code to keep track of branch/code coverage in a libfuzzer-compatible way
2. It add the libfuzzer main() function.
These pretty much inherently require a separate build: you can't do fuzzing without (1), because the mutations that libfuzzer makes to the corpus are based on coverage inform
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457048406)
@ryanofsky As long as we're talking about libfuzzer-based fuzzing (which is what we mostly use in this project, though not exclusively), the `-fsanitize=fuzzer` option does two things:
1. It instruments the code to keep track of branch/code coverage in a libfuzzer-compatible way
2. It add the libfuzzer main() function.
These pretty much inherently require a separate build: you can't do fuzzing without (1), because the mutations that libfuzzer makes to the corpus are based on coverage inform
...
💬 maflcko commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457071014)
> Yes, I've never written or run a fuzz test because they require separate setup
I see. I think what you are asking for could be achieved by reverting this pull request, possibly hiding the performance impact behind `-DCMAKE_BUILD_TYPE=Debug` (or another cmake dev-only flag). Then, you could use the fuzz executable from the "normal" build (without sanitizers and a fuzz engine) for blackbox fuzzing in AFL++ QEMU mode (or something similar). However, for me personally just going with a separate
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457071014)
> Yes, I've never written or run a fuzz test because they require separate setup
I see. I think what you are asking for could be achieved by reverting this pull request, possibly hiding the performance impact behind `-DCMAKE_BUILD_TYPE=Debug` (or another cmake dev-only flag). Then, you could use the fuzz executable from the "normal" build (without sanitizers and a fuzz engine) for blackbox fuzzing in AFL++ QEMU mode (or something similar). However, for me personally just going with a separate
...
💬 ryanofsky commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457117169)
Thanks for clarifications and ideas. Iit sounds like doing what I want would require `fuzzer-no-link` instead of `fuzzer` but otherwise it should be possible for BUILD_FOR_FUZZING to just enable extra instrumentation and not make every library and every other binary unusable. To me this just seems like a most straightforward way and least surprising way of organizing the build, that would make fuzzing easier to get started with by just being able to turn on an option that doesn't break everythin
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457117169)
Thanks for clarifications and ideas. Iit sounds like doing what I want would require `fuzzer-no-link` instead of `fuzzer` but otherwise it should be possible for BUILD_FOR_FUZZING to just enable extra instrumentation and not make every library and every other binary unusable. To me this just seems like a most straightforward way and least surprising way of organizing the build, that would make fuzzing easier to get started with by just being able to turn on an option that doesn't break everythin
...
💬 sipa commented on pull request "Update secp256k1 subtree to v0.6.0":
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2457195648)
Another included benefit is that the default test iteration count for the secp256k1 `tests` binary has been reduced, which means Bitcoin Core's `ctest` run should speed up (the `tests` binary is the single longest running task currently).
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2457195648)
Another included benefit is that the default test iteration count for the secp256k1 `tests` binary has been reduced, which means Bitcoin Core's `ctest` run should speed up (the `tests` binary is the single longest running task currently).
👍 tdb3 approved a pull request: "test: cover edge case for lockunspent rpc"
(https://github.com/bitcoin/bitcoin/pull/31209#pullrequestreview-2415647691)
ACK e58c4307ef576915200c4eda57d6d6ff10088667
Thank you.
Coverage report seems to indicate coverage was added.
Sanity checked the change by executing functional tests (without legacy-wallet, but with `wallet_basic.py --descriptors`) with the following update.
```diff
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index f1430a3c601..81fd75db0fd 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -320,7 +320,7 @@ RPCHelpMan lockunspent()
...
(https://github.com/bitcoin/bitcoin/pull/31209#pullrequestreview-2415647691)
ACK e58c4307ef576915200c4eda57d6d6ff10088667
Thank you.
Coverage report seems to indicate coverage was added.
Sanity checked the change by executing functional tests (without legacy-wallet, but with `wallet_basic.py --descriptors`) with the following update.
```diff
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index f1430a3c601..81fd75db0fd 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -320,7 +320,7 @@ RPCHelpMan lockunspent()
...
👍 ryanofsky approved a pull request: "Prune mining interface"
(https://github.com/bitcoin/bitcoin/pull/31196#pullrequestreview-2415644803)
Code review 6d46e067b71901308433afd7c70d4ea64c2a1d1b, but the rpc_generate.py test is currently broken (https://cirrus-ci.com/task/4929149096165376?logs=ci#L4665). Suggested a fix below
(https://github.com/bitcoin/bitcoin/pull/31196#pullrequestreview-2415644803)
Code review 6d46e067b71901308433afd7c70d4ea64c2a1d1b, but the rpc_generate.py test is currently broken (https://cirrus-ci.com/task/4929149096165376?logs=ci#L4665). Suggested a fix below
💬 ryanofsky commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#discussion_r1829361447)
In commit "Remove testBlockValidity() from mining interface" (4d4a1822ba8eef2ff857885c5253156dacff8d3c)
This change seem to be causing the rpc_generate.py test to fail. Can be fixed with:
```diff
--- a/test/functional/rpc_generate.py
+++ b/test/functional/rpc_generate.py
@@ -87,7 +87,7 @@ class RPCGenerateTest(BitcoinTestFramework):
txid1 = miniwallet.send_self_transfer(from_node=node)['txid']
utxo1 = miniwallet.get_utxo(txid=txid1)
rawtx2 = miniwallet.crea
...
(https://github.com/bitcoin/bitcoin/pull/31196#discussion_r1829361447)
In commit "Remove testBlockValidity() from mining interface" (4d4a1822ba8eef2ff857885c5253156dacff8d3c)
This change seem to be causing the rpc_generate.py test to fail. Can be fixed with:
```diff
--- a/test/functional/rpc_generate.py
+++ b/test/functional/rpc_generate.py
@@ -87,7 +87,7 @@ class RPCGenerateTest(BitcoinTestFramework):
txid1 = miniwallet.send_self_transfer(from_node=node)['txid']
utxo1 = miniwallet.get_utxo(txid=txid1)
rawtx2 = miniwallet.crea
...
💬 ryanofsky commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#discussion_r1829368566)
In commit "Remove testBlockValidity() from mining interface" (4d4a1822ba8eef2ff857885c5253156dacff8d3c)
Review note: it seemed like a potential problem that this check is being dropped, but looking more closely it should be ok. In the `getblocktemplate` "proposal" call this condition is already checked in, and in the `generateblock` call the check wouldn't reliable because cs_main isn't hold, and more reliable checking happens later when the block is connected.
(https://github.com/bitcoin/bitcoin/pull/31196#discussion_r1829368566)
In commit "Remove testBlockValidity() from mining interface" (4d4a1822ba8eef2ff857885c5253156dacff8d3c)
Review note: it seemed like a potential problem that this check is being dropped, but looking more closely it should be ok. In the `getblocktemplate` "proposal" call this condition is already checked in, and in the `generateblock` call the check wouldn't reliable because cs_main isn't hold, and more reliable checking happens later when the block is connected.
👍 ryanofsky approved a pull request: "refactor: mining interface 30955 followups"
(https://github.com/bitcoin/bitcoin/pull/31197#pullrequestreview-2415686066)
Code review ACK 058862581085316927287817b2af01e8f4765a1d. Looks good, thanks for the cleanups!
(https://github.com/bitcoin/bitcoin/pull/31197#pullrequestreview-2415686066)
Code review ACK 058862581085316927287817b2af01e8f4765a1d. Looks good, thanks for the cleanups!
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829410358)
Fixed in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829410358)
Fixed in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829410806)
Fixed in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829410806)
Fixed in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5
💬 maflcko commented on pull request "test: cover edge case for lockunspent rpc":
(https://github.com/bitcoin/bitcoin/pull/31209#issuecomment-2457266568)
Not sure about this. This seems to be adding coverage for code that shouldn't exist in the first place.
The integer is passed to `COutPoint`, which only takes unsigned integers, so parsing the integer as such would be more consistent. The check would then be redundant and could be removed.
(https://github.com/bitcoin/bitcoin/pull/31209#issuecomment-2457266568)
Not sure about this. This seems to be adding coverage for code that shouldn't exist in the first place.
The integer is passed to `COutPoint`, which only takes unsigned integers, so parsing the integer as such would be more consistent. The check would then be redundant and could be removed.
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829412307)
Updated comment in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829412307)
Updated comment in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829420178)
that's a mistake, I'll fix if I touch the PR
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829420178)
that's a mistake, I'll fix if I touch the PR
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829420263)
will fixup if I end up touching things
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829420263)
will fixup if I end up touching things
💬 hebasto commented on pull request "Update secp256k1 subtree to v0.6.0":
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2457287984)
> Another included benefit is that the default test iteration count for the secp256k1 `tests` binary has been reduced, which means Bitcoin Core's `ctest` run should speed up (the `tests` binary is the single longest running task currently); see [bitcoin-core/secp256k1#1581](https://github.com/bitcoin-core/secp256k1/pull/1581).
Additionally, the subtree tests can now match `ctest` regex options, such as `-E` or `-R`:
```
$ ctest --test-dir build -j 16 -R secp256k1
```
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2457287984)
> Another included benefit is that the default test iteration count for the secp256k1 `tests` binary has been reduced, which means Bitcoin Core's `ctest` run should speed up (the `tests` binary is the single longest running task currently); see [bitcoin-core/secp256k1#1581](https://github.com/bitcoin-core/secp256k1/pull/1581).
Additionally, the subtree tests can now match `ctest` regex options, such as `-E` or `-R`:
```
$ ctest --test-dir build -j 16 -R secp256k1
```