💬 ryanofsky commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1589584105)
This PR has a lot of review (6 stale acks), and adds some very good test coverage. So maybe with an update and few reacks it could be merged.
I had a question about 2 test functions that didn't appear to be called in: https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1224295479
I also don't think the change setting BLANK flank when DISABLE_PRIVATE_KEYS flag is set is a good one. BLANK seems like a confusing flag would be good to use less frequently rather than more frequently. Sett
...
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1589584105)
This PR has a lot of review (6 stale acks), and adds some very good test coverage. So maybe with an update and few reacks it could be merged.
I had a question about 2 test functions that didn't appear to be called in: https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1224295479
I also don't think the change setting BLANK flank when DISABLE_PRIVATE_KEYS flag is set is a good one. BLANK seems like a confusing flag would be good to use less frequently rather than more frequently. Sett
...
💬 ryanofsky commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1589588481)
This can be rebased now that #27708 is merged
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1589588481)
This can be rebased now that #27708 is merged
💬 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_r1228374362)
Removed
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228374362)
Removed
💬 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_r1228374497)
Removed
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228374497)
Removed
💬 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_r1228374607)
Removed
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228374607)
Removed
💬 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_r1228374846)
Done
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228374846)
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_r1228374949)
Done
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228374949)
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_r1228375177)
Added tests as suggested
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228375177)
Added tests as suggested
💬 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_r1228375315)
Added as suggested
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1228375315)
Added as suggested
💬 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
(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
(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
(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
(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
(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.
(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.
(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)
(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.
(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.
(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.
(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.