💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2017542083)
No, I'll write a detailed comment later today or tomorrow, but it turns out the lack of this second pointer caused the pool test to fail, because on macOS and Windows; they really do have two pointers per node. I was able to determine this with certainty with debug printing (you can see it in some of the force-pushes above). I added a comment just before the start of this `struct unordered_node` definition to explain this second pointer, did you see that? I didn't call it forward and previous be
...
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2017542083)
No, I'll write a detailed comment later today or tomorrow, but it turns out the lack of this second pointer caused the pool test to fail, because on macOS and Windows; they really do have two pointers per node. I was able to determine this with certainty with debug printing (you can see it in some of the force-pushes above). I added a comment just before the start of this `struct unordered_node` definition to explain this second pointer, did you see that? I didn't call it forward and previous be
...
✅ darosior closed a pull request: "fuzz: a new target for the coins database"
(https://github.com/bitcoin/bitcoin/pull/28216)
(https://github.com/bitcoin/bitcoin/pull/28216)
💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2759383349)
Closing my old open PRs. I have intended to come back to it for a while but as a matter of fact i have not. Anyone interested feel free to pick it up.
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2759383349)
Closing my old open PRs. I have intended to come back to it for a while but as a matter of fact i have not. Anyone interested feel free to pick it up.
🚀 ryanofsky merged a pull request: "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py"
(https://github.com/bitcoin/bitcoin/pull/31874)
(https://github.com/bitcoin/bitcoin/pull/31874)
✅ darosior closed a pull request: "Wallet: don't underestimate the fees when spending a Taproot output"
(https://github.com/bitcoin/bitcoin/pull/26573)
(https://github.com/bitcoin/bitcoin/pull/26573)
💬 darosior commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2759384124)
Closing my old open PRs. I have intended to come back to it for a while but as a matter of fact i did not. Anyone interested feel free to pick it up.
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2759384124)
Closing my old open PRs. I have intended to come back to it for a while but as a matter of fact i did not. Anyone interested feel free to pick it up.
💬 fjahr commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2759425553)
Thanks for giving more context @darosior . I am certainly concept ACK on adding this new test and -0 on removing the old one. If other reviewers agree that the old test should be removed I'm fine with it.
We seem to have differing views on two points (that are related): 1. How much more robust the new test is than the old one, you described you view in detail now added the context for that and I just see a lot more ways that the new test could still break. 2. What level the test should focus
...
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2759425553)
Thanks for giving more context @darosior . I am certainly concept ACK on adding this new test and -0 on removing the old one. If other reviewers agree that the old test should be removed I'm fine with it.
We seem to have differing views on two points (that are related): 1. How much more robust the new test is than the old one, you described you view in detail now added the context for that and I just see a lot more ways that the new test could still break. 2. What level the test should focus
...
💬 kevkevinpal commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2759426759)
rebased to [29fa2b9](https://github.com/bitcoin/bitcoin/pull/29500/commits/29fa2b98a8988f9525773ae4b4994ea7f2d64776)
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2759426759)
rebased to [29fa2b9](https://github.com/bitcoin/bitcoin/pull/29500/commits/29fa2b98a8988f9525773ae4b4994ea7f2d64776)
💬 darosior commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2759457813)
Ok, i'll keep this open a couple more days in case someone else has a different opinion. Otherwise i guess this can be closed.
> What level the test should focus on. I would interpret what you write as you think it's not necessary to test failures that are caught by deserialization.
This is of course not what i say. Testing edge cases is good. It's just better if it's done deterministically.
Again, the test as it stands today does not check a deserialization error but a "height too high
...
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2759457813)
Ok, i'll keep this open a couple more days in case someone else has a different opinion. Otherwise i guess this can be closed.
> What level the test should focus on. I would interpret what you write as you think it's not necessary to test failures that are caught by deserialization.
This is of course not what i say. Testing edge cases is good. It's just better if it's done deterministically.
Again, the test as it stands today does not check a deserialization error but a "height too high
...
💬 fjahr commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759462479)
> @fjahr Did you try with `ASAN_OPTIONS=detect_container_overflow=0`? If so, did you get any issue?
I actually hadn't because for what I was working on I didn't need the sanitiziers so I went with nosan right away but I just tried it now and it does indeed fix it for me with this with or without corpus, e.g. this works:
```
$ cmake --preset=libfuzzer -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" -DCMAKE_EXE_LINKER_FL
...
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759462479)
> @fjahr Did you try with `ASAN_OPTIONS=detect_container_overflow=0`? If so, did you get any issue?
I actually hadn't because for what I was working on I didn't need the sanitiziers so I went with nosan right away but I just tried it now and it does indeed fix it for me with this with or without corpus, e.g. this works:
```
$ cmake --preset=libfuzzer -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" -DCMAKE_EXE_LINKER_FL
...
💬 1440000bytes commented on issue "bitcoind crash with corrupt wallet.dat":
(https://github.com/bitcoin/bitcoin/issues/32124#issuecomment-2759465941)
Lot of people who use fuzzing, maybe. They can fuzz loadwallet.
(https://github.com/bitcoin/bitcoin/issues/32124#issuecomment-2759465941)
Lot of people who use fuzzing, maybe. They can fuzz loadwallet.
💬 maflcko commented on issue "test: `p2p_message_capture.py` fails with GCC 14 & undefined sanitizer":
(https://github.com/bitcoin/bitcoin/issues/32016#issuecomment-2759479042)
> Maybe related: [#28761](https://github.com/bitcoin/bitcoin/issues/28761)
Yeah, it is the same one. It was fixed upstream in the standard as of this year, see https://www.open-std.org/JTC1/SC22/WG14/www/docs/n3466.pdf
(https://github.com/bitcoin/bitcoin/issues/32016#issuecomment-2759479042)
> Maybe related: [#28761](https://github.com/bitcoin/bitcoin/issues/28761)
Yeah, it is the same one. It was fixed upstream in the standard as of this year, see https://www.open-std.org/JTC1/SC22/WG14/www/docs/n3466.pdf
💬 maflcko commented on pull request "depends: patch Qt rounding bugs":
(https://github.com/bitcoin/bitcoin/pull/32081#issuecomment-2759495386)
I guess this won't be needed after https://github.com/bitcoin/bitcoin/pull/30997 (assuming that it makes it in for the next release)
(https://github.com/bitcoin/bitcoin/pull/32081#issuecomment-2759495386)
I guess this won't be needed after https://github.com/bitcoin/bitcoin/pull/30997 (assuming that it makes it in for the next release)
💬 maflcko commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2017626752)
I'd be surprised if anything made a difference here in this function and I think anything is fine here (`std::string` copy, `const std::string&` reference, or even a span or `std::string_view`).
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2017626752)
I'd be surprised if anything made a difference here in this function and I think anything is fine here (`std::string` copy, `const std::string&` reference, or even a span or `std::string_view`).
⚠️ 1440000bytes opened an issue: "Consensus code without testing"
(https://github.com/bitcoin/bitcoin/issues/32156)
https://github.com/bitcoin/bitcoin/pull/32155
This pull request is not discussed, reviewed, tested etc. enough to be merged in core.
I cannot comment in https://github.com/bitcoin/bitcoin/pull/32155 because they have blocked me on GitHub.
(https://github.com/bitcoin/bitcoin/issues/32156)
https://github.com/bitcoin/bitcoin/pull/32155
This pull request is not discussed, reviewed, tested etc. enough to be merged in core.
I cannot comment in https://github.com/bitcoin/bitcoin/pull/32155 because they have blocked me on GitHub.
💬 l0rinc commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759530458)
`ASAN_OPTIONS=detect_container_overflow=0` that's a weird syntax - unfortunately that still fails for me.
<details>
<summary>Details</summary>
```bash
ASAN_OPTIONS=detect_container_overflow=0 FUZZ=psbt_base64_decode build_fuzz/bin/fuzz
```
```bash
fuzz(61762,0x1fa5e4840) malloc: nano zone abandoned due to inability to reserve vm space.
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3061578036
INFO: Loaded 1 modules (1253375 inline 8-bit counters): 1253375 [0x
...
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2759530458)
`ASAN_OPTIONS=detect_container_overflow=0` that's a weird syntax - unfortunately that still fails for me.
<details>
<summary>Details</summary>
```bash
ASAN_OPTIONS=detect_container_overflow=0 FUZZ=psbt_base64_decode build_fuzz/bin/fuzz
```
```bash
fuzz(61762,0x1fa5e4840) malloc: nano zone abandoned due to inability to reserve vm space.
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3061578036
INFO: Loaded 1 modules (1253375 inline 8-bit counters): 1253375 [0x
...
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2017635268)
I find it a bit hard to check this, but wouldn't the assertion then be triggerable if an alternative block at that height is received? How about adding a `ReachedTargetHeight`, and use that in the `if` block? Something like:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 2d19ce45e5..da9efae00f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -6091 +6091 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(Chai
- !validate
...
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2017635268)
I find it a bit hard to check this, but wouldn't the assertion then be triggerable if an alternative block at that height is received? How about adding a `ReachedTargetHeight`, and use that in the `if` block? Something like:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 2d19ce45e5..da9efae00f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -6091 +6091 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation(Chai
- !validate
...
💬 maflcko commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2759535981)
> What I'm missing is a `SLOWCHECK`/`DCHECK` macro that is active in debug and always active in fuzz builds (and fails fuzz-builds on failure even in release).
The `Assume()` macro does exactly this, albeit the fuzz part is only documented in the code itself.
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2759535981)
> What I'm missing is a `SLOWCHECK`/`DCHECK` macro that is active in debug and always active in fuzz builds (and fails fuzz-builds on failure even in release).
The `Assume()` macro does exactly this, albeit the fuzz part is only documented in the code itself.
👍 brunoerg approved a pull request: "contrib: document asmap-tool commands more thoroughly"
(https://github.com/bitcoin/bitcoin/pull/32110#pullrequestreview-2723443303)
reACK 6afffba34e086de4cf0bb86729e12d116c1dcc9b
(https://github.com/bitcoin/bitcoin/pull/32110#pullrequestreview-2723443303)
reACK 6afffba34e086de4cf0bb86729e12d116c1dcc9b
💬 hodlinator commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2759540878)
> The `Assume()` macro does exactly this, albeit the fuzz part is only documented in the code itself.
Ah, nice. Was getting thrown off by `ABORT_ON_FAILED_ASSUME` being debug-only, but `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` seems to achieve what I'm after.
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2759540878)
> The `Assume()` macro does exactly this, albeit the fuzz part is only documented in the code itself.
Ah, nice. Was getting thrown off by `ABORT_ON_FAILED_ASSUME` being debug-only, but `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` seems to achieve what I'm after.