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

💬 martinus commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1162043929)
> I see that there is a PASTE macro that can be used instead of directly using the token-pasting operator (##), but I can't figure out why it exists and if it offers any advantage.
https://github.com/bitcoin/bitcoin/blob/db720b5a703c90625fa7a4773bd2db5672427cbe/src/util/macros.h#L8

The macro is a bit badly named, the main one here is `PASTE2`. The advantage is that due to the indirection you can use macros as arguments in the macro and they will be expanded before concatenation.
See e.g. https:
...

💬 josibake commented on pull request "contrib: followups to #27358 (verify-binaries)":
(https://github.com/bitcoin/bitcoin/pull/27440#discussion_r1162070190)
> Anybody interested in taking [#27440 (comment)](https://github.com/bitcoin/bitcoin/pull/27440#discussion_r1161268754) ? :)

I can take a crack at it, I already started working on a proof of concept for searching for the file name regardless of the folder structure

💬 mzumsande commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1502320345)
> It is still possible to guess mempool transactions using `getdata` by maintaining mempool with no restrictions.

This doesn't work for new transactions, a node answers a `getdata` request for a tx it didn't announce to a peer via an `inv` if that transaction is older than `UNCONDITIONAL_RELAY_DELAY` (2 minutes), see [here](https://github.com/bitcoin/bitcoin/blob/d544d03ba6c7ed3c5879c8bc1108f45d0aac2798/src/net_processing.cpp#L2261-L2263). This is meant to protect against transaction origin d
...

📝 fanquake opened a pull request: "test: build Valgrind (3.20) from source & use Clang 16"
(https://github.com/bitcoin/bitcoin/pull/27444)
I was originally going to update to using Clang-16 & Valgrind 3.19 ([via Ubuntu 23.04](https://packages.ubuntu.com/lunar/valgrind)), however ran into issues with 3.19 & aarch64 (testing on top of #27364). Instead, I decided to switch to building the latest version of Valgrind from source (which takes minimal effort). This seems to have fixed all issues.

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1162194379)
Fixed.

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1162194453)
I've made this clearer by using `has_value()`.

📝 sipa opened a pull request: "Update src/secp256k1 subtree to release v0.3.1"
(https://github.com/bitcoin/bitcoin/pull/27445)
There is no urgent need for any of the changes in v0.3.1 (compared to the v0.3.0 that's currently subtreed), but if anyone may compile Bitcoin Core from source using Clang v14+, this will prevent known timing leaks in the signing/keygen logic.

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1162194642)
I prefer the `else if` style, but have collapsed this and `PurposeToString` to be more readable.

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1162194740)
Done.

💬 achow101 commented on pull request "contrib: followups to #27358 (verify-binaries)":
(https://github.com/bitcoin/bitcoin/pull/27440#issuecomment-1502548625)
ACK ad841608d4edf6151b60e483793f60ba9f03cdbf

🚀 achow101 merged a pull request: "contrib: followups to #27358 (verify-binaries)"
(https://github.com/bitcoin/bitcoin/pull/27440)

💬 achow101 commented on pull request "doc: correct sqlite & qrencode versions used in depenendencies.md":
(https://github.com/bitcoin/bitcoin/pull/27441#issuecomment-1502549784)
ACK a12d9cfa46ad5f5a5144daabbc146d0175642c69