💬 kevkevinpal commented on pull request "tests: Added progress tracker when running fuzz test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/32354#issuecomment-2843234359)
Pushed [9d08d71](https://github.com/bitcoin/bitcoin/pull/32354/commits/9d08d7140fc91f60d6d09497633b207a18aa5d92)
It adds the duration it took to run the fuzz target
Example
```
1/219 - addition_overflow, Duration: 0.44 s
2/219 - address_deserialize, Duration: 0.56 s
3/219 - addr_info_deserialize, Duration: 0.72 s
4/219 - asmap, Duration: 0.36 s
5/219 - asmap_direct, Duration: 1.41 s
6/219 - autofile, Duration: 1.41 s
7/219 - base32_encode_decode, Duration: 0.28 s
8/219 - base58_en
...
(https://github.com/bitcoin/bitcoin/pull/32354#issuecomment-2843234359)
Pushed [9d08d71](https://github.com/bitcoin/bitcoin/pull/32354/commits/9d08d7140fc91f60d6d09497633b207a18aa5d92)
It adds the duration it took to run the fuzz target
Example
```
1/219 - addition_overflow, Duration: 0.44 s
2/219 - address_deserialize, Duration: 0.56 s
3/219 - addr_info_deserialize, Duration: 0.72 s
4/219 - asmap, Duration: 0.36 s
5/219 - asmap_direct, Duration: 1.41 s
6/219 - autofile, Duration: 1.41 s
7/219 - base32_encode_decode, Duration: 0.28 s
8/219 - base58_en
...
💬 rebroad commented on pull request "Enhanced Traffic Graph Widget with Multi-timeframe Support and Data Persistence":
(https://github.com/bitcoin-core/gui/pull/866#discussion_r2069423549)
> The GUI should not be changing low-level internals.
>
> If for some reason the clientModel is changed, its data should be taken as authoritative, even if it contradicts the history so far (if that's an issue, wipe the history).
>
> Alternatively, maybe the history should be stored with the node, and loaded into the GUI (or RPC, in another hypothetical PR).
@luke-jr I've removed the setTotalBytes functions as I agree with your reasoning. The GUI keeps a local track of the totals from t
...
(https://github.com/bitcoin-core/gui/pull/866#discussion_r2069423549)
> The GUI should not be changing low-level internals.
>
> If for some reason the clientModel is changed, its data should be taken as authoritative, even if it contradicts the history so far (if that's an issue, wipe the history).
>
> Alternatively, maybe the history should be stored with the node, and loaded into the GUI (or RPC, in another hypothetical PR).
@luke-jr I've removed the setTotalBytes functions as I agree with your reasoning. The GUI keeps a local track of the totals from t
...
🤔 Christewart reviewed a pull request: "test: test MAX_SCRIPT_SIZE for block validity"
(https://github.com/bitcoin/bitcoin/pull/32304#pullrequestreview-2808445583)
Perhaps worth adding a P2WSH test while you are at it?
>P2WSH allows maximum script size of 10,000 bytes, as the 520-byte push limit is bypassed.
https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#p2wsh
(https://github.com/bitcoin/bitcoin/pull/32304#pullrequestreview-2808445583)
Perhaps worth adding a P2WSH test while you are at it?
>P2WSH allows maximum script size of 10,000 bytes, as the 520-byte push limit is bypassed.
https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#p2wsh
🤔 achow101 reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2808401804)
I think the first 2 commits could be combined. The first commit is harder to review since it introduces a bunch of helpers that are completely unused. In fact, the file is introduced but not to the build system, so we can't even check if it compiles.
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2808401804)
I think the first 2 commits could be combined. The first commit is harder to review since it introduces a bunch of helpers that are completely unused. In fact, the file is introduced but not to the build system, so we can't even check if it compiles.
💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2069436214)
In 1c1087132eb88c1e4e5a3ded89c8712ac8a54638 "test: Recreate BnB iteration exhaustion test"
nit: This line is double the length of all the other ones, it could use as linebreak somewhere.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2069436214)
In 1c1087132eb88c1e4e5a3ded89c8712ac8a54638 "test: Recreate BnB iteration exhaustion test"
nit: This line is double the length of all the other ones, it could use as linebreak somewhere.
💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2069416151)
In 30479487c26dec47ac609009838d82b53cce4a3c "test: Recreate simple BnB success tests"
This is essentially a reimplementation of `util::Join`. This could be a one-liner of:
```suggestion
return "[" + util::Join(selection.GetInputSet(), " ", [](const auto& input){ return util::ToString(input->txout.nValue);}) + "]";
```
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2069416151)
In 30479487c26dec47ac609009838d82b53cce4a3c "test: Recreate simple BnB success tests"
This is essentially a reimplementation of `util::Join`. This could be a one-liner of:
```suggestion
return "[" + util::Join(selection.GetInputSet(), " ", [](const auto& input){ return util::ToString(input->txout.nValue);}) + "]";
```
🤔 w0xlt reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2808453203)
ACK https://github.com/bitcoin/bitcoin/pull/29532/commits/dbf1f2663b1afbe03d6b1855f83db604bc79979e
> I ran each commit, and I did not receive this warning. Could you let me know which commit resulted in that warning for you?
I was caused by the `coinselection_tests.cpp` as you mentioned here: https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2065255362
> Do you mean that I introduce helper functions like "TestBnBSuccess" for the new tests instead of making them their own test c
...
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2808453203)
ACK https://github.com/bitcoin/bitcoin/pull/29532/commits/dbf1f2663b1afbe03d6b1855f83db604bc79979e
> I ran each commit, and I did not receive this warning. Could you let me know which commit resulted in that warning for you?
I was caused by the `coinselection_tests.cpp` as you mentioned here: https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2065255362
> Do you mean that I introduce helper functions like "TestBnBSuccess" for the new tests instead of making them their own test c
...
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2069453148)
As mentioned in https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813590511, I can't always reproduce these locally, it's why used the CI to verify why these were needed in the first place.
I have tried every combination I could think of locally, I couldn't reproduce any such errors in coins. If you can, please let me know how to do that exactly.
Is there any other CI run I should be looking at to see if there are other failures?
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2069453148)
As mentioned in https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813590511, I can't always reproduce these locally, it's why used the CI to verify why these were needed in the first place.
I have tried every combination I could think of locally, I couldn't reproduce any such errors in coins. If you can, please let me know how to do that exactly.
Is there any other CI run I should be looking at to see if there are other failures?
💬 davidgumberg commented on issue "build: x86 afl++ ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2843318720)
> Could someone provide configuration options and a line from the verbose build to show what exactly is broken?
Reproduced as follows:
```bash
# clone and checkout hash before addr san was disabled
git clone git@github.com:dergoegge/fuzzamoto.git && cd fuzzamoto && git checkout 3f72735
docker build .
# or, to get full logs
docker build --no-cache --progress=plain . 2>&1 | tee build.log
```
Here is the configure line, and noteworthy output:
```bash
$ cmake -B build_fuzz --toolchain ./d
...
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2843318720)
> Could someone provide configuration options and a line from the verbose build to show what exactly is broken?
Reproduced as follows:
```bash
# clone and checkout hash before addr san was disabled
git clone git@github.com:dergoegge/fuzzamoto.git && cd fuzzamoto && git checkout 3f72735
docker build .
# or, to get full logs
docker build --no-cache --progress=plain . 2>&1 | tee build.log
```
Here is the configure line, and noteworthy output:
```bash
$ cmake -B build_fuzz --toolchain ./d
...
💬 ryanofsky commented on pull request "[DRAFT] ipc: add windows support":
(https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2843318872)
Updated 87432b6a4325e09a13c912d77b386daa3832b34d -> 0a35e21103b7fed6e46c809e9029446277b6f6ee ([`pr/ipc-win.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.2) -> [`pr/ipc-win.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-win.2..pr/ipc-win.3)) to fix test CI failure https://cirrus-ci.com/task/5583224107171840
---
re: https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2843024844
Agree change
...
(https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2843318872)
Updated 87432b6a4325e09a13c912d77b386daa3832b34d -> 0a35e21103b7fed6e46c809e9029446277b6f6ee ([`pr/ipc-win.2`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.2) -> [`pr/ipc-win.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-win.2..pr/ipc-win.3)) to fix test CI failure https://cirrus-ci.com/task/5583224107171840
---
re: https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2843024844
Agree change
...
🤔 w0xlt reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2808489885)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2808489885)
Concept ACK.
💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2069519376)
renamed to `convert_to_json_for_cli`
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2069519376)
renamed to `convert_to_json_for_cli`
💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2843416255)
> If this ends up fixing all issues, should add `--usecli` to atleast one CI, so we catch regressions?
I have added `--usecli` to one CI job now (multiprocess, i686, DEBUG)
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2843416255)
> If this ends up fixing all issues, should add `--usecli` to atleast one CI, so we catch regressions?
I have added `--usecli` to one CI job now (multiprocess, i686, DEBUG)
🤔 w0xlt reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2808627212)
ACK https://github.com/bitcoin/bitcoin/pull/27286/commits/26014804bb792bde141baf3e9d9ac30b6a27c988
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2808627212)
ACK https://github.com/bitcoin/bitcoin/pull/27286/commits/26014804bb792bde141baf3e9d9ac30b6a27c988
💬 hebasto commented on issue "build: x86 afl++ ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2843487857)
@davidgumberg
> > Could someone provide configuration options and a line from the verbose build to show what exactly is broken?
>
> Reproduced as follows:
Thank you!
Could you please try the following patch:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 99490f742a..47d50b6ba7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -192,9 +192,9 @@ set(APPEND_CXXFLAGS "" CACHE STRING "(Objective) C++ compiler flags that are app
set(APPEND_LDFLAGS "" CACHE STRING "Linker flags that
...
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2843487857)
@davidgumberg
> > Could someone provide configuration options and a line from the verbose build to show what exactly is broken?
>
> Reproduced as follows:
Thank you!
Could you please try the following patch:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 99490f742a..47d50b6ba7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -192,9 +192,9 @@ set(APPEND_CXXFLAGS "" CACHE STRING "(Objective) C++ compiler flags that are app
set(APPEND_LDFLAGS "" CACHE STRING "Linker flags that
...
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2069552602)
Done - moved to after the write.
https://github.com/bitcoin/bitcoin/compare/b7f3b6fd96ca2d5d1f9538affef682f778bd07e8..2aadd877f0db599d67d7ca74b8173b0148c9e098.
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2069552602)
Done - moved to after the write.
https://github.com/bitcoin/bitcoin/compare/b7f3b6fd96ca2d5d1f9538affef682f778bd07e8..2aadd877f0db599d67d7ca74b8173b0148c9e098.
💬 achow101 commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#issuecomment-2843503818)
ACK fa0993462b3bc8db40c1a6ea2e7f5fd4a22353a6
(https://github.com/bitcoin/bitcoin/pull/32344#issuecomment-2843503818)
ACK fa0993462b3bc8db40c1a6ea2e7f5fd4a22353a6
💬 davidgumberg commented on issue "test: functional test failures under -DENABLE_WALLET=OFF":
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2843508491)
I couldn't reproduce this on x86_64 linux, could you share the `build/test/config.ini` file?
AFAICT, none of these lines that result in a crash should be running when `is_wallet_compiled() == false`, and that's based on the contents of config.ini:
https://github.com/bitcoin/bitcoin/blob/68ac9f116c0228a277f18f60ba2278b56356e6ac/test/functional/test_framework/test_framework.py#L268-L270
https://github.com/bitcoin/bitcoin/blob/68ac9f116c0228a277f18f60ba2278b56356e6ac/test/functional/test_framewo
...
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2843508491)
I couldn't reproduce this on x86_64 linux, could you share the `build/test/config.ini` file?
AFAICT, none of these lines that result in a crash should be running when `is_wallet_compiled() == false`, and that's based on the contents of config.ini:
https://github.com/bitcoin/bitcoin/blob/68ac9f116c0228a277f18f60ba2278b56356e6ac/test/functional/test_framework/test_framework.py#L268-L270
https://github.com/bitcoin/bitcoin/blob/68ac9f116c0228a277f18f60ba2278b56356e6ac/test/functional/test_framewo
...
💬 davidgumberg commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2069567781)
`nNow` is still getting set above before the write
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2069567781)
`nNow` is still getting set above before the write
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2069580172)
Indeed - fixed!
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r2069580172)
Indeed - fixed!