👍 brunoerg approved a pull request: "net: Continuous ASMap health check"
(https://github.com/bitcoin/bitcoin/pull/27581#pullrequestreview-1763350846)
crACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3

code lgtm, will test in practice soon!

🤔 ryanofsky reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1763352955)
Rebased 437a0c2aa1f0c6b55bdc4a2d91d2b7f0b1cb8e69 -> 8a02ce59ffa16c73611aeda6caf8b54d85c6208f ([`pr/noshut.18`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.18) -> [`pr/noshut.19`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.19), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/noshut.18-rebase..pr/noshut.19)) due to conflict with #28946

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414460418)
For end users, it may not be particularly useful. However, I think that additional information at the RPC level could be beneficial for other utilities, like a coin selection metrics tool or for connected applications that uses core as their backend.
That said, there's no need to add it unless there's a need for it.

Other than that, will return to the struct commit then.

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414501781)
Yeah. That would work as well.
The `CreateTransaction` usage comes from the previous version of this work (before we agreed to disable BnB when SFFO is enabled). Where BnB was enabled and this was verifying that the resulting transaction didn't contain a change output.

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414510843)
Done as suggested

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414510887)
Done as suggested

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414510991)
Done as suggested

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414511167)
Thx, fixed.

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414511497)
Pulled.

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1414455724)
In ec308d7873f7eb4d597c00708091ede55e9ff5a5 "coinselection: Add CoinGrinder algorithm"

I wish there was a better way to write feerate*3. This is kinda hard to read.

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1414513187)
In 6ba9c1cd1032c75e618d8ca7b7aee6ebed16d22d "opt: Cut if last addition was minimal weight"

Why 2x `max_weight` instead of just `max_weight`?

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1414479523)
In ec308d7873f7eb4d597c00708091ede55e9ff5a5 "coinselection: Add CoinGrinder algorithm"

Since this lambda is used in only one spot, I think it would be a bit easier to read to just have it inline.

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1414446693)
In 313d4df88360edf1bad1f3415166a96f0ee93f0a "opt: Tie-break UTXO sort by waste for BnB"

This comparison is a little hard to follow as it requires reminding yourself of the effects of the waste score. It wasn't immediately obvious why the weight matters for this comparison. It seems like it would be simpler to just actually calculate the wastes and compare them.

```suggestion
return (a.fee - a.long_term_fee) > (b.fee - b.long_term_fee);
```

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1839509876)
Updated per feedback. Thanks everyone.

Test wise; replaced the logs debugger approach for a low level function call (per https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1414434210). And applied
the test variables naming suggestions.

💬 willcl-ark commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414525515)
To make it even more clear to the end user that i) a backup is imperative and ii) that a new backup will contain both sets of descriptors, you could consider something like:

```
"After wallet encryption is complete and before making any transactions, you must immediately make a new backup of the wallet file, which will contain the new and old descriptors, using the `backupwallet` RPC.\n"
```

🤔 achow101 reviewed a pull request: "Add Gutter Guard Selector"
(https://github.com/bitcoin/bitcoin/pull/28977#pullrequestreview-1763468491)
Concept ACK

💬 achow101 commented on pull request "Add Gutter Guard Selector":
(https://github.com/bitcoin/bitcoin/pull/28977#discussion_r1414530480)
In 1e19f925bb3d2b74933d8cb6d3628477cdd58c32 "Add Gutter Guard Selector"

These two loops could probably be consolidated into a single one since they do the same thing, just under different conditions.

💬 achow101 commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1414538735)
In 5101f054ce435ac653a3f5e780fca066e2f25088 "test: test sendall and send do anti-fee-sniping"

nit: `gettransaction` has a `verbose` argument that does the decoding so this doesn't need to call `decoderawtransaction`. Same below.

💬 achow101 commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1414539439)
In 5101f054ce435ac653a3f5e780fca066e2f25088 "test: test sendall and send do anti-fee-sniping"

nit: `getrawtransaction` has a `verbosity` option that will do the decoding, so no need to call `decoderawtransaction` here.

💬 achow101 commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1414539685)
In 5101f054ce435ac653a3f5e780fca066e2f25088 "test: test sendall and send do anti-fee-sniping"

Shouldn't this check for `getblockcount() - 100`?