💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228375478)
Changed

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228375629)
Done

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228376733)
Done

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228376844)
Done

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228377838)
Done

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228378132)
Added a sentence in the doc.

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228378394)
Added a check and the test.

🚀 fanquake merged a pull request: "fuzz: Fix mini_miner_selection running out of coin"
(https://github.com/bitcoin/bitcoin/pull/27806)

💬 achow101 commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#discussion_r1228391144)
Added a hint.

👍 Xekyo approved a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1477583153)
ACK 2d739a425c92e2dd409c45273e8e376d39ead1cc

My concern about the `min_viable_change` and `change_fee` causing issues did not substantiate after some light fuzzing. Since it does not seem to cause crashes, having independent values for the two parameters may be better. Thanks for adding more coverage.

💬 Xekyo commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1228387547)
Nevermind that. I ran over about 500k executions on the coinselection fuzz test and could not substantiate my concern.

🤔 hebasto reviewed a pull request: "ci: enable AArch64 target in MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27824#pullrequestreview-1477606221)
Post-merge ACK 2ebeb421dd21a69344a32c681ccbfcf5e5c6dab9. Tested on Ubuntu 23.04, `aarch64`.

FWIW, the [default](https://github.com/bitcoin/bitcoin/pull/27824#discussion_r1217967152) values, i.e., not setting `LLVM_TARGETS_TO_BUILD` altogether, works as well.

👍 ryanofsky approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1477609782)
Code review ACK ce88124bf499ae9608a2607ccddc00df3d7439bc. This looks great and I would encourage others to review. It should be much simpler to review now. Basically none of the commits are adding new code anymore, just moving code from one place to another.

Since my last review https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917 there were a lot of internal changes to make commits self-contained, but the only changes to the final code were making `LoadHDChain` catch ex
...

💬 mzumsande commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1227463946)
`IsChildWithParents()` is unused after this outside of tests - can we remove it?

💬 mzumsande commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1228398085)
I don't think it's unnecessary - the diamond package would be accepted if we remove this check, the subpackage algorithm doesn't help with this. However, I'm also not sure if it's sufficient - e.g. what if we'd add another "grandchild" to the child of the diamond package, with a feerate slightly above the previous package feerate (e.g. 5.1 sat/vB). Then this check wouldn't fail anymore, but the descendant feerate of the child (i.e. the combined fees of the child and the grandchild) could be bel
...

💬 mzumsande commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1228287010)
after c44df9b0bfa66c77e5cbbff6688ea73cf39ec1d3, the `packages.md` explanation "Packages must be child-with-unconfirmed-parents packages." is out of date. It would be good to document the definition of an "ancestor package" there instead.

🤔 Xekyo reviewed a pull request: "wallet: tx creation, don't select outputs from txes that are being replaced"
(https://github.com/bitcoin/bitcoin/pull/26732#pullrequestreview-1477680060)
Concept ACK

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228448590)
Split this out into its own commit.

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228449099)
I think this should be a bit better now?

💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1228455392)
No, going to change it.