π¬ 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
π€ spboy777 reviewed a pull request: "depends: Fix `CXXFLAGS` on NetBSD"
(https://github.com/bitcoin/bitcoin/pull/31502#pullrequestreview-2708018703)
F
(https://github.com/bitcoin/bitcoin/pull/31502#pullrequestreview-2708018703)
F
π¬ yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2745259917)
> Close, but there is a subtlety here..
Thanks, that's a good point I might have overlooked, that in a high fee environment, the tie break on weight is different than in a low fee environment.
> Unfortunately not. We explored sorting by descending value density while developing CoinGrinder..
Hmm that make sense now that you say it. sorting by density isn't strictly equivalent to sorting by value with weight as a tie break and as such would change the behavior of the search.
So, it sounds li
...
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2745259917)
> Close, but there is a subtlety here..
Thanks, that's a good point I might have overlooked, that in a high fee environment, the tie break on weight is different than in a low fee environment.
> Unfortunately not. We explored sorting by descending value density while developing CoinGrinder..
Hmm that make sense now that you say it. sorting by density isn't strictly equivalent to sorting by value with weight as a tie break and as such would change the behavior of the search.
So, it sounds li
...