🤔 furszy reviewed a pull request: "index: Deduplicate HashKey / HeightKey handling"
(https://github.com/bitcoin/bitcoin/pull/32997#pullrequestreview-3094087935)
As `DBHeightKey` and `DBHashKey` are pretty similar. What do you think about a generic template for both? https://github.com/furszy/bitcoin-core/commit/e5dd2b02f15f9f68fbe84cc30255573ad23dbf2c.
The only specialization I had to make was for the field serialization.
(https://github.com/bitcoin/bitcoin/pull/32997#pullrequestreview-3094087935)
As `DBHeightKey` and `DBHashKey` are pretty similar. What do you think about a generic template for both? https://github.com/furszy/bitcoin-core/commit/e5dd2b02f15f9f68fbe84cc30255573ad23dbf2c.
The only specialization I had to make was for the field serialization.
💬 murchandamus commented on issue "Slow unit tests delay functional tests and leave CPU unused":
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3161509474)
I started looking into that as it aligns well with one of my projects to migrate the `coinselector_tests` to an updated test suite `coinselection_tests`.
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3161509474)
I started looking into that as it aligns well with one of my projects to migrate the `coinselector_tests` to an updated test suite `coinselection_tests`.
💬 pinheadmz commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3161538060)
> With this commit, anyone can build and run their own image directly from source.
That's what the release binaries are for? The security is the same: verifying a signature from a trusted developer either on a guix build or a commit in this repository.
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3161538060)
> With this commit, anyone can build and run their own image directly from source.
That's what the release binaries are for? The security is the same: verifying a signature from a trusted developer either on a guix build or a commit in this repository.
💬 achow101 commented on pull request "cmake: Install internal binaries to <prefix>/libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3161544820)
ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
```
$ find guix-build-f49840dd902c/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum 01:40:49 PM
9080e91998fa6f384c00a2e3aab6316fdd2174a89a303b0a602880daf515defb guix-build-f49840dd902c/output/aarch64-linux-gnu/SHA256SUMS.part
c4061ae7a239ea148d2423f7414d9dd03cefbdfd25c2142476fc400b57ccfef8 guix-build-f49840dd902c/output/aarch64-linux-gnu/bitcoin-f49840dd902c-aarch64-linux-gnu-debu
...
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3161544820)
ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
```
$ find guix-build-f49840dd902c/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum 01:40:49 PM
9080e91998fa6f384c00a2e3aab6316fdd2174a89a303b0a602880daf515defb guix-build-f49840dd902c/output/aarch64-linux-gnu/SHA256SUMS.part
c4061ae7a239ea148d2423f7414d9dd03cefbdfd25c2142476fc400b57ccfef8 guix-build-f49840dd902c/output/aarch64-linux-gnu/bitcoin-f49840dd902c-aarch64-linux-gnu-debu
...
💬 stickies-v commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3161571573)
re-ACK 8dd5c0a0447bbb6fe597aabfa725b397276da166
Thanks for addressing all comments and keeping everything super organized all the time. A pleasure to review your PR, again.
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3161571573)
re-ACK 8dd5c0a0447bbb6fe597aabfa725b397276da166
Thanks for addressing all comments and keeping everything super organized all the time. A pleasure to review your PR, again.
💬 Sammie05 commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3161583815)
<pre> ```diff diff --git a/CMakeLists.txt b/CMakeLists.txt --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -186,6 +186,7 @@ string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}") set(configure_warnings) +list(APPEND
configure_warnings "Testing https://github.com/bitcoin/bitcoin/pull/33144") include(CheckLinkerSupportsPIE) check_linker_supports_pie(configure_warnings) ``` </pre>
Tested locally — ACK
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3161583815)
<pre> ```diff diff --git a/CMakeLists.txt b/CMakeLists.txt --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -186,6 +186,7 @@ string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}") set(configure_warnings) +list(APPEND
configure_warnings "Testing https://github.com/bitcoin/bitcoin/pull/33144") include(CheckLinkerSupportsPIE) check_linker_supports_pie(configure_warnings) ``` </pre>
Tested locally — ACK
💬 darosior commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3161593678)
> maybe you're referring to not adding even more errors that can be converted into witness-stripped?
Yes this is what i meant. There are cases today where we would return WITNESS_STRIPPED for an otherwise invalid transaction (such as it could only be made valid by changing its txid, and we are missing the opportunity of adding its txid to the reject filter). This PR adds more such cases. This is minimized in the latest iteration (https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3156
...
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3161593678)
> maybe you're referring to not adding even more errors that can be converted into witness-stripped?
Yes this is what i meant. There are cases today where we would return WITNESS_STRIPPED for an otherwise invalid transaction (such as it could only be made valid by changing its txid, and we are missing the opportunity of adding its txid to the reject filter). This PR adds more such cases. This is minimized in the latest iteration (https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3156
...
💬 Sammie05 commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3161615471)
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -186,6 +186,7 @@
string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}")
set(configure_warnings)
+list(APPEND configure_warnings "Verifying AUTHOR_WARNING behavior (#33144)")
include(CheckLinkerSupportsPIE)
check_linker_supports_pie(configure_warnings)
Tested locally ACK
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3161615471)
diff --git a/CMakeLists.txt b/CMakeLists.txt
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -186,6 +186,7 @@
string(APPEND CMAKE_CXX_LINK_EXECUTABLE " ${APPEND_LDFLAGS}")
set(configure_warnings)
+list(APPEND configure_warnings "Verifying AUTHOR_WARNING behavior (#33144)")
include(CheckLinkerSupportsPIE)
check_linker_supports_pie(configure_warnings)
Tested locally ACK
💬 darosior commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2258302488)
If we can cheaply detect this txid refers to an always-invalid transaction and cache it, that seems like a win. It might be fine to cache even less such errors, but i'd rather this was done separately. There is already enough happening in this PR 14 days from the feature freeze.
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2258302488)
If we can cheaply detect this txid refers to an always-invalid transaction and cache it, that seems like a win. It might be fine to cache even less such errors, but i'd rather this was done separately. There is already enough happening in this PR 14 days from the feature freeze.
💬 sipa commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3161635101)
Concept ACK.
First of all, I'm convinced by the rationale for #33050 and #33105, and if we do (at least) the latter, then this PR isn't necessary anymore to get rid of the triple-validation costs, which would make it low-urgency. I think it's still a nice cleanup, as I don't think the code does much right now (see below).
Aside, I don't think we need to worry about the impact it may have on pre-segwit or pre-wtxidrelay peers; given how widespread wtxidrelay peers are, I think we can reason
...
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3161635101)
Concept ACK.
First of all, I'm convinced by the rationale for #33050 and #33105, and if we do (at least) the latter, then this PR isn't necessary anymore to get rid of the triple-validation costs, which would make it low-urgency. I think it's still a nice cleanup, as I don't think the code does much right now (see below).
Aside, I don't think we need to worry about the impact it may have on pre-segwit or pre-wtxidrelay peers; given how widespread wtxidrelay peers are, I think we can reason
...
💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258335380)
The CI failure is caused by `charlie` spending the `confirmed_v2` UTXO when it is creating one of the unconfirmed UTXOs.
In 2d37a5113ff0b0ab682788e4ef13c4cc223c17fe, we should still expect to see this failure, and in fact, more likely to see it (~50% of the time) because the UTXO pool of alice is much smaller at this point in time. The only reason we do not see it is because the test framework sets fallbackfee to 20 sat/vb which is in the high feerate regime which changes the behavior of coin
...
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2258335380)
The CI failure is caused by `charlie` spending the `confirmed_v2` UTXO when it is creating one of the unconfirmed UTXOs.
In 2d37a5113ff0b0ab682788e4ef13c4cc223c17fe, we should still expect to see this failure, and in fact, more likely to see it (~50% of the time) because the UTXO pool of alice is much smaller at this point in time. The only reason we do not see it is because the test framework sets fallbackfee to 20 sat/vb which is in the high feerate regime which changes the behavior of coin
...
🤔 w0xlt reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3094356552)
ACK https://github.com/bitcoin/bitcoin/pull/32896/commits/43a479ca6885b4222fb5e0f673354e54144d6835
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3094356552)
ACK https://github.com/bitcoin/bitcoin/pull/32896/commits/43a479ca6885b4222fb5e0f673354e54144d6835
🤔 jonatack reviewed a pull request: "Removing Bitcoin core text where unnecessary"
(https://github.com/bitcoin/bitcoin/pull/33126#pullrequestreview-3094421132)
Concept ACK, provided this small patch is robust. It's a minor change to be sure. Nevertheless, it seems cleaner not to hardcode it and would convey a magnanimous FOSS spirit that I encourage.
(https://github.com/bitcoin/bitcoin/pull/33126#pullrequestreview-3094421132)
Concept ACK, provided this small patch is robust. It's a minor change to be sure. Nevertheless, it seems cleaner not to hardcode it and would convey a magnanimous FOSS spirit that I encourage.
💬 achow101 commented on pull request "refactor,test: follow-ups to multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/33039#issuecomment-3161767528)
ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0
(https://github.com/bitcoin/bitcoin/pull/33039#issuecomment-3161767528)
ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0
💬 hodlinator commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2258425819)
Haven't explored this in depth, but my guess is that the zip filename comes from this line in `add_macos_deploy_target`:
https://github.com/bitcoin/bitcoin/blob/2c9aa1afa8dab8cf2c86b10010f1d3b342d93425/cmake/module/Maintenance.cmake#L87
An alternative approach might be to change that to something generic like `CFBundleName = \"BundleName\"`, or make it have something closer to the final name in the first place.
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2258425819)
Haven't explored this in depth, but my guess is that the zip filename comes from this line in `add_macos_deploy_target`:
https://github.com/bitcoin/bitcoin/blob/2c9aa1afa8dab8cf2c86b10010f1d3b342d93425/cmake/module/Maintenance.cmake#L87
An alternative approach might be to change that to something generic like `CFBundleName = \"BundleName\"`, or make it have something closer to the final name in the first place.
💬 murchandamus commented on issue "Slow unit tests delay functional tests and leave CPU unused":
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3161801097)
In the context of `coinselector_tests` being slow, I found looking at some flamegraphs that most of the time is spent on Knapsack and BnB. I have a [rewrite of BnB](https://github.com/bitcoin/bitcoin/pull/32150) open, that I would expect to improve BnB’s performance (and thus improve this test’s performance, in case someone is interested to review) and I have been prototyping another coin selection algorithm that I hope will bring the last missing piece to obsolete Knapsack (consolidatory behavi
...
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3161801097)
In the context of `coinselector_tests` being slow, I found looking at some flamegraphs that most of the time is spent on Knapsack and BnB. I have a [rewrite of BnB](https://github.com/bitcoin/bitcoin/pull/32150) open, that I would expect to improve BnB’s performance (and thus improve this test’s performance, in case someone is interested to review) and I have been prototyping another coin selection algorithm that I hope will bring the last missing piece to obsolete Knapsack (consolidatory behavi
...
🤔 achow101 reviewed a pull request: "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key"
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3094495318)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3094495318)
Concept ACK
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258450650)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`has_priv_key` should be a reference not a pointer. The pointer-ness is never used in this function.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258450650)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`has_priv_key` should be a reference not a pointer. The pointer-ness is never used in this function.
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258440013)
In 64213213b3691c9d93de95f880f820a640cd3c35 "descriptors: add IsWatchOnly()"
I think this function can be vastly simplified by calling `GetPrivKey` on all the `PubkeyProvider`, and `IsWatchOnly` on all subdescriptors.
nit: I don't like this function name since whether the descriptor is watchonly depends on the contents of the provided parameter. It's not a property like a `Is*` name implies. Watchonly also is not a property applied to descriptors as private keys are not actually part of th
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258440013)
In 64213213b3691c9d93de95f880f820a640cd3c35 "descriptors: add IsWatchOnly()"
I think this function can be vastly simplified by calling `GetPrivKey` on all the `PubkeyProvider`, and `IsWatchOnly` on all subdescriptors.
nit: I don't like this function name since whether the descriptor is watchonly depends on the contents of the provided parameter. It's not a property like a `Is*` name implies. Watchonly also is not a property applied to descriptors as private keys are not actually part of th
...
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258454050)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`assert`s should not have side effects. If we are calling a function to actually do something, it should not be in an `assert`.
However, if the return value of `ToStringHelper` is always supposed to be `true`, then it should not be returning anything anyways. Furthermore, I don't see why `ToStringHelper` doesn't return whether the string has a private key, as we are doing in this
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258454050)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`assert`s should not have side effects. If we are calling a function to actually do something, it should not be in an `assert`.
However, if the return value of `ToStringHelper` is always supposed to be `true`, then it should not be returning anything anyways. Furthermore, I don't see why `ToStringHelper` doesn't return whether the string has a private key, as we are doing in this
...