💬 maflcko commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2306438411)
lgtm ACK cb89c8dc6a5c3fbcc73b92f4d87433c57b6ed000

💬 TheCharlatan commented on pull request "validation: improve m_best_header tracking":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1728579922)
I've been looking the logic over again and think it might be a good idea to call `NotifyHeaderTip` here too. It basically tracks along `m_best_header`, but with the current code will only notify when processing the next block or header.

💬 sipsorcery commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2306583830)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4.

Tested successful builds on:
- Win11 + msvc x64 dynamic and static
- Win11 + WSL Ubuntu 24.04 + mingw32

I couldn't perform the build on Win11 + WSL + Ubuntu 20.04 as the g++-mingw toolset not available. I don't think this is a problem since it's trivial to install a newer Ubuntu version on WSL.

💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306601521)
The cases where enabling full-rbf causes problems are very rare: you need to be running a service that is _not_ vulnerable to unconfirmed double-spends in general. Yet _does_ have some kind of bug related to double spends. And those rare services can still easily turn full-rbf off with a single config setting.

Meanwhile, the harm to compact block propagation caused by not enabling full-rbf can be seen as a bug that should be fixed.

💬 glozow commented on pull request "contrib: add dockerfile for building image fron source code":
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2306619681)
> * In a distributed development environment (I mean development for bitcoin core), an available dockerfile can help independent developers align the compilation environment.

I'm pretty sure dev environment convergence is something we explicitly *don't* want for Bitcoin Core.

> In any case, have you seen the existing CI dockerfiles, which are probably what you are looking for? If not, it would be good to explain the difference.

+1

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2306678413)
`1dcd626fe1...587d996f07`: rebase, get the extra printout from https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2245216013 and resolve a silent merge conflict with https://github.com/bitcoin/bitcoin/pull/28052

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2306692234)
> TESTED_TX_RESULTS is missing TxValidationResult::TX_RECONSIDERABLE

That's an oversight, I've added that and UNKNOWN now which should mean we see packages.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728679985)
I wrote the assertion incorrectly. The only combination that's not ok is both.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728680149)
added

💬 virtu commented on pull request "[WIP] seeds: Add additional seed source and bump uptime requirements for Onion and I2P nodes":
(https://github.com/bitcoin/bitcoin/pull/30695#discussion_r1728710468)
I hadn't regenerated to node lists because I wanted to wait until Ava's seeder resumed exporting good I2P node data. I have now pushed a preliminary export anyway. Everything should be there.

💬 virtu commented on pull request "[WIP] seeds: Add additional seed source and bump uptime requirements for Onion and I2P nodes":
(https://github.com/bitcoin/bitcoin/pull/30695#issuecomment-2306750156)
Can you recheck for the peers that were missing? [On a side note, I noticed what (based on the user agent string) might be your cjdns node is no longer part of the cjdns node list because the node didn't have the NODE_NETWORK version bit set.]

Also, how do you define very good nodes? Right now, seeds are selected more or less randomly from the set of nodes that pass all selection criteria. Instead of doing a `random.shuffle()`, it might be a better idea to sort them based on availability.

💬 fanquake commented on pull request "build: add `standard branch-protection` to hardening flags for aarch64-linux":
(https://github.com/bitcoin/bitcoin/pull/30433#discussion_r1728766663)
The same logic as all other flags applies, the user has the final say.

💬 fanquake commented on pull request "build: add `standard branch-protection` to hardening flags for aarch64-linux":
(https://github.com/bitcoin/bitcoin/pull/30433#discussion_r1728767510)
I'll take a look, and we can backport something if relevant.

💬 fanquake commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2306840123)
> the statement from the PR description:
> is not accurate.

That text is from the GCC documentation, and in this context, of, the compiler being configured for CET, after configuring with `--enable-cet`, as far as I'm aware, it is accurate. If you think it isn't, can you file an issue upstream, and link it here it?

The point of this PR has never been to add any additional metadata to the binaries/make any other changes, the point is just to explictly use the hardening option when configur
...

💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306867512)
> Yeah it might be too much zeal, yet I’ve been surprised when the option was introduced with `24.x` how much services were still claiming to rely zero-conf acceptance.

Only one service, GAP600, claimed to still rely on zero-conf and they refused to actually provide any examples of clients actually using them. I think it's highly likely that GAP600 was actually a scam that took peoples' money and provided nothing of value. Us continuing to claim that zeroconf services still exist is actually
...

💬 fanquake commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306877771)
I'm just going to backport the other two fixes here, and will follow up with any other changes before the next 27.x.

💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306879047)
@fanquake That's perfectly reasonable. Concept ACK.

👍 stickies-v approved a pull request: "[27.x] Even more backports"
(https://github.com/bitcoin/bitcoin/pull/30558#pullrequestreview-2257069799)
ACK b06c4c6550545351610fc3278dffdd63d5954cf8

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728863544)
```suggestion
const std::unique_ptr<TxDownloadManagerImpl> m_impl;
```

👉👈

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728864722)
```suggestion
FUZZ_TARGET(txdownloadman_impl, .init = initialize)
```

(if you take the other naming suggestion)

👉👈

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728864031)
```suggestion
FUZZ_TARGET(txdownloadman_one_honest_peer, .init = initialize)
```

👉👈