💬 ajtowns commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397509594)
Could move `NetMsgMaker` entirely into `PushMessage` at that point, perhaps. *shrug*

🚀 fanquake merged a pull request: "doc: remove mingw-w64 install for "older" systems"
(https://github.com/bitcoin/bitcoin/pull/28900)

🤔 theStack reviewed a pull request: "wallet: batch all individual spkms setup db writes in a single db txn"
(https://github.com/bitcoin/bitcoin/pull/28894#pullrequestreview-1737343840)
Concept ACK

Didn't look deeper at the code yet, but ran the new benchmark (on an SSD) for comparison via
```
$ ./src/bench/bench_bitcoin -filter=WalletCreatePlain\|WalletCreateEncrypted
```

master (commit bb4554c81e0d819d74996f89cbb9c00476aedf8c which adds the benchmark)
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 797,
...

💬 ajtowns commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397534534)
It's not buggy, because blockstorage.h includes dbwrapper.h which includes streams.h; but I think it should just be including streams.h directly, and that seems to be what iwyu is recommending?

💬 maflcko commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397538918)
I know it isn't buggy, but it seems fragile to rely on `dbwrapper` having the include. iwyu used to be happy with a fwd decl, so maybe just keep it as-is?

💬 maflcko commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397543026)
A fwd decl should be enough, see also https://godbolt.org/z/7Txqjrh8e, unless I am missing something?

💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397547317)
> Could move `NetMsgMaker` entirely into `PushMessage` at that point, perhaps. _shrug_

I don't think so, because a message may be constructed once and then sent to several peers, so this wouldn't be a refactor and potentially cause a performance penalty?

📝 hebasto opened a pull request: "ci: Avoid toolset ambiguity that MSVC can't handle"
(https://github.com/bitcoin/bitcoin/pull/28905)
This PR introduces a workaround, which is similar to the one removed in https://github.com/bitcoin/bitcoin/pull/28796, required to work with the new windows-2022 image version [20231115](https://github.com/actions/runner-images/blob/win22/20231115.2/images/windows/toolsets/toolset-2022.json) properly.

Tested on the following image versions:
- [20231029.1.0](https://github.com/hebasto/bitcoin/actions/runs/6904313692/job/18784722567)
- [20231115.2.0](https://github.com/hebasto/bitcoin/actions
...

💬 maflcko commented on pull request "ci: Avoid toolset ambiguity that MSVC can't handle":
(https://github.com/bitcoin/bitcoin/pull/28905#issuecomment-1816718574)
Looks like this reverts 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da?

💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#issuecomment-1816721115)
Removed the class and replaced it with a template function in a namespace to address the feedback of both reviewers (Thanks!)

💬 hebasto commented on pull request "ci: Avoid toolset ambiguity that MSVC can't handle":
(https://github.com/bitcoin/bitcoin/pull/28905#issuecomment-1816721500)
> Looks like this reverts 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da?

Different versions though.

💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397571858)
> There's not really any need for a `NetMsgMaker` objects anymore after this change -- `NetMsgMaker::Make()` could just be a global/static function.

Thanks, done in https://github.com/bitcoin/bitcoin/pull/28892#issuecomment-1816721115

💬 sipa commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397572868)
There is a stale `PROTOCOL_VERSION` here I think.

💬 theuni commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397574627)
> Could move `NetMsgMaker` entirely into `PushMessage` at that point, perhaps. _shrug_

FWIW, NetMsgMaker was originally created exactly to prevent this for the sake of logical separation. The idea is/was that `net` shouldn't have any idea how to serialize a `CBlock`, rather it should just receive the dumb bytes.

💬 maflcko commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397581604)
> There is a stale `PROTOCOL_VERSION` here I think.

Are you sure? According to the Bitcoin wiki the first 4 bytes in the version message are there to " Identifies protocol version being used by the node ", so changing this may change the test, so better done in a follow-up?

The goal here is only to remove the *serialize* version and type.

💬 brunoerg commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1816739193)
utACK cc627169206fe902157806d88fcaf2b05701c38d

💬 sipa commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397595842)
Oof, yes, you're right.

💬 ajtowns commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397620257)
I don't think you're missing anything; but I don't really see the point of including a fwd declaration when that's not enough to be able to use the function, especially when the full header's already included indirectly? The tidy job says:

```
+#include "flatfile.h"
+#include "streams.h"
+#include "uint256.h"
```

which seems reasonable to me? When I run iwyu locally, it also seems to suggest primitives/block.h (rather than `class CBlock`), which is already included via chain.h anyway,
...

💬 ajtowns commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1397632706)
@theuni I think my idea there was that `template<typename T...> PushMessage(CNode& node, T... stuff)` that accepts `CBlock` and so on would be in net_processing.cpp, and that would call `CNode::PushMessage(msg)` after serializing? That would fit with https://github.com/ajtowns/bitcoin/commit/6be38ef045cd1b974ae13d705260e83b769a48b5 at least. I'm just making this up as I go along though :man_shrugging:

💬 maflcko commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#discussion_r1397653703)
> The tidy job says:

For reference, on master it doesn't say that. See for example https://cirrus-ci.com/task/5754375344226304 (https://api.cirrus-ci.com/v1/task/5754375344226304/logs/ci.log):

```
The full include-list for node/blockstorage.h:
#include <attributes.h> // for LIFETIMEBOUND
#include <chain.h> // for CBlockFileInfo, CBlockIndex
#include <dbwrapper.h> // for CDBWrapper
#include <kernel/blockmanager_opts.h> // for BlockMan
...