:lock: hebasto locked an issue: "Bugreport"
(https://github.com/bitcoin-core/gui/issues/829)

👍 marcofleon approved a pull request: "Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305"
(https://github.com/bitcoin/bitcoin/pull/28263#pullrequestreview-2179899100)
Tested ACK 8607773750e60931e51a33e48cd077a1dedf9db3. Ran both targets for ~200 CPU hours. Coverage of intended targets looks good to me. The simulation of damaged keys and checks that follow seem useful as well.

📝 fanquake opened a pull request: "guix: bump time-machine to 3c58b759a51072aabd7eaaca680674a0c2b36c23"
(https://github.com/bitcoin/bitcoin/pull/30452)
Needed for https://github.com/bitcoin/bitcoin/issues/30210. This doesn't switch runtimes, because upstream is
still configured to use the old runtime. See:
https://git.savannah.gnu.org/cgit/guix.git/commit/?id=17188be0f723e00377b21b767f5447d7938a116e.

git-mimimal 2.45.1 -> 2.45.2
Kernel Headers 6.1.92 -> 6.1.98
LLVM 18.1.6 -> 18.1.8
mingw-w64 11.0.1 -> 12.0.0
patch 2.7.6 -> 2.7.6-0.f144b35

Draft; as the Windows build is now non-deterministic (across architectures), as atleast one sto
...

💬 fanquake commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2230620672)
> I've sent a patchset a while ago

Thanks. That has now landed, so we'll bump the time machine to get things started here. See #30452.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679202356)
Wow, that's a lot more complicated than I imagined, let's not touch it :D

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679203516)
Have you given it any more thought? I've tested it since, so i'll ack it, and will reack when you need more changes.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2230625876)
ACK 21090d032f5c4d90a41c3ca2030690c75899ec1a, nicely done!

🚀 fanquake merged a pull request: "Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305"
(https://github.com/bitcoin/bitcoin/pull/28263)

💬 GBKS commented on pull request "wallet: Improve error log color in the console":
(https://github.com/bitcoin-core/gui/pull/823#issuecomment-2230680718)
> cc @GBKS for a designer's opinion.

Personally, I find the new color more pleasant, and it goes better with the teal. It's a bit too toned down for me, and the old red does a better job at drawing attention and ensuring you look at it, which is appropriate for a scammer warning. I'd probably look at something in-between (strong red, but not jarring), and also tweak the other colors used in the logs so they all go together well and have good contrast on light and dark backgrounds (the teal ha
...

📝 maflcko opened a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453)
After `add_outbound_p2p_connection`, the test framework normally sends a version message only in reply to a received version. This is fine, but the protocol does not require this and tolerates a version to be sent earlier.

However, this is untested, and the missing test coverage leads to bugs being missed. For example https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2166824948

Fix it by adding a test.

💬 maflcko commented on pull request "test: Non-Shy version sender":
(https://github.com/bitcoin/bitcoin/pull/30453#issuecomment-2230681886)
Can be tested by adding a bug. For example:

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index d674758abd..e6f741a408 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5328,7 +5328,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt

// For outbound connections, ensure that the initial VERSION message
// has been sent first before processing any incoming messages
- if (!pfrom->IsInboundConn() &
...

💬 maflcko commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2230682823)
Added the missing test in https://github.com/bitcoin/bitcoin/pull/30453

📝 maflcko converted_to_draft a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453)
After `add_outbound_p2p_connection`, the test framework normally sends a version message only in reply to a received version. This is fine, but the protocol does not require this and tolerates a version to be sent earlier.

However, this is untested, and the missing test coverage leads to bugs being missed. For example https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2166824948

Fix it by adding a test.

💬 ryanofsky commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1679279840)
In commit "Pass coinbase constraints to createNewBlock" (e67e4669a9e263d90f0e276c8e65e93c39170375)

Current code seems ok, but depending on how important it is for this not to generate an invalid block, it may make sense to use `assert` instead of `Assume`, since Assume does not do anything in release builds. Or this could also abort in debug builds but cap the values in release builds:

```c++
if (!Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST)) {
optio
...

👍 ryanofsky approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2180053795)
Code review ACK 95c2edefe383c2b8a1735c26d8442fd36fa5e339

👋 maflcko's pull request is ready for review: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453)

💬 vasild commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#discussion_r1679318486)
`make_unique` takes the arguments to pass to the constructor which it will invoke internally. Passing it a `Sv2Client` temporary invokes the copy-constructor. Avoid that.
```suggestion
auto client = std::make_unique<Sv2Client>(id, std::move(sock), std::move(transport));
```

👍 ryanofsky approved a pull request: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2180077594)
Code review ACK 1e8f33ec15bc468385b2807e42d60c841c407681

💬 ryanofsky commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1679295004)
In commit "Add GetCoinBaseMerklePath helper" (c230cc1a72dc4e4079cb2f7b05586e6f32e844ab)

May want to declare this static to avoid creating an unused link symbol.

💬 ryanofsky commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1679318724)
In commit "Have createNewBlock return BlockTemplate interface" (b4782696a8ec5e21a41db3702d4b4dcc90ea40dd)

Could you move these member declarations to the bottom of the class? It makes it easier to see what state the interface accesses when state is declared at beginning or end of the class, not in the middle mixed with method definitions.

I'm also wondering if it might be more efficient to avoid the call to CBlock::GetBlockHeader():

```diff
--- a/src/node/interfaces.cpp
+++ b/src/nod
...