💬 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

💬 instagibbs commented on issue "Why does `submitpackage` require at least two transactions":
(https://github.com/bitcoin/bitcoin/issues/31085#issuecomment-2416189977)
I think it's reasonable to support. I'll take a look.

💬 laanwj commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1802716839)
nit: please put multiple statements on multiple lines, this is pretty hard to read code as-is :sweat_smile:

💬 laanwj commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2416224594)
ACK on overall change, small nit

📝 instagibbs opened a pull request: "Package validation: accept packages of size 1"
(https://github.com/bitcoin/bitcoin/pull/31096)
There's no particular reason to restrict single transaction submissions with submitpackage. This change relaxes the RPC checks as well as the `IsChildWith*` checks to allow these cases, as well as return false rather than segfault with an empty package.

Resolves #31085

💬 instagibbs commented on issue "Why does `submitpackage` require at least two transactions":
(https://github.com/bitcoin/bitcoin/issues/31085#issuecomment-2416273464)
Opened #31096