💬 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.
💬 davidgumberg commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2744568286)
Rebased to address @vasild feedback to drop `hardening_interface`, and added a check to `CMakeLists.txt` to avoid `-z separate-code` on NetBSD < 11.0 (thanks @hebasto). I've also updated the PR description.
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2744568286)
Rebased to address @vasild feedback to drop `hardening_interface`, and added a check to `CMakeLists.txt` to avoid `-z separate-code` on NetBSD < 11.0 (thanks @hebasto). I've also updated the PR description.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2008492939)
@ajtowns That's fair; I liked to think of TxGraph as something with well-specified behavior, but there are several instances of unspecified/best-effort behavior already:
* The cluster linearizations used (as no optimality is guaranteed)
* Reordering of tx removal / dep adding due to lazy batching
* Tie-breaking of order of same-chunk-feerate transactions from different clusters
* The exact logic Trim uses
Going to mark this resolved.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2008492939)
@ajtowns That's fair; I liked to think of TxGraph as something with well-specified behavior, but there are several instances of unspecified/best-effort behavior already:
* The cluster linearizations used (as no optimality is guaranteed)
* Reordering of tx removal / dep adding due to lazy batching
* Tie-breaking of order of same-chunk-feerate transactions from different clusters
* The exact logic Trim uses
Going to mark this resolved.
🤔 sipa reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2707582068)
Changes:
* Did a big pass through the interface documentation in txgraph.h
* Moved the support for Refs outliving TxGraph to a separate commit, and added the test suggested in https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2742606528.
* Some smaller things listed below:
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2707582068)
Changes:
* Did a big pass through the interface documentation in txgraph.h
* Moved the support for Refs outliving TxGraph to a separate commit, and added the test suggested in https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2742606528.
* Some smaller things listed below:
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2008491083)
Marking resolved.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2008491083)
Marking resolved.