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

🤔 hebasto reviewed a pull request: "build: use `-no_exported_symbols` on macOS"
(https://github.com/bitcoin/bitcoin/pull/29072#pullrequestreview-2180434936)
Tested 81d4dc8e8739df0e9a8e92f55071733f6500617b on macOS 14.5 Sonoma (Apple Silicon):

- the master branch @ 9c5cdf07f30f816cd134e2cd2dca9c27ef7067a5:
```
% nm --extern-only src/bitcoind | wc -l
579
```
- this PR:
```
% nm --extern-only src/bitcoind | wc -l
537
```

I don't know the reasons, but `bitcoind` still exports
```
0000000100000000 T __mh_execute_header
```

The new `-no_exported_symbols` linker flag is
...

💬 theStack commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1679523217)
> Can you retest after todays rebase?

Sure. Retested on commit d0019f290ab67ab4190274bb325bfc29bbfcea56, can confirm that the BIP352 indexer on signet runs through (up to current block 204598) now without crashing.

💬 fanquake commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2231067816)
> I don't know the reasons, but bitcoind still exports

That is expected. See the PR description.

> The new -no_exported_symbols linker flag is not printed in the configure summary.

That's expected. We don't print the RE LD flags.

💬 mzumsande commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2231073104)
I didn't find a reference in a BIP (there isn't any for the general version message flow as far as I can see) and maybe "violation" is too strong, but any message received before the version would be ignored by bitcoin core peers:
https://github.com/bitcoin/bitcoin/blob/46878326808f643f08ec3f69dcec5c8fafeb7b59/src/net_processing.cpp#L3889-L3893
So this is clearly not the intended way the protocol should work.

👍 theuni approved a pull request: "build: use `-no_exported_symbols` on macOS"
(https://github.com/bitcoin/bitcoin/pull/29072#pullrequestreview-2180454092)
utACK 81d4dc8e8739df0e9a8e92f55071733f6500617b

👍 hebasto approved a pull request: "build: use `-no_exported_symbols` on macOS"
(https://github.com/bitcoin/bitcoin/pull/29072#pullrequestreview-2180461920)
ACK 81d4dc8e8739df0e9a8e92f55071733f6500617b.

> > The new -no_exported_symbols linker flag is not printed in the configure summary.
>
> That's expected. We don't print the RE LD flags.

Not related to this PR changes, but that's odd as it undermines the summary's goal.

🚀 fanquake merged a pull request: "build: use `-no_exported_symbols` on macOS"
(https://github.com/bitcoin/bitcoin/pull/29072)

💬 mzumsande commented on pull request "locks: introduce mutex for tx download, flush rejection filters once per tip change":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1679560788)
Makes sense! In `net.cpp`, the `AssertLockNotHeld(x)` / `LOCK(x)` pattern is so common that I didn't even consider the possibility that this could be on purpose.