🤔 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`.
💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1840791636)
> This is irrelevant to my point that this is not a bug fix and shouldn't be included in a bug fix PR. I also don't agree with the justification and claim that coin selection is well-documented.
Where did I claim that coin selection is well-documented? I said that it is more documented than fee estimation. And fee estimation is being logged.
> 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 lo
...
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1840791636)
> This is irrelevant to my point that this is not a bug fix and shouldn't be included in a bug fix PR. I also don't agree with the justification and claim that coin selection is well-documented.
Where did I claim that coin selection is well-documented? I said that it is more documented than fee estimation. And fee estimation is being logged.
> 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 lo
...
🚀 fanquake merged a pull request: "depends: Build the `native_capnp` and `capnp` packages with CMake"
(https://github.com/bitcoin/bitcoin/pull/28856)
(https://github.com/bitcoin/bitcoin/pull/28856)
💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1840801375)
Rebased after #28856.
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1840801375)
Rebased after #28856.
🤔 stickies-v reviewed a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-1764747210)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-1764747210)
Concept ACK
💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1415619542)
I think this approach is more unsafe than it has to be, it's pretty easy to not handle the `error`. Would it make more sense to make the ctor private and add a `util::Result<CTxMemPool> Make(const Options& opts)` factory method?
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1415619542)
I think this approach is more unsafe than it has to be, it's pretty easy to not handle the `error`. Would it make more sense to make the ctor private and add a `util::Result<CTxMemPool> Make(const Options& opts)` factory method?
💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1415372320)
I don't think this actually does anything? (+[here](https://github.com/bitcoin/bitcoin/pull/28830/files#diff-07fe07ecfff4dd7c233a453006567140ff20d226fd73a20406df14982b1e415bR132))
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1415372320)
I don't think this actually does anything? (+[here](https://github.com/bitcoin/bitcoin/pull/28830/files#diff-07fe07ecfff4dd7c233a453006567140ff20d226fd73a20406df14982b1e415bR132))
💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1840805224)
> Where did I claim that coin selection is well-documented?
Sorry, my comment should say `s/well-documented/more documented/`
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1840805224)
> Where did I claim that coin selection is well-documented?
Sorry, my comment should say `s/well-documented/more documented/`