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

💬 stickies-v commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163968385)
nit: we use snake case now ([developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c)), I think it's fine to have this one be snake case and `Consensus::Params::nPowTargetSpacing` be camel case.

```suggestion
std::optional<int64_t> pow_target_spacing{};
```

💬 stickies-v commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163988830)
I think the doc also needs to specify that `-signetchallenge` is required when using this option

💬 stickies-v commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163975357)
nit: I'd define the default in a variable and use it here to avoid hardcoding the default here so it doesn't go out of sync

💬 stickies-v commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1163959320)
Would `InitParameterInteraction()` (`init.cpp`) be a better place for this?

⚠️ BataBoom opened an issue: "cli not returning data"
(https://github.com/bitcoin/bitcoin/issues/27450)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I created a new wallet, then deleted the new wallet returning it to what it was (1 wallet) or so I think. The logs look good from what I can tell but when I run bitcoin-cli getbalance, getnewaddress, etc it just stalls and doesnt return anything?

I should have made a new wallet over litecoin, id much rather have that down than a bitcoin gateway. ooooF. Any ideas/help is greatly apprecia
...

✅ BataBoom closed an issue: "cli not returning data"
(https://github.com/bitcoin/bitcoin/issues/27450)

💬 carnhofdaki commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1505220494)
How does it work in Elements (Blockstream Liquid)? Is the 1-minute block time hardwired there?

I agree with @ajtowns this may cause inconsistencies (and I would add "confusion"). For example nodes that will not set the parameter to the same value as miners will simply refuse the blocks which do not fit their point-of-view. So in the end the parameter would need to be distributed the same way as `signetchallenge` (which is a once and for all setting) and can not be changed on-the-fly on an alr
...

📝 tarunraheja84 opened a pull request: "updated"
(https://github.com/bitcoin/bitcoin/pull/27451)
Steps to compile Bitcoin Core in Windows (most used OS) inserted.