👍 dergoegge approved a pull request: "fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid"
(https://github.com/bitcoin/bitcoin/pull/28997#pullrequestreview-1764713862)
ACK 38816ff64ed90a55e4879e9b440cdc876302f750
(https://github.com/bitcoin/bitcoin/pull/28997#pullrequestreview-1764713862)
ACK 38816ff64ed90a55e4879e9b440cdc876302f750
💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840511168)
Added the upstream patch to fix Boost Process.
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840511168)
Added the upstream patch to fix Boost Process.
🚀 fanquake merged a pull request: "fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid"
(https://github.com/bitcoin/bitcoin/pull/28997)
(https://github.com/bitcoin/bitcoin/pull/28997)
💬 hebasto commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1840534127)
@luke-jr
> Weak Concept NACK: We should probably still build the SSE4.1 code if the compiler supports it.
This PR branch still builds the SSE4.1 code if the compiler supports it, no?
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1840534127)
@luke-jr
> Weak Concept NACK: We should probably still build the SSE4.1 code if the compiler supports it.
This PR branch still builds the SSE4.1 code if the compiler supports it, no?
💬 maflcko commented on pull request "type-safe(r) GenTxid constructors":
(https://github.com/bitcoin/bitcoin/pull/28658#discussion_r1415395926)
I don't think `explicit GenTxid(...)` requires changing a bunch of code. It is simply a constructor that can be used by new code, and old code can (for now) use the old methods to create the object.
Should be fine to add and use it, maybe as part of one of the follow-ups to convert more code?
(https://github.com/bitcoin/bitcoin/pull/28658#discussion_r1415395926)
I don't think `explicit GenTxid(...)` requires changing a bunch of code. It is simply a constructor that can be used by new code, and old code can (for now) use the old methods to create the object.
Should be fine to add and use it, maybe as part of one of the follow-ups to convert more code?
💬 maflcko commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1415458663)
One thing I don't like about `modernize-use-equals-default` is that it makes aggregate initialization possible again, when a developer disabled it by declaring a constructor.
Though, now that C++20 will be required soon, I guess it can be enabled. C.f.
```cpp
struct A {
// A() {}; // "main" fn fails to compile under C++17 and 20
A() = default; // "main" fn fails to compile under C++20
int b{};
};
int main() { (void)A{1}; }
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1415458663)
One thing I don't like about `modernize-use-equals-default` is that it makes aggregate initialization possible again, when a developer disabled it by declaring a constructor.
Though, now that C++20 will be required soon, I guess it can be enabled. C.f.
```cpp
struct A {
// A() {}; // "main" fn fails to compile under C++17 and 20
A() = default; // "main" fn fails to compile under C++20
int b{};
};
int main() { (void)A{1}; }
💬 maflcko commented on pull request "fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid":
(https://github.com/bitcoin/bitcoin/pull/28997#issuecomment-1840649876)
> Fixes the bugs in the fuzz test with no more changes as an alternative to #28658
Can you explain the "bugs"? The wtxid is equal to the txid for transactions that have not witness data, and all transactions in this test have no witness data.
Seems fine to fix the style and make this refactor, but I don't see any bugs.
Also, if there was a bug, the test should be fixed to catch it, no?
(https://github.com/bitcoin/bitcoin/pull/28997#issuecomment-1840649876)
> Fixes the bugs in the fuzz test with no more changes as an alternative to #28658
Can you explain the "bugs"? The wtxid is equal to the txid for transactions that have not witness data, and all transactions in this test have no witness data.
Seems fine to fix the style and make this refactor, but I don't see any bugs.
Also, if there was a bug, the test should be fixed to catch it, no?
🤔 josibake reviewed a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1764894179)
Looks good! I am uncomfortable with adding the log message as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop this commit for now and revisit debug logging in a separate PR, unrelated to the v26 release?
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1764894179)
Looks good! I am uncomfortable with adding the log message as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop this commit for now and revisit debug logging in a separate PR, unrelated to the v26 release?
💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1415478557)
in https://github.com/bitcoin/bitcoin/pull/28994/commits/06cd7e0bea4408cc508ad4074a44ddced3c773f8:
Why 31 and 68? It's a bit confusing as it seems like these numbers were chosen with intention, but when I changed them both to 0 the test still passed (and also still failed as expected with `params.m_subtract_fee_outputs = false`).
If there is a reason for choosing these numbers, it's worth documenting the reason in a comment. If the numbers are arbitrary, it would be better to use 0 for bot
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1415478557)
in https://github.com/bitcoin/bitcoin/pull/28994/commits/06cd7e0bea4408cc508ad4074a44ddced3c773f8:
Why 31 and 68? It's a bit confusing as it seems like these numbers were chosen with intention, but when I changed them both to 0 the test still passed (and also still failed as expected with `params.m_subtract_fee_outputs = false`).
If there is a reason for choosing these numbers, it's worth documenting the reason in a comment. If the numbers are arbitrary, it would be better to use 0 for bot
...
💬 martinus commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1415481883)
Ah, I wasn't aware of that, good to know :+1:
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1415481883)
Ah, I wasn't aware of that, good to know :+1:
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1840673663)
Updated to LLVM 17.0.6 in depends and based on top of #28622. Also sent a patch to update Guix to 17.0.6: https://lists.gnu.org/archive/html/guix-patches/2023-12/msg00226.html.
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1840673663)
Updated to LLVM 17.0.6 in depends and based on top of #28622. Also sent a patch to update Guix to 17.0.6: https://lists.gnu.org/archive/html/guix-patches/2023-12/msg00226.html.
💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840675170)
Guix Build (aarch64):
```bash
5d111a7c468338e34022cd20103ecd2c130a0d385dcf1d0f0110450b6a71243f guix-build-b3ab0c3819cf/output/arm64-apple-darwin/SHA256SUMS.part
e9270dbd3358f7d071533142c669ab0f13af63dc1786a9100a43f366539d03c1 guix-build-b3ab0c3819cf/output/arm64-apple-darwin/bitcoin-b3ab0c3819cf-arm64-apple-darwin-unsigned.tar.gz
2af5a689a4f9d7da60003ab0a4844d43239d88c21b984e870f1d8e6cc2d5e28e guix-build-b3ab0c3819cf/output/arm64-apple-darwin/bitcoin-b3ab0c3819cf-arm64-apple-darwin-unsign
...
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840675170)
Guix Build (aarch64):
```bash
5d111a7c468338e34022cd20103ecd2c130a0d385dcf1d0f0110450b6a71243f guix-build-b3ab0c3819cf/output/arm64-apple-darwin/SHA256SUMS.part
e9270dbd3358f7d071533142c669ab0f13af63dc1786a9100a43f366539d03c1 guix-build-b3ab0c3819cf/output/arm64-apple-darwin/bitcoin-b3ab0c3819cf-arm64-apple-darwin-unsigned.tar.gz
2af5a689a4f9d7da60003ab0a4844d43239d88c21b984e870f1d8e6cc2d5e28e guix-build-b3ab0c3819cf/output/arm64-apple-darwin/bitcoin-b3ab0c3819cf-arm64-apple-darwin-unsign
...
💬 fanquake commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1840685446)
> The PR description has been updated to mention one more fixed bug.
Is this #28993, which turned out to not be a bug, or is this a different bug? Because that conclusion from #28993 was that cross-compilation for Darwin, using master, does work? If so, what is this PR fixing in regards to compiling arm-64-apple-darwin?
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1840685446)
> The PR description has been updated to mention one more fixed bug.
Is this #28993, which turned out to not be a bug, or is this a different bug? Because that conclusion from #28993 was that cross-compilation for Darwin, using master, does work? If so, what is this PR fixing in regards to compiling arm-64-apple-darwin?
💬 hebasto commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1840695572)
> > The PR description has been updated to mention one more fixed bug.
>
> Is this #28993, which turned out to not be a bug, or is this a different bug?
A different one, which is outdated `config.guess` and `config.sub` files in the `capnp` package.
> Because that conclusion from #28993 was that cross-compilation for Darwin, using master, does work?
Right. Cross-compiling for `x86_64-apple-darwin` works indeed.
> If so, what is this PR fixing in regards to cross-compiling for `ar
...
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1840695572)
> > The PR description has been updated to mention one more fixed bug.
>
> Is this #28993, which turned out to not be a bug, or is this a different bug?
A different one, which is outdated `config.guess` and `config.sub` files in the `capnp` package.
> Because that conclusion from #28993 was that cross-compilation for Darwin, using master, does work?
Right. Cross-compiling for `x86_64-apple-darwin` works indeed.
> If so, what is this PR fixing in regards to cross-compiling for `ar
...
💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1415543551)
31 -> p2wpkh output size.
68 -> p2wpkh input size (high-r signature).
The previous test version didn't have any hardcoded value. However, since we are now calling a low-level function, we need to hardcode them. Setting them to 0 would be incorrect. Even though they are not currently in use on this process, these values must align with those used in the wallet. We learned this lesson the hard way; the `coinselector_tests` is full of invalid values that lack real meaning, affecting the introdu
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1415543551)
31 -> p2wpkh output size.
68 -> p2wpkh input size (high-r signature).
The previous test version didn't have any hardcoded value. However, since we are now calling a low-level function, we need to hardcode them. Setting them to 0 would be incorrect. Even though they are not currently in use on this process, these values must align with those used in the wallet. We learned this lesson the hard way; the `coinselector_tests` is full of invalid values that lack real meaning, affecting the introdu
...
💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1415562873)
Cool, agree with using real-world values, but I think it's worth documenting why those specific values were chosen in a comment since the choice of values isn't relevant to the test.
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1415562873)
Cool, agree with using real-world values, but I think it's worth documenting why those specific values were chosen in a comment since the choice of values isn't relevant to the test.
💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1840765663)
> I am uncomfortable with adding the log message as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop this commit for now and revisit debug logging in a separate PR, unrelated to the v26 release?
Logs aren't meant for end users; logs are useful for us to understand what is going on internally. Debug issues and pro
...
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1840765663)
> I am uncomfortable with adding the log message as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop this commit for now and revisit debug logging in a separate PR, unrelated to the v26 release?
Logs aren't meant for end users; logs are useful for us to understand what is going on internally. Debug issues and pro
...
💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1840778172)
> Logs aren't meant for end users
Strongly disagree with this statement, and regardless of who they are intended for, users (people who run nodes) have access to these logs and do look at them.
> This is not different to what we do for the [fee calculation](https://github.com/bitcoin/bitcoin/blob/b3ab0c3819cf36e902989e0544365807ac0823e6/src/wallet/spend.cpp#L1263). And there is more documentation about the selection waste than doc about each of the logged [fee reasons](https://github.com/b
...
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1840778172)
> Logs aren't meant for end users
Strongly disagree with this statement, and regardless of who they are intended for, users (people who run nodes) have access to these logs and do look at them.
> This is not different to what we do for the [fee calculation](https://github.com/bitcoin/bitcoin/blob/b3ab0c3819cf36e902989e0544365807ac0823e6/src/wallet/spend.cpp#L1263). And there is more documentation about the selection waste than doc about each of the logged [fee reasons](https://github.com/b
...
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1840779612)
@martinus something similar was mentioned in https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-457720636. However, with your approach every access would require 4 lookups.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1840779612)
@martinus something similar was mentioned in https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-457720636. However, with your approach every access would require 4 lookups.
💬 hebasto commented on issue "`capnp` fails when cross-compiling":
(https://github.com/bitcoin/bitcoin/issues/28993#issuecomment-1840785721)
> That makes more sense. Thanks for the bug report in any case!
I was careless and didn't run `make distclean`.
(https://github.com/bitcoin/bitcoin/issues/28993#issuecomment-1840785721)
> That makes more sense. Thanks for the bug report in any case!
I was careless and didn't run `make distclean`.