💬 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.

💬 Xekyo commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161939317)
Nit, typo:
```suggestion
* receiving addresses since BIP70 payment protocol support was added in
```

💬 Xekyo commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161954369)
Nit, since each immediately returns, this may be more readable:

```suggestion
if (s == "receive") return AddressPurpose::RECEIVE;
if (s == "send") return AddressPurpose::SEND;
if (s == "refund") return AddressPurpose::REFUND;
```

💬 Xekyo commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161967546)
Perhaps mention that the “purpose string” appears in the context of addresses, at least for me that would not necessarily have been obvious just from this release note without reviewing the PR.

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#issuecomment-1502196628)
Thank you @pinheadmz, @vasild and @TheCharlatan. Repushed to address feedback, should be trivial to re-ACK.

<details><summary><code>git diff e67da5f 7ccdd74</code></summary><p>

```diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index fc7d5a5c282..fb175fa2532 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -1379,7 +1379,7 @@ RPCHelpMan importmulti()

for (const UniValue& data : requests.getValues()) {
const int
...

🤔 mzumsande reviewed a pull request: "fuzz: Add HeadersSyncState target"
(https://github.com/bitcoin/bitcoin/pull/26662#pullrequestreview-1378034873)
ACK 3153e7d779ac284f86e433af033d63f13f361b6f

I reviewed the fuzzing code and let the fuzzer run for a while with no crashes. I also verified that it does reach most of the code in `headerssync.cpp`.