💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161810707)
Changed

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161810822)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161810914)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161811025)
Done

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161811758)
Either way someone will ask why it isn't done the other way, i.e. https://github.com/bitcoin/bitcoin/pull/17578#pullrequestreview-338232718, so I went with the smallest diff.

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1161841784)
Done as suggested

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1161841837)
Done as suggested

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1161841922)
Removed

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1161841985)
Done as suggested

💬 theuni commented on pull request "contrib: followups to #27358 (verify-binaries)":
(https://github.com/bitcoin/bitcoin/pull/27440#discussion_r1161851409)
Yeah, I think this is the crux of the thing. Agree that we need a domain argument for this script to be generic enough to be useful. So I propose that we (as yet another follow-up):
- Document the current assumed remote dir structure and call it the standard*.
- Hook up a domain option that's intended to work with above well-documented standard.

This would allow for a crude federated model of pulling that suits the multisig model well, so I think it's a good way forward for us.

* Obvious
...

👍 ryanofsky approved a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217#pullrequestreview-1377763599)
Code review ACK 98821a7aca88ac9401786e848899866b71446460. Just suggested changes since last review, the main one being changing `translateTransactionType` to use a switch statement.

This had 3 reviews before, so it would be nice to have reacks and hopefully merge this soon.

👍 theuni approved a pull request: "contrib: followups to #27358 (verify-binaries)"
(https://github.com/bitcoin/bitcoin/pull/27440#pullrequestreview-1377799833)
ACK ad841608d4edf6151b60e483793f60ba9f03cdbf. Thanks for doing these.

Anybody interested in taking https://github.com/bitcoin/bitcoin/pull/27440#discussion_r1161268754 ? :)

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161892675)
Done.

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161898967)
It would only be broken if the word "warnings" is backported into the loadwallet help in a previous release, but to address your request replaced this line with `if int(node_master.getnetworkinfo()["version"]) >= 249900:`

🤔 Xekyo reviewed a pull request: "wallet: group outputs only once, decouple it from Coin Selection"
(https://github.com/bitcoin/bitcoin/pull/25806#pullrequestreview-1377824610)
Post merge ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44

💬 Xekyo commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1161893162)
```suggestion
if (!positive_only || (coin.GetEffectiveValue() > 0)) {
```

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161912974)
Done.

💬 sdaftuar commented on pull request "refactor, net processing: Avoid CNode::m_relays_txs usage":
(https://github.com/bitcoin/bitcoin/pull/27270#issuecomment-1502120989)
utACK

>CNode::m_relays_txs is meant to only be used for the eviction logic in net. TxRelay::m_relay_txs will hold the same value and is meant to be used on the application layer to determine if we will/should relay transactions to a peer.

@dergoegge Thanks for mentioning this understanding -- I hadn't looked at this code in a while, but I agree with this and I think this is helpful for everyone to understand as we do more work in this module.

👍 Xekyo approved a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217#pullrequestreview-1377895422)
ACK 98821a7aca88ac9401786e848899866b71446460

💬 Xekyo commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161952231)
I thought here first that if the user does not set a label, we might treat an address as change. However, I realized that an empty string `""` is truthy, while no string being set is falsy, i.e. even if the address label is set to `""` which appears to be the case for any receive addresses, it’ll be truthy.