💬 hebasto commented on issue "macos: dmg not working on macOS Sonoma (14.x)":
(https://github.com/bitcoin/bitcoin/issues/28767#issuecomment-1838840548)
Closing for now. Feel free to reopen if relevant.

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1838855510)
> I don’t know about automatically flushing out <= 0 fees even when the local mempool is empty. One could add significant computing payload on its mining competitors by throwing buffy packages with 0 fees parent and then replace the high-fee child (in a cycle).

The replacements are paying for those CPU cycles and bandwidth via "incremental feerate", it's equivalent to an entire package simply being RBF'd like normal. Keeping 0-fee parents hanging around in mempool would also allow new entrie
...

🤔 sr-gi reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1762709866)
ACK [f895ae4](https://github.com/bitcoin/bitcoin/pull/28765/commits/f895ae4b7aa7263acba1adb3886c47a36f40b024)

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1414062310)
nit: if you have created a refactor commit to deal with removing the TODOs, it may be worth moving that change there.

Not sure if you're planning to squash it or not. Feel free to disregard.

💬 hebasto commented on pull request "depends: Include `config.guess` and `config.sub` into `meta_depends`":
(https://github.com/bitcoin/bitcoin/pull/28870#issuecomment-1838878294)
cc @fanquake

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