💬 hodlinator commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2415939359)
We have a winner! https://github.com/hodlinator/bitcoin/actions/runs/11311779692/job/31458399560
Downloaded the artifacts now just to be sure, but I won't be able to look at them until early next week.
Might keep the CI schedule running for one more dump before turning it off unless anyone disagrees.

💬 cdecker commented on pull request "chainparams: Re-add seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2415961306)
> No IPv6 support?

I might add ipv6 support, but still fighting with my cloud provider about that.

💬 cdecker commented on pull request "chainparams: Re-add seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2415961904)
> It currently doesn't give me any results for `x9`, IIRC the most common filter:
> ```
> host x9.seed.bitcoinstats.com.
> ```

Interesting, I'll check why that is.

🤔 hodlinator reviewed a pull request: "fees: Remove CLIENT_VERSION serialization"
(https://github.com/bitcoin/bitcoin/pull/29702#pullrequestreview-2370780537)
Concept ACK fa7c73c9d6db856e68309cfe00c5fd00d845a6d9

`nFileVersion` aka `nVersionThatWrote` was presumably [passed into](https://github.com/bitcoin/bitcoin/pull/29702/files#diff-7194b705e3c48e34fbd6e648fbbee39c6e0b49ee9658658b930678a9a2d31dd6L417) `TxConfirmStats::Read` in order to make it possible to deserialize the data differently for files written by older versions of the software to maintain some kind of upgrade path. Since no such backwards compatibility logic was implemented yet, it ca
...

💬 hodlinator commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1802068570)
nit: Even though it is `static` and thus file-local, I would still prefer it had a prefix mentioning "fees". Might be clearer in various development tools listing symbols even if it's clear to the compiler.
```suggestion
static constexpr int FEES_FILE_VERSION{149900};
```

💬 hodlinator commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1802069449)
`nFileVersion` aka `nVersionThatWrote` was presumably passed in here in order to make it possible to deserialize the data differently for files written by older versions of the software to maintain some kind of upgrade path. Since no such backwards compatibility logic was implemented yet, it can technically be removed as done now.

One could conceptually treat the remaining version value (presently still called `nVersionRequired`) as the version to perform backwards compatible reads for instea
...

💬 hodlinator commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1802081828)
nit: Could improve adjacent line:
```suggestion
throw std::runtime_error(strprintf("File version (%d) indicates it was generated by a newer version of the software.", nVersionRequired));
```

💬 willcl-ark commented on pull request "fees: document non-monotonic estimation edge case":
(https://github.com/bitcoin/bitcoin/pull/31080#discussion_r1802585830)
OK pushed that in [372ca14](https://github.com/bitcoin/bitcoin/pull/31080/commits/372ca14fbe0e716c789b74b766a8532f0f174268)

✅ Sjors closed a pull request: "ci: skip Github CI on branch pushes for forks"
(https://github.com/bitcoin/bitcoin/pull/30487)

💬 Sjors commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2416092451)
A use case that mentioned to me offline is that some people, before opening a PR to the main repo, like to push their branch and have CI check it. They then wait for CI on the branch to pass before opening a new PR. Their main reason for this is to reduce noise on the main repo.

It seems this PR breaks that use case. I suggest we reconsider it if and when Github adds support for custom ENV variables on a fork, like Cirrus does.

👍 laanwj approved a pull request: "test: Print CompletedProcess object on error"
(https://github.com/bitcoin/bitcoin/pull/31067#pullrequestreview-2371619568)
for reference, a CompletedProcess object is formatted like this:
```python
>>> print(subprocess.run(['/bin/ls'], capture_output=True, text=True))
CompletedProcess(args=['/bin/ls'], returncode=0, stdout='...', stderr='')
```
This contains some useful information, so ACK fa43c4f93ca5b40734ec9b3ff91b74acf3ed7cf2

🤔 laanwj reviewed a pull request: "Add Signet and testnet4 launch shortcuts for Windows"
(https://github.com/bitcoin/bitcoin/pull/26334#pullrequestreview-2371649358)
ACK cfd03de965a081facbd72316c76603dd7aa511bd

👍 laanwj approved a pull request: "Add Signet and testnet4 launch shortcuts for Windows"
(https://github.com/bitcoin/bitcoin/pull/26334#pullrequestreview-2371650372)
ACK cfd03de965a081facbd72316c76603dd7aa511bd

💬 willcl-ark commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2416121958)
From https://github.com/bitcoin/bitcoin/commit/e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33:

> When a contributor maintains a fork of the repo, any pull request they make
> to their own fork, or to the main repository, will trigger two CI runs:
> one for the branch push and one for the pull request.
> This can be avoided by setting SKIP_BRANCH_PUSH=true as a custom env variable
> in Cirrus repository settings, accessible from

GitHub supports setting custom env vars in each repo like Cirrus
...

✅ Sjors closed a pull request: "Stratum v2 Noise Protocol"
(https://github.com/bitcoin/bitcoin/pull/29346)

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2416132740)
I moved this to https://github.com/Sjors/bitcoin/pull/66 so we can focus on building an interface for external applications to use, and getting multiprocess in a release. I plan to open a tracking issue for that.

The followup PRs #30315 and #29432 will also be moved shortly.

@stratospher I will apply your feedback on the moved PR later.

✅ Sjors closed a pull request: "Stratum v2 Transport"
(https://github.com/bitcoin/bitcoin/pull/30315)

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2416152718)
I moved this to https://github.com/Sjors/bitcoin/pull/67 so we can focus on building an interface for external applications to use, and getting multiprocess in a release.

💬 laanwj commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1802670686)
Maybe factor this shovel-from-socketa-to-socketb loop out to a function in netutil as well.

💬 laanwj commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1802683914)
i don't think `sendall` can be guaranteed with non-blocking sockets, it will fail if it gets EAGAIN
https://stackoverflow.com/questions/6240737/python-socket-sendall-function