💬 murchandamus commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2744164157)
> In summery, two UTXOs are equivalent if both value and weight are the same, and if value is the same but weight is different, prefer the UTXO with lower weight since waste score is a derivative of weight.
Close, but there is a subtlety here. If we were sorting by effective_value and tie-breaking fee or weight, we would always be preferring lower weight, but instead, we tie-break on waste. That means that we will prefer lower weight if `fee_rate` is higher than `long_term_fee_rate`, but we wi
...
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2744164157)
> In summery, two UTXOs are equivalent if both value and weight are the same, and if value is the same but weight is different, prefer the UTXO with lower weight since waste score is a derivative of weight.
Close, but there is a subtlety here. If we were sorting by effective_value and tie-breaking fee or weight, we would always be preferring lower weight, but instead, we tie-break on waste. That means that we will prefer lower weight if `fee_rate` is higher than `long_term_fee_rate`, but we wi
...
💬 murchandamus commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2744215852)
Oh I had missed this, and noticed it via the new fuzz target corpora. Great idea!
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2744215852)
Oh I had missed this, and noticed it via the new fuzz target corpora. Great idea!
💬 sipa commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2008187157)
86, no?
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2008187157)
86, no?
💬 Sjors commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2008203657)
Indeed, fixed.
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2008203657)
Indeed, fixed.
💬 murchandamus commented on issue "Migrate from BTC/kvB to sat/vB on RPC and startup options":
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2744291162)
Could you please clarify the precision that you intend to use for sat/vB? I would recommend that we continue to use sat/kvB internally, maybe sat/kwu in the long-term, as we do only want to use integers but do want more precision than integer sat/vB. sat/kvB gives us three decimals of precision which seems sufficient.
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2744291162)
Could you please clarify the precision that you intend to use for sat/vB? I would recommend that we continue to use sat/kvB internally, maybe sat/kwu in the long-term, as we do only want to use integers but do want more precision than integer sat/vB. sat/kvB gives us three decimals of precision which seems sufficient.
👍 hodlinator approved a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2705248957)
re-ACK 7b51bf1d4c1933677867f7364a2378e41bc23929
#### Changes (`git range-diff master 94967c353e 7b51bf1d4c`)
* [Disable Vulkan](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781) in a different place to make sure support is properly disabled.
* Fixed invalid [cmake --parallel](https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005045288) invocation.
* Less [hardcoded loglevel](https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005002572).
* [Move tran
...
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2705248957)
re-ACK 7b51bf1d4c1933677867f7364a2378e41bc23929
#### Changes (`git range-diff master 94967c353e 7b51bf1d4c`)
* [Disable Vulkan](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781) in a different place to make sure support is properly disabled.
* Fixed invalid [cmake --parallel](https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005045288) invocation.
* Less [hardcoded loglevel](https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005002572).
* [Move tran
...
💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2007165031)
Was thinking we have `dnf`-commands in build-unix.md. But I guess it's not so useful in build-unix.md since most distros have pre-built Qt lib packages, so one doesn't need ninja (and distros that build from source would have ninja as a dependency for Qt anyway).
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2007165031)
Was thinking we have `dnf`-commands in build-unix.md. But I guess it's not so useful in build-unix.md since most distros have pre-built Qt lib packages, so one doesn't need ninja (and distros that build from source would have ninja as a dependency for Qt anyway).
💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#issuecomment-2744295836)
@jurraca
The asmap binary format effectively just encodes a mapping from ip ranges to AS numbers by encoding them as a sequence of match instructions, conditional jumps, and return values.
A whole lot of ranges are unmapped, however, i.e., there just is no AS number for them. What the `--fill` option does is allow the encoder to map these unmapped ranges to anything, picking whatever is most efficient to encode. For example, if `123.45.0.0/24` and `123.45.2.0/23` map to AS12345, but `123.4
...
(https://github.com/bitcoin/bitcoin/pull/32110#issuecomment-2744295836)
@jurraca
The asmap binary format effectively just encodes a mapping from ip ranges to AS numbers by encoding them as a sequence of match instructions, conditional jumps, and return values.
A whole lot of ranges are unmapped, however, i.e., there just is no AS number for them. What the `--fill` option does is allow the encoder to map these unmapped ranges to anything, picking whatever is most efficient to encode. For example, if `123.45.0.0/24` and `123.45.2.0/23` map to AS12345, but `123.4
...
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2008237122)
fixed
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2008237122)
fixed
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2008237336)
yes added - nothing bad would happen if we didn't (because BLOCK_FAILED_CHILD would be recalculated at next startup) but I think it's definitely better to do it.
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2008237336)
yes added - nothing bad would happen if we didn't (because BLOCK_FAILED_CHILD would be recalculated at next startup) but I think it's definitely better to do it.
💬 sipa commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2744314652)
@EthanHeilman Those are convincing arguments for adding simple tapscript unit tests, agreed.
> Is there a way to do script flag tests in the functional tests?
Only to the extent that it can be done by demonstrating differing behavior pre/post softfork activation, not per individual script validation flag.
> The static test cases are valuable, but they are multistep process to create and are hard to read. Imagine you write some code and get a test failure.
To be fair, I don't think th
...
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2744314652)
@EthanHeilman Those are convincing arguments for adding simple tapscript unit tests, agreed.
> Is there a way to do script flag tests in the functional tests?
Only to the extent that it can be done by demonstrating differing behavior pre/post softfork activation, not per individual script validation flag.
> The static test cases are valuable, but they are multistep process to create and are hard to read. Imagine you write some code and get a test failure.
To be fair, I don't think th
...
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2008242379)
Yes, I like that solution, it's less code and since we would `continue` when encountering an invalid header before we'd never set an invalid block as best header, so it's correct.
I added you as co-author of that commit with the email from your github profile - let me know if you want met to change anything about that!
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2008242379)
Yes, I like that solution, it's less code and since we would `continue` when encountering an invalid header before we'd never set an invalid block as best header, so it's correct.
I added you as co-author of that commit with the email from your github profile - let me know if you want met to change anything about that!
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2744317516)
[fe83133](https://github.com/bitcoin/bitcoin/commit/fe83133998cb7206281eb6d7a0a405aa025bee1b) to [7b6fe5a](https://github.com/bitcoin/bitcoin/commit/7b6fe5a2196d53a3024dfe7f11b316a50acc3cb7) - addressed feedback by @stringintech, thanks!
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2744317516)
[fe83133](https://github.com/bitcoin/bitcoin/commit/fe83133998cb7206281eb6d7a0a405aa025bee1b) to [7b6fe5a](https://github.com/bitcoin/bitcoin/commit/7b6fe5a2196d53a3024dfe7f11b316a50acc3cb7) - addressed feedback by @stringintech, thanks!
💬 jurraca commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#issuecomment-2744427175)
thanks for the explanation! Seems like the tradeoff could be a bit dangerous if there are a lot of unmapped ranges. Updated the `--fill` section to reflect the tradeoff a bit better.
(https://github.com/bitcoin/bitcoin/pull/32110#issuecomment-2744427175)
thanks for the explanation! Seems like the tradeoff could be a bit dangerous if there are a lot of unmapped ranges. Updated the `--fill` section to reflect the tradeoff a bit better.
📝 darosior opened a pull request: "qa: make feature_assumeutxo.py test more robust"
(https://github.com/bitcoin/bitcoin/pull/32117)
This is another robustness fix that i [dropped](https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2698108727) from https://github.com/bitcoin/bitcoin/pull/31910, intending to just tweak the error in my branch which triggers the issue. As i played more with it i eventually decided to submit a proper fix instead. Since the rationale initially [confused reviewers](https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1966033115) i tried to explain it in details in the commit message (r
...
(https://github.com/bitcoin/bitcoin/pull/32117)
This is another robustness fix that i [dropped](https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2698108727) from https://github.com/bitcoin/bitcoin/pull/31910, intending to just tweak the error in my branch which triggers the issue. As i played more with it i eventually decided to submit a proper fix instead. Since the rationale initially [confused reviewers](https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1966033115) i tried to explain it in details in the commit message (r
...
📝 brunoerg opened a pull request: "fuzz: wallet: fix crypter target"
(https://github.com/bitcoin/bitcoin/pull/32118)
The crypter target has an issue, it's calling `DecryptKey` with a random secret and a random public key that will unlikely be related to the key used to encrypt, so it won't have any effect. This PR changes fixes it and also removes the `DecryptSecret` call since this function is already (and only) called within `DecryptKey`.
(https://github.com/bitcoin/bitcoin/pull/32118)
The crypter target has an issue, it's calling `DecryptKey` with a random secret and a random public key that will unlikely be related to the key used to encrypt, so it won't have any effect. This PR changes fixes it and also removes the `DecryptSecret` call since this function is already (and only) called within `DecryptKey`.
💬 hebasto commented on issue "build: make macOS build Clang only":
(https://github.com/bitcoin/bitcoin/issues/30206#issuecomment-2744536272)
[Here](https://github.com/hebasto/bitcoin/tree/250321-qt6-clang) is a working proof of concept based on the branch from https://github.com/bitcoin/bitcoin/pull/30997. The last commit makes the macOS builds on Guix Clang only.
(https://github.com/bitcoin/bitcoin/issues/30206#issuecomment-2744536272)
[Here](https://github.com/hebasto/bitcoin/tree/250321-qt6-clang) is a working proof of concept based on the branch from https://github.com/bitcoin/bitcoin/pull/30997. The last commit makes the macOS builds on Guix Clang only.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008402492)
To clarify things, I reworked the commit and [provided](https://github.com/bitcoin/bitcoin/issues/30206#issuecomment-2744536272) a working proof of concept that addresses https://github.com/bitcoin/bitcoin/issues/30206.
The key point to keep in mind is that the build system for Qt 6 is entirely different from that of Qt 5. For this reason, I don't think this change could be split out.
However, the new approach is flexible enough to accommodate the needs of other native packages in the futu
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2008402492)
To clarify things, I reworked the commit and [provided](https://github.com/bitcoin/bitcoin/issues/30206#issuecomment-2744536272) a working proof of concept that addresses https://github.com/bitcoin/bitcoin/issues/30206.
The key point to keep in mind is that the build system for Qt 6 is entirely different from that of Qt 5. For this reason, I don't think this change could be split out.
However, the new approach is flexible enough to accommodate the needs of other native packages in the futu
...
💬 davidgumberg commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#discussion_r2008405401)
Fixed in latest push
(https://github.com/bitcoin/bitcoin/pull/32071#discussion_r2008405401)
Fixed in latest push
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2744566414)
1. Addressed the rest of the feedback from @fanquake.
2. Provided a working proof of concept addressinghttps://github.com/bitcoin/bitcoin/issues/30206, based ob this branch.
3. Fixed a bug when `make HOST=<host> NO_QT=1` builds the `native_qt` package.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2744566414)
1. Addressed the rest of the feedback from @fanquake.
2. Provided a working proof of concept addressinghttps://github.com/bitcoin/bitcoin/issues/30206, based ob this branch.
3. Fixed a bug when `make HOST=<host> NO_QT=1` builds the `native_qt` package.