💬 MarcoFalke commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163735086)
> This fails the linter, could use ParseInt64().

Is there a reason to not just use `GetIntArg` directly? If not, for new code, `ToIntegral` should be used, unless the legacy `ParseInt` behavior of accepting a leading `+` is needed?

💬 MarcoFalke commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163736613)
Any reason to leak implementation details (`__func__`) to the user when it doesn't add any value? `-signetblocktime must be greater than 0` should be self-explanatory?

💬 sdaftuar commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1504865608)
> If you control both nodes, wouldn't it be easier to run the RPC on the sending side directly to fan out all transactions instead of going an extra hop over P2P?

@MarcoFalke Yeah I think this is a fair point... And I guess really if there were a use case, it would be for users of other software interacting with Bitcoin Core and wanting some functionality like this without having to recompile their Bitcoin Core node to run a custom patch (a situation that does not apply to me!).

👍 fanquake approved a pull request: "doc: update OpenBSD build docs for 7.3 (external signer support available)"
(https://github.com/bitcoin/bitcoin/pull/27449#pullrequestreview-1380822649)
ACK 6b17994ede6fe1961667d2e96127291b2a8b4f9d - haven't tested, but looks good to me.

💬 josibake commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1504894418)
reACK https://github.com/bitcoin/bitcoin/commit/18fc71a3adee5de0f74ba6ff18f5ed31ba79a646

🚀 fanquake merged a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217)

💬 willcl-ark commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1505002617)
Thanks for the comments all. I am currently editing a ML post to get feedback from a wider audience too, as sensibly suggested.

Replies to some comments below:

> It could still be helpful in testing. Since we have lot of inconsistencies in mempool policies recently for political reasons, I think we should keep this P2P message available at least for testing if NetPermissionFlags::Mempool flag cannot be removed.

AFAIK even for use in "testing" this would require custom code or a patched
...

💬 fanquake commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1505059300)
> would a higher timeout fix this in your environment?

I bumped the timeout, and it didn't seem to make any difference.

> Are you doing some extreme parallelization or someting, such that the test is simply that slow?

Nothing like that, and the machine isn't particularly slow. i.e it has no issue running various valgrind and other CI jobs. I'll take another look.

💬 fanquake commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1505069287)
> No objection if you want to use bookworm

Ok. I've just moved to `debian:bookworm` for now. That gives us clang-15 and valgrind 3.19 to use.

I'll split out the compile-from-source change, as I'd still like to improve support for aarch64, and don't think the overhead of compiling Valgrind is much.

🤔 jnewbery reviewed a pull request: "Remove BIP35 mempool p2p message"
(https://github.com/bitcoin/bitcoin/pull/27426#pullrequestreview-1381043081)
Concept ACK, as long as there aren't responses to the ML post that reveal that this is a widely used feature.

💬 jnewbery commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#discussion_r1163968663)
I think it makes sense to combine the two remaining `if (fSendTrickle)` blocks now, since `fSendTrickle` can't be modified between them:

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 2a223a2adf..386783a46c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5568,14 +5568,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
}
}

- // Time to send but the peer has requested we not relay
...

💬 jnewbery commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#discussion_r1163960285)
This can be combined with the conditional above now:

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 2a223a2adf..050563ea95 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2252,12 +2252,10 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const GenTxid& gtxid, const std::chrono::seconds now)
{
auto txinfo = m_mempool.i
...

💬 jnewbery commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#discussion_r1163971096)
There seems to precedence for updating the document to say when support was removed, rather than deleting the line entirely (see BIP 61 below).

💬 glozow commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1163980505)
Yes you're right, this should be checking the child. Edited this to be checking that "This would be enough to pay for the child alone."

👍 MarcoFalke approved a pull request: "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs"
(https://github.com/bitcoin/bitcoin/pull/27444#pullrequestreview-1381080456)
lgtm

💬 MarcoFalke commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1163983711)
You can use gcc and drop it, if you want

💬 MarcoFalke commented on pull request "ci: use LLVM/Clang 15 and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1505093759)
> to improve support for aarch64

What is the error message on aarch64? Does it happen with gcc and clang?

🤔 stickies-v reviewed a pull request: "Allow configuring target block time for a signet"
(https://github.com/bitcoin/bitcoin/pull/27446#pullrequestreview-1381041535)
Concept ACK, I've seen the need for signet custom block times come up quite often and I think it makes sense.

💬 stickies-v commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163966920)
I think this can be simplified with `GetIntArg` which returns a `std::optional`:

<details>
<summary>git diff</summary>

```diff
diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index f41d2aaf8..02d51039e 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -27,19 +27,14 @@ void ReadSigNetArgs(const ArgsManager& args, CChainParams::SigNetOptions& option
}
options.challenge.emplace(ParseHex(signet_challenge[0]));
}
- if (args.IsArgSet("-signe
...

💬 stickies-v commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163982117)
I think this can be simplified quite a bit by just initializing `SigNetOptions::nPowTargetSpacing` with a default value:

<details>
<summary>git diff</summary>

```diff
diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp
index 7f66a6b8e..32905af48 100644
--- a/src/kernel/chainparams.cpp
+++ b/src/kernel/chainparams.cpp
@@ -290,7 +290,6 @@ public:
explicit SigNetParams(const SigNetOptions& options)
{
std::vector<uint8_t> bin;
- int64_t nPow
...