β οΈ maflcko opened an issue: "fuzz: package_rbf crashes after out-of-range memory read"
(https://github.com/bitcoin/bitcoin/issues/32121)
Diff to reproduce (turns UB into a runtime exception):
```diff
diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp
index 3e5b361186..74099f770d 100644
--- a/src/test/fuzz/rbf.cpp
+++ b/src/test/fuzz/rbf.cpp
@@ -118,7 +118,7 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
}
assert(iter <= g_outpoints.size());
replacement_tx->vin.resize(1);
- replacement_tx->vin[0].prevout = g_outpoints[iter++];
+ replacement_tx->vin[0].prevout = g_outpoints.at(iter++);
...
(https://github.com/bitcoin/bitcoin/issues/32121)
Diff to reproduce (turns UB into a runtime exception):
```diff
diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp
index 3e5b361186..74099f770d 100644
--- a/src/test/fuzz/rbf.cpp
+++ b/src/test/fuzz/rbf.cpp
@@ -118,7 +118,7 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
}
assert(iter <= g_outpoints.size());
replacement_tx->vin.resize(1);
- replacement_tx->vin[0].prevout = g_outpoints[iter++];
+ replacement_tx->vin[0].prevout = g_outpoints.at(iter++);
...
π maflcko opened a pull request: "fuzz: Fix off-by-one in package_rbf target"
(https://github.com/bitcoin/bitcoin/pull/32122)
Running the while loop up to `NUM_ITERS` times may set `iter` to `g_outpoints.size()`, which will then lead to an out-of-bounds read.
There was an assert, which I guess tried to catch this, but the condition in the assert was wrong as well.
Fix all issues by replacing the broken assert with the internal and correct check inside `std::vector::at` and by limiting `iter` to `NUM_ITERS` in the while loop.
(https://github.com/bitcoin/bitcoin/pull/32122)
Running the while loop up to `NUM_ITERS` times may set `iter` to `g_outpoints.size()`, which will then lead to an out-of-bounds read.
There was an assert, which I guess tried to catch this, but the condition in the assert was wrong as well.
Fix all issues by replacing the broken assert with the internal and correct check inside `std::vector::at` and by limiting `iter` to `NUM_ITERS` in the while loop.
π¬ maflcko commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2745083344)
Can be tested with the input and steps from https://github.com/bitcoin/bitcoin/issues/32121
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2745083344)
Can be tested with the input and steps from https://github.com/bitcoin/bitcoin/issues/32121
π¬ ajtowns commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2745105618)
> lgtm
Okay, done. How's that look?
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2745105618)
> lgtm
Okay, done. How's that look?
π¬ maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2008700665)
nit: Forgot to mention this, but to me it seems simpler to have the program automatically set this. Otherwise, it is just busy work for the dev.
in `src/test/fuzz/fuzz.cpp` you can set it to true, and default it to false, no?
This is also the code prior to the changes in https://github.com/bitcoin/bitcoin/pull/31191/files
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2008700665)
nit: Forgot to mention this, but to me it seems simpler to have the program automatically set this. Otherwise, it is just busy work for the dev.
in `src/test/fuzz/fuzz.cpp` you can set it to true, and default it to false, no?
This is also the code prior to the changes in https://github.com/bitcoin/bitcoin/pull/31191/files
π€ maflcko reviewed a pull request: "Rust tool to import bip39 mnemonic"
(https://github.com/bitcoin/bitcoin/pull/32115#pullrequestreview-2707937328)
> Or perhaps Rust Bitcoin could ship it as an example.
Yeah, right now it looks like this is just a few lines of docs and example code with the bulk being several imported rust-bitcoin dependencies. So I wonder what the benefits are to include it here, instead of somewhere closer where the majority of its (implicit) code sits.
(https://github.com/bitcoin/bitcoin/pull/32115#pullrequestreview-2707937328)
> Or perhaps Rust Bitcoin could ship it as an example.
Yeah, right now it looks like this is just a few lines of docs and example code with the bulk being several imported rust-bitcoin dependencies. So I wonder what the benefits are to include it here, instead of somewhere closer where the majority of its (implicit) code sits.
π¬ maflcko commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2008706369)
This is a bit confusing. What does "back it up" mean? The wallet, the mnemonic? Also, I am not sure about the risks that users are exposed to now. I've only read the first section of https://github.com/bitcoin/bips/wiki/Comments:BIP-0039 and it mentions loss of funds. So I think it would be better to educate the users about how wallet incompatibilities can silently lead to loss of funds, instead of implying that a backup of the mnemonic is sufficient for recovery.
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2008706369)
This is a bit confusing. What does "back it up" mean? The wallet, the mnemonic? Also, I am not sure about the risks that users are exposed to now. I've only read the first section of https://github.com/bitcoin/bips/wiki/Comments:BIP-0039 and it mentions loss of funds. So I think it would be better to educate the users about how wallet incompatibilities can silently lead to loss of funds, instead of implying that a backup of the mnemonic is sufficient for recovery.
π€ maflcko reviewed a pull request: "fuzz: wallet: fix crypter target"
(https://github.com/bitcoin/bitcoin/pull/32118#pullrequestreview-2707939655)
Would be nice to add a short line on how to test/observe this fix
(https://github.com/bitcoin/bitcoin/pull/32118#pullrequestreview-2707939655)
Would be nice to add a short line on how to test/observe this fix
π¬ maflcko commented on pull request "fuzz: wallet: fix crypter target":
(https://github.com/bitcoin/bitcoin/pull/32118#discussion_r2008708494)
Why is this needed?
(https://github.com/bitcoin/bitcoin/pull/32118#discussion_r2008708494)
Why is this needed?
π¬ jsarenik commented on issue "Migrate from BTC/kvB to sat/vB on RPC and startup options":
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2745135513)
One more RPC worth mentioning here is `getblockstats` which is currently showing sat/vB in its `feerate_percentiles` JSON array.
I would find it much more helpful if it printed the integer values in sat/kvB as mentioned by @murchandamus .
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2745135513)
One more RPC worth mentioning here is `getblockstats` which is currently showing sat/vB in its `feerate_percentiles` JSON array.
I would find it much more helpful if it printed the integer values in sat/kvB as mentioned by @murchandamus .
π¬ jsarenik commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2745145771)
utACK 6913904
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2745145771)
utACK 6913904
π¬ hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2745146334)
Updated 507c593f18fbd6635dbc29a8f67f6e277c2b33b8 -> 5b32eb7acca176efa4d20aa093ff56b8545eab05 ([pr30997.69](https://github.com/hebasto/bitcoin/commits/pr30997.69) -> [pr30997.70](https://github.com/hebasto/bitcoin/commits/pr30997.70), [diff](https://github.com/hebasto/bitcoin/compare/pr30997.69..pr30997.70)):
- addressed @fanquake's [comment](https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008659388)
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2745146334)
Updated 507c593f18fbd6635dbc29a8f67f6e277c2b33b8 -> 5b32eb7acca176efa4d20aa093ff56b8545eab05 ([pr30997.69](https://github.com/hebasto/bitcoin/commits/pr30997.69) -> [pr30997.70](https://github.com/hebasto/bitcoin/commits/pr30997.70), [diff](https://github.com/hebasto/bitcoin/compare/pr30997.69..pr30997.70)):
- addressed @fanquake's [comment](https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008659388)
π¬ hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008716515)
Thanks! The patch has been [applied](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2745146334).
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008716515)
Thanks! The patch has been [applied](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2745146334).
π¬ hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008720963)
These issues are somewhat related:
- https://bugreports.qt.io/browse/QTBUG-86283
- https://bugreports.qt.io/browse/QTBUG-114570
@fanquake
As a buildsystem maintainer, do you think this is a blocker for this PR?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008720963)
These issues are somewhat related:
- https://bugreports.qt.io/browse/QTBUG-86283
- https://bugreports.qt.io/browse/QTBUG-114570
@fanquake
As a buildsystem maintainer, do you think this is a blocker for this PR?
π¬ ajtowns commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2008731010)
Oh yeah, that's obviously better.
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2008731010)
Oh yeah, that's obviously better.
π€ hodlinator reviewed a pull request: "doc: clarify the documentation of `Assume` assertion"
(https://github.com/bitcoin/bitcoin/pull/32100#pullrequestreview-2707994339)
Concept ACK
Good to document useful aspects of `Assume`.
(https://github.com/bitcoin/bitcoin/pull/32100#pullrequestreview-2707994339)
Concept ACK
Good to document useful aspects of `Assume`.
π¬ hodlinator commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2008737842)
This seems sufficient to me:
```suggestion
expression is always evaluated. However, if the compiler can prove that a
statement inside `Assume` is side-effect-free, it may optimize the call away,
skipping its evaluation in production. This enables a lower-cost way of
making explicit statements about the code, aiding review.
```
Motivation:
> `Assume` can also act as a lightweight debugging assertion, ensuring statements are tested e.g., during fuzzingβto catch violations.
E
...
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2008737842)
This seems sufficient to me:
```suggestion
expression is always evaluated. However, if the compiler can prove that a
statement inside `Assume` is side-effect-free, it may optimize the call away,
skipping its evaluation in production. This enables a lower-cost way of
making explicit statements about the code, aiding review.
```
Motivation:
> `Assume` can also act as a lightweight debugging assertion, ensuring statements are tested e.g., during fuzzingβto catch violations.
E
...
π¬ hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008739707)
> Possibly, have we reported this (regression?) upstream?
Sure! Please see https://bugreports.qt.io/browse/QTBUG-135042.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008739707)
> Possibly, have we reported this (regression?) upstream?
Sure! Please see https://bugreports.qt.io/browse/QTBUG-135042.
π¬ stringintech commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2008742566)
Thanks a lot! Yes the email is okay.
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2008742566)
Thanks a lot! Yes the email is okay.
π€ spboy777 reviewed a pull request: "contrib: Make deterministic-coverage error messages more readable"
(https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2708009681)
A
(https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2708009681)
A