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

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1414516901)
In commit "rpc: refactor single/batch requests" (125ef7a61885d2ffc0d32b66cdfb3dced2a4417b)

This change doesn't seem right because `jreq.parse` was just called above on line 196, and now the request is going to be parsed for a second time inside `JSONRPCExecOne` for no reason. I think this could be fixed by just dropping the `jreq.parse` call inside `JSONRPCExecOne`. It looks like later commit 4f22286c4aa65a113c44153847c857075a4225d6 does this anyway, so would be better to drop it up front and
...

💬 achow101 commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#issuecomment-1839550503)
ACK f05302427386fe63f4929a7198652cb1e4ab3bcc

👍 ryanofsky approved a pull request: "script, assumeutxo: Enhance validations in utxo_snapshot.sh"
(https://github.com/bitcoin/bitcoin/pull/28852#pullrequestreview-1763521007)
Code review ACK 11b7269d83a56f919f9dddb7f7c72a96d428627f

💬 ryanofsky commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#discussion_r1414558799)
In commit "script: Enhance validations in utxo_snapshot.sh" (11b7269d83a56f919f9dddb7f7c72a96d428627f)

Assignment seems a little out of place here, would suggest moving it below `PRUNE=...` with other variable assignments.

✅ ryanofsky closed an issue: "Stuck chainstate when utxo_snapshot.sh fails"
(https://github.com/bitcoin/bitcoin/issues/27841)

🚀 ryanofsky merged a pull request: "script, assumeutxo: Enhance validations in utxo_snapshot.sh"
(https://github.com/bitcoin/bitcoin/pull/28852)