📝 furszy opened a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994)
Solves #28918. Coming from https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1838626406 discussion.

The intention is to decouple only the bugfix relevant commits from #28985, allowing them to be included in the 26.x release. This way, we can avoid disabling the coin selection fuzzing test for an entire release.

Note:
Have introduced few changes to the bug fix commit so that the unit tests pass without the additional burden introduced in #28985.

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1414077590)
Yes, I believe the `LockCoin` function does mutate the global `wallet`. I'll need to figure out how to make this deterministic then.

I apologize for the delayed response, I was/am occupied with something else. But, I'm looking forward to brainstorming this and finding a solution as soon as possible.

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1414079147)
Thank you! I'll rebase in the next force push.

💬 maflcko commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414083527)
Is this used? What would this be used for? Seems easier to use a bool, no?

💬 fanquake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414086098)
In d62d49c605f89a325b929e8fb48091b0b56399ee: is there really no other way to know which coin selection algorithm is being used, than to grep output logs for non-existence? This doesn't seem like the best way to test for a condition.

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414078838)
The `DISABLE` case doesn’t seem to get used anywhere.

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414092780)
```suggestion
fee_to_deduct = available_coins.coins[OutputType::BECH32].begin()->GetFee() * 2; // x2 because we expect two inputs
const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT - fee_to_deduct, coin_control, coin_selection_params_bnb);
```

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414092139)
Nit: I think "deduct" would fit better here than "deduce":

```suggestion
CAmount fee_to_deduct = available_coins.coins[OutputType::BECH32].begin()->GetFee();
const auto result11 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT - fee_to_deduct, coin_control, coin_selection_params_bnb);
```

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414093367)
```suggestion
const auto result13 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT - fee_to_deduct, coin_control, coin_selection_params_bnb);
```

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414093219)
```suggestion
fee_to_deduct = available_coins.coins[OutputType::BECH32].begin()->GetFee() * 2; // x2 because we expect two inputs
```

👍 ryanofsky approved a pull request: "depends: Build the `native_capnp` and `capnp` packages with CMake"
(https://github.com/bitcoin/bitcoin/pull/28856#pullrequestreview-1762769796)
Code review ACK 11d797e3a078b8f5f0039a1073047d3f0a8c6cdc. Since last review arm64-apple-darwin platform is now mentioned in the commit message, and the change to `depends/packages/libmultiprocess.mk` in d1604d4b1d1ee8df279a1776303e167cc3d06193 which was unrelated (but probably still a good optimization) was reverted.

👍 ryanofsky approved a pull request: "depends: fix libmultiprocess build on aarch64"
(https://github.com/bitcoin/bitcoin/pull/28846#pullrequestreview-1762817862)
Thanks for reopening, code review ACK 68823aa3c071b49754fe3396be49f5531aa33fd4 for just the third commit. This PR could be a draft since it is based on another PR.

💬 ryanofsky commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1414128811)
In commit "depends: always install libmultiprocess to /lib" (68823aa3c071b49754fe3396be49f5531aa33fd4)

I still think it would be nice to add some code comment here, maybe like the one I suggested earlier:

> \# Hardcode library install path to "lib" to match the PKG_CONFIG_PATH setting in depends/config.site.in which also hardcodes "lib". Without this setting, cmake by default would use the OS library directory, which might be "lib64" or something else, not "lib", on multiarch systems.

I
...

💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1414135724)
Sure, pushed up your comment and added co-author.

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414142645)
I actually did it in this way per achow request during coredev. My initial version involved returning the coin selection information in a separate struct https://github.com/furszy/bitcoin-core/commit/a1b2c53d1feddf748d7d91a884fe52e50998f61e in the tx creation result.

I do prefer my previous approach because it would allows us to retrieve this information at the RPC level as well. However, I am also fine going with this other option as well. It's not something widely used nowadays anyway. We c
...

👍 ryanofsky approved a pull request: "depends: fix libmultiprocess build on aarch64"
(https://github.com/bitcoin/bitcoin/pull/28846#pullrequestreview-1762850175)
Code review ACK 1a90ac42e2591b5105ac846a5f6d07ec4b95fa1a, just adding comment since last review (thanks!)

💬 dergoegge commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414154588)
> My initial version involved returning the coin selection information in a separate struct https://github.com/furszy/bitcoin-core/commit/a1b2c53d1feddf748d7d91a884fe52e50998f61e in the tx creation result.

Your initial version seems better to me. Looks like about the same amount of code or less for the better outcome. Checking for log messages in tests is a bit brittle and usually results in more churn down the line when log messages change.

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414156191)
No, it is not used. IIRC, I did it in this way to get a mechanism to enable/disable the logs watcher on the fly, without requiring to create a new one per watched message.
Can migrate it to use a bool too. Was just considering potential future utilities.

💬 maflcko commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414161933)
nit from tidy CI:


```
tainer_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/spend.cpp
wallet/spend.cpp:1266:28: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors]
1266 | wallet.WalletLogPrintf("Coin Selection: Algorithm:%s, Waste:%d", GetAlgorithmName(result.GetAlgo()), result.GetWaste());
| ^
|
...

💬 dergoegge commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1839024177)
Concept ACK