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

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

Add a test to check this is indeed tolerated.

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters once per tip change":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2230965781)
reACK c85accecafc20f6a6ae94bdf6cdd3ba9747218fd

reviewed via `git range-diff master 17d41e7 c85acce`

💬 maflcko commented on pull request "test: Non-Shy version sender":
(https://github.com/bitcoin/bitcoin/pull/30453#issuecomment-2230968900)
Made a draft for now, due to failing CI. Also, edited OP to remove the sentence that implied this was a regression test for a known bug. (I don't think this checks for a previously known bug)

<!--

Arguably, https://github.com/bitcoin/bitcoin/pull/30453#issuecomment-2230681886 just shows a bug in the test framework, because `p2p_conn.wait_for_verack()` should also wait on the version msg being received from the node first.

So what the test checks is that the outbound version is sent bef
...

🤔 furszy reviewed a pull request: "index: Check all necessary block data is available before starting to sync"
(https://github.com/bitcoin/bitcoin/pull/29770#pullrequestreview-2180361000)
> > It would be nice to implement [81638f5](https://github.com/bitcoin/bitcoin/commit/81638f5d42b841fb327393974320ca47db39eb09) differently, without adding the `<chain.h>` dependency to all index headers. This dependency provides access to node internal types that indexes running on other processes (in the future) will not know about.
>
> Should not be too complicated to get rid of the dependency. I will draft something, maybe I'll just give @maflcko s [suggestion](https://github.com/bitcoin/
...

🚀 ryanofsky merged a pull request: "kernel: De-globalize static validation variables"
(https://github.com/bitcoin/bitcoin/pull/30425)

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2231054406)
Getting a mismatch again. This time for block 716120 (00000000000000000009bc9db59a266e367f54fdd4cda7b5ddb31f6ae9723083).

```
bitcoin-cli getsilentpaymentblockdata 00000000000000000009bc9db59a266e367f54fdd4cda7b5ddb31f6ae9723083 | jq '.bip352_tweaks | sort'
[
"031799df7770cfdabe460e47f4571fe4ded7d7a7922afa0f5ad91753473269cdbb",
"0323fefb4e4bf637d7819c2030996dbe88d97c6a2f5708841df0e65fafd32c5a0a",
"03b2bda3a513123fe810cb1e65cd8a71a2495ea9229c50e4acc8d4b5baa51b4147",
"03cdc8c9f07d
...