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

💬 vasild commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2230816117)
@Sjors, thanks for your suggestions at https://github.com/vasild/bitcoin/commit/f0dc62f8ab773ef7cbf44ba7119d08af66506428. I extended `DynSock` to be able to `Accept()` new connections, returning a socket that has been provided by the tests.

Now the test `sv2_connman_tests/client_tests` uses mocked sockets and passes!

See https://github.com/vasild/bitcoin/commits/sv2_mocksock/ which is:

* This PR, modified to handle `Accept()`. If you like that I will push the mods to this PR.
* Plus ht
...

💬 maflcko commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2230847225)
Re: https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2166824948


> In that case, I think that `ProcessMessage` could send out other messages (WTXIDRELAY etc.) before sending the version message, which would be a protocol violation.

Can you add a reference for this? In the BIP I could only find "The wtxidrelay message MUST be sent in response to a version message from a peer", not that it has to be sent *after* the own version.

📝 ryanofsky unlocked a pull request: "coins: allow write to disk without cache drop"
(https://github.com/bitcoin/bitcoin/pull/17487)
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):

Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

---

In certain circumstances, we may want to flush chainstate data to disk without
emptying `cacheCoins`, which affects performance. UTXO snapshot
activation is one such case, as we populate `cacheCoins` with the snapshot
contents and want to persist immediately afterwards but also
...

💬 andrewtoth commented on pull request "coins: allow write to disk without cache drop":
(https://github.com/bitcoin/bitcoin/pull/17487#discussion_r1679399182)
This is done in #28280.