👍 guggero approved a pull request: "p2p: do not make automatic outbound connections to addnode peers"
(https://github.com/bitcoin/bitcoin/pull/28895#pullrequestreview-1737192944)
utACK 5e7cc4144bb09cfde48ba5bc055a2da3ca4252d1

I think it's important that nodes added by `addnode` always get the elevated protection level.

> A completely unrelated problem is that this problem exists because cjdns has so few peers <10? on mainnet, which is a shame considering how much engineering effort went into it, but I'm not sure what to do about that.

I'm still trying to wrap my head around CJDNS and how it can be set up in a docker based environment. If successful, I'll try cre
...

💬 hebasto commented on issue "v26.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1816644217)
> > All binaries are downloaded from [bitcoincore.org/bin](https://bitcoincore.org/bin/).
>
> Does it also happen if you compile from source?

It does.

> If yes, you could try bisecting (without having to go through guix builds)

Due to unknown for reason natively compiled binaries, at least `bitcoind -reindex-chainstate`, are significantly slower than Guix built ones. Therefore, I'm in the middle of bisecting using Guix builds.

💬 ajtowns commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#issuecomment-1816647954)
ACK fade05b27348f18d1f9cf8c0b5cc26eaffe6b452

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