💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415150781)
You're right, I didn't think about the `operator bool`. Thanks.

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415152306)
> But I think just making the call and logging an error when the call fails is the most straightforward approach.

Agreed, can be marked as resolved.

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415149823)
That makes sense, and I agree with your rationale, so I think my suggestion here in `test/util/index.cpp` indeed should be disregarded. In `node::AbortNode()` though, we are sending an interrupt signal to shutdown the node, so I still think it would make sense [there](https://github.com/bitcoin/bitcoin/pull/28051/files#diff-01b4a8b64504f4f4879e9a7fbac613d882a54975c9629c814e844463fb786a8fR19-R25)? (non-blocking nit, ofc)

💬 stickies-v commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1415185397)
> Lower level code should just be returning errors and notifications, and high level code should be using that information to decide whether to shut down

That does seem like a better design, indeed. I agree then that having the bad approach be more verbose is not something to worry about, and my suggestion should be discarded. Thank you for providing the rationale, I think this might be useful to add to the PR description?

👍 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

💬 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.

🚀 fanquake merged a pull request: "fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid"
(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?

💬 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?

💬 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}; }

💬 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?

🤔 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?

💬 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
...

💬 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:

💬 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.

💬 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
...

💬 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?

💬 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
...

💬 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
...

💬 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.